static code analysis

[vladopajic]

This issue lists suggestions for static code analysis with goal to ensure higher code quality.

1. Have single source for linting rules symmetrical in CI/CD and local environments Bee code currently has three different rules and only one of those is enforced by CI. Problems with this way of linting: a) it causes confusion what are rules to be followed, b) rules which are not enforced by CI are going to be forgotten c) rules executed in CI/CD should be symmetrical to local (so devs can check code locally) What could be done: have single source for all linting rules in .golangci.yml and single make lint command to run those lints on entire code base.

2. Trailing white space check only works in CI This check seems to be only working in CI on diff changes. It would be easier for devs if this check is symmetrical in CI/CD and local environments. What could be done: implement command make whitespace-check which would check entire code base.

3. Increase golangci-lint strictness In order to ensure higher code quality bee code base should consider including more linters. Below are suggested linters bee should include:

3.1) Compliant rules list List includes rules which bee can start using right away because code is compliant with them.

    - asciicheck
    - bidichk
    - deadcode
    - depguard
    - errcheck
    - gofmt
    - goheader
    - gomoddirectives
    - goprintffuncname
    - gosimple
    - govet
    - importas
    - ineffassign
    - megacheck
    - misspell
    - misspell
    - nakedret
    - nilerr
    - promlinter
    - rowserrcheck
    - sqlclosecheck
    - staticcheck
    - structcheck
    - typecheck
    - unconvert
    - unconvert
    - unused
    - varcheck

3.2) Easy to fix list List of rules which could be easily fixed (there are few problems in the code witch needs to be addressed)

  - errorlint ##important
  - errname
  - dogsled
  - bodyclose
  - exportloopref
  - forbidigo
  - goconst
  - gochecknoinits
  - gofumpt
  - nilnil
  - nlreturn
  - noctx
  - prealloc
  - predeclared
  - wastedassign
  - whitespace

3.3) Test related List includes rules which affect how tests are written. These seem to be relatively easy to be fixed.

 - paralleltest ## important **
 - tparallel 
 - testpackage
 - thelper

** Using parallel test can reduce total test execution time. At the same time it enforces tests quality (for example using global variables in those tests can cause race condition).

3.4) High impact rules These rules have high impact on code quality. Fixing some of those need more effort compared to linters above. Since fixing all issues could require bigger effort, some issues could be fixed and some temporally ignored in order to get these rules enabled for future code changes.

  - cyclop           # many issues; improves code readability; can start with some higher value then reduce it
  - dupl             # many issues; improves code quality
  - gochecknoglobals # many issues
  - gocognit         # many issues; improves code readability
  - gocyclo          # many issues; improves code readability
  - gocritic         # many issues; bugs, performance and style issues.
  - goerr113         # uniform error handling
  - nestif           # improves code readability
  - revive           # style issues; improves code readability (make code uniform)
  - stylecheck       # style issues; improves code readability (make code uniform)
  - unparam          # style issues; code quality
  - gomnd            # style issues; improves code readability 
  - makezero         # performance
  - wrapcheck        # many issues; improves error handling
  - ifshort          # style issues; improves code readability 

4. License header There is no tool for checking code license in .go code. As it currently requires developers in code review process to ensure this it can lead to code being pushed without this header. What could be done: implement command make header-check which would check if all files have necessary headers.

[mrekucci]

• megacheck

Is deprecated.

• rowserrcheck
• sqlclosecheck

Only relevant to SQL.

• bodyclose

Not needed for server-side since: https://github.com/golang/go/blob/master/src/net/http/request.go#L174-L177

• gofumpt

We should stick with gofmt.

• nilnil

Is too restrictive, consider a valid case where you return a nil (a.k.a empty) slice and no error: return nil, nil.

• nlreturn

I'd drop this since It's opinionated.

• gocyclo
• cyclop

Those two seem to be doing the same.

• revive
• stylecheck

Not sure what they bring on top of the golint?

• ifshort

Also, this seems too restrictive, sometimes you just need to do it on a separate line because in one line it's too long.

4. License header There is no tool for checking code license in .go code. As it currently requires developers in code review process to ensure this it can lead to code being pushed without this header. What could be done: implement command make header-check which would check if all files have necessary headers.

The goheader linter you've mentioned above does this: https://github.com/denis-tingaikin/go-header.

Overall, I would agree with the linters in sections 3.1, 3.2 and 3.3, except for those I specifically mentioned. However, some of the linters in section 3.4 are opinionated/restrictive, so I wouldn't recommend including them at this time and we can revisit them later.

[vladopajic]

Closing this Issues, after enabling all linters