diff --git a/.golangci-maximal.yml b/.golangci-maximal.yml new file mode 100644 index 0000000..78c5318 --- /dev/null +++ b/.golangci-maximal.yml @@ -0,0 +1,183 @@ +linters: + disable-all: true + enable: + - copyloopvar + - cyclop + - depguard + - dupword + - errcheck + - errname + - errorlint + - exhaustruct + - exptostd + - funlen + - gci + - gocheckcompilerdirectives + - gocognit + - gocritic + - gocyclo + - godot + - goimports + - govet + - iface + - importas + - ineffassign + - intrange + - maintidx + - misspell + - nestif + - nilnesserr + - nolintlint + - predeclared + - recvcheck + - revive + - staticcheck + - testifylint + - unconvert + - unparam + - unused + - usestdlibvars + - usetesting + fast: false + +linters-settings: + depguard: + rules: + # This is just an example of a rule you could create, though + forbid-some-pacakge: + list-mode: Lax + files: + - "**/*.go" + deny: + pkg: "github.com/some/package" + desc: "Don't use some/package. It's got issues." + + errcheck: + check-type-assertions: true + + gci: + sections: + - standard + - default + + # This configure gocritic so that it is _only_ used to run ruleguard. It's default checks include + # some that can be a bit annoying, like insisting on using `switch` instead of if/else all over + # the place. + gocritic: + disable-all: true + enabled-checks: + - ruleguard + settings: + ruleguard: + rules: "${configDir}/gorules/*.go" + # This causes a warning when rules cannot be compiled. + failOn: dsl + + + govet: + # This is not part of the default checks, but it's useful for finding bugs and generally + # confusing code. + shadow: true + + importas: + no-extra-aliases: true + alias: + - pkg: github.com/google/uuid + alias: guuid + + misspell: + locale: US + extra-words: + - typo: "cancelation" + correction: "cancellation" + - typo: "cancelations" + correction: "cancellations" + - typo: "cancelling" + correction: "canceling" + - typo: "cancelled" + correction: "canceled" + + recvcheck: + exclusions: + # It's normal for `Marshal*` methods to not take a pointer. + - "*.MarshalBSON" + # We have some structs that are passed around as non-pointers, but `UnmarshalBSON` always + # takes a pointer since it's populating the receiver directly. + - "*.UnmarshalBSON" + + revive: + enable-all-rules: false + # revive has a lot of possible rules. This is a bare minimum config. Check out the revive docs + # at https://github.com/mgechev/revive for more options. + rules: + - name: argument-limit + arguments: [5] + - name: function-result-limit + arguments: [3] + - name: import-shadowing + - name: unused-parameter + - name: unused-receiver + + testifylint: + enable: + - bool-compare + - compares + - empty + - error-is-as + - error-nil + - expected-actual + - float-compare + - len + - require-error + - suite-extra-assert-call + require-error: + # The default is to insist on require not only for "no error" checks but also for "error is X" + # checks, which is weird. + fn-pattern: ^NoErrorf?$ + + unused: + # If a field is written but never read, it's almost certainly not being used. The one exception + # (for which we'll add a `nolint` comment) is with fields that exist solely for serialization + # (to YAML, BSON, etc.). + field-writes-are-uses: false + # We want to actually check that exported fields are used. + exported-fields-are-used: false + # Setting this to false caught some places where we created and wrote to local variables but + # never read from them. + local-variables-are-used: false + + usetesting: + # We _do_ want to use the testing package's versions of these methods. They ensure that we clean + # up after ourselves automatically. + os-setenv: true + os-temp-dir: true + +issues: + max-same-issues: 0 + + # This allows you to write bson.D literals without spelling out each `bson.E` it contains: + # + # doc := bson.D{{"_id", 42}, {"color", "green"}} + # + # As opposed to: + # + # doc := bson.D{bson.E{"_id", 42}, bson.E{"color", "green"}} + # + # This allows allows the same shorter version for the `bson.Binary` and `bson.Timestamp` types. + # + # The "(bson|primitive)" bit accounts for the package name changes between v1 and v2 of the Golang + # driver. You should probably change this to the appropriate package for whichever driver version + # your project uses, if you just use one version. + exclude: + - "composites: .+((bson|primitive).(Binary|E|Timestamp)) struct literal uses unkeyed fields" + + exclude-rules: + # We're okay with type assertions panicking in test code and dev tools. + - path: _test\.go|_test_suite\.go|cmd/ + linters: + - errcheck + # This is the output we see from errcheck for failures to check type assertion results. We are + # okay with not checking these in tests, since if the type assertion fails we'll just get a + # panic. Failing to check an error from a _function_ gives us output like "Error return value + # of `os.Mkdir` is not checked". + text: "Error return value is not checked" diff --git a/CHANGELOG-MongoDB.md b/CHANGELOG-MongoDB.md index 0777d13..2780283 100644 --- a/CHANGELOG-MongoDB.md +++ b/CHANGELOG-MongoDB.md @@ -7,3 +7,5 @@ - Added a recommendation to use `gopls` in general, and specifically to configure your editor to use it to update imports. - Added recommendations to use `golangci-lint` or `nogo` for linting orchestration. +- Added recommendations for many more linters on top of the recommended minimal linting config. + diff --git a/src/lint.md b/src/lint.md index 6e5ec7e..4fda554 100644 --- a/src/lint.md +++ b/src/lint.md @@ -19,6 +19,129 @@ quality without being unnecessarily prescriptive: [revive]: https://github.com/mgechev/revive [staticcheck]: https://staticcheck.dev +## Additional Recommended Linters + +We recommend consider some or all the following linters in addition to those listed above. Note that +many of these linters have a variety of configuration options. We encourage you to read each +linter's documentation and consider how you'd like to configure them when you adopt them. + +### Code Complexity and Quality + +- [cyclop] to check code's cyclomatic complexity, which is a heuristic evaluation of "how hard is + this code to understand" based on things like how many statements a function has, how many + conditional it uses, etc. +- [funlen] to check a function's length in lines and statements. +- [gocognit] to check code's cognitive complexity. This is similar to cognitive complexity but uses + a different heuristic. +- [gocritic] is a style checker along the lines of `govet` and `revive`. Notably, this linter allows + you to use [ruleguard]. See the [Ruleguard] section for more details. +- [maintidx] measures the maintainability index of functions. Again, this is imilar to the + cyclomatic and cognitive complexity heuristics. +- [nestif] detects `if` nesting past a given level. + + [cyclop]: https://github.com/bkielbasa/cyclop + [funlen]: https://github.com/ultraware/funlen + [gocognit]: https://github.com/uudashr/gocognit + [gocritic]: https://go-critic.com/ + [maintidx]: https://github.com/uudashr/gocognit + [nestif]: https://github.com/nakabonne/nestif + [ruleguard]: https://github.com/quasilyte/go-ruleguard + +### Error Types, Messages, and Error Handling + +- [errname] checks that error types and names follow Golang standards. +- [errorlint] checks for code that embeds errors as strings instead of wrapping them, as well as + code that uses type assertions instead of `errors.As` or `errors.Is`. +- [nilnesserr] checks for incorrect error handling corner cases, for example checking if `err2 != + nil` and then returning `err` instead of `err2`. + + [errname]: https://github.com/Antonboom/errname + [errorlint]: https://github.com/polyfloyd/go-errorlint + [nilnesserr]: https://github.com/alingse/nilnesserr + +### Imports + +- [depguard] allows you to forbid imports of specific packages, either entirely, or only in certain + files. +- [gci] enforces import order and general formatting of the `imports` section. This goes a bit + beyond what `gopls` does in terms of formatting imports. +- [importas] enforces the use of the same alias everywhere a given package is aliased. In other + words, it requires to always alias `github.com/google/uuid` as something like `guuid`, rather than + using `guuid` in one package and `u` in another. + + [depguard]: https://github.com/OpenPeeDeeP/depguard + [gci]: https://github.com/daixiang0/gci + [importas]: https://github.com/julz/importas + +## Spelling and Grammar + +- [dupword] checks for duplicate words in comments or strings like "failed because the the server + returned an error". +- [godot] requires all comments to end in a period. +- [misspell] checks for commonly misspelled English words. + + [dupword]: https://github.com/Abirdcfly/dupword + [godot]: https://github.com/tetafro/godot + [misspell]: https://github.com/client9/misspell + +### Code Quality and Cleanup + +- [copyloopvar] finds places where loop variables are copied, like `i := i`. This is no longer + necessary as of Go 1.22. +- [exhaustruct] requires that all fields of a struct be initialized when a struct is created. +- [exptostd] checks for use of function in `golang.org/x/exp` packages that can be replaced by + functions from the standard library. +- [gocheckcompilerdirectives] checks that Go compiler directive comments, like `//go:build ...`, are + valid. +- [iface] detects unused interfaces, multiple identical interfaces, and functions which return + interfaces but always return the same concrete implementation type. +- [ineffassign] detects when a variable is assigned a value but that value is never used, for + example if you reassign to a variable but never use the first value. +- [intrange] finds loops where you can use the `for i := 10` integer range feature added in Go 1.22. +- [nolintlint] finds `//nolint` comments for `golangci-lint` that are ill-formed, don't have an + exaplanatory comment, or are applied to code that doesn't trigger the disabled linter(s). +- [predeclared] finds code that shadow predeclared Go identifiers like a function named `copy`. +- [recvcheck] checks that all receivers for a type are pointers or non-pointers. + - Note that if you use this you will almost certainly want to exempt `MarshalBSON` and + `UnmarshalBSON` methods from this linter. +- [testifylint] checks for various issues when using the [testify] test packages. +- [unconvert] finds unnecessary type conversions. +- [unparam] finds unused function parameters. +- [unused] finds unused constants, variables, functions, and types. +- [usestdlibvars] finds places where you can use stdlib vars like `http.StatusOK` instead of `200`. +- [usetesting] finds places where you can using `testing` package helpers like `t.TempDir` instead + of `os.TempDir`. + + [copyloopvar]: https://github.com/karamaru-alpha/copyloopvar + [exhaustruct]: https://github.com/GaijinEntertainment/go-exhaustruct + [exptostd]: https://github.com/ldez/exptostd + [gocheckcompilerdirectives]: https://github.com/leighmcculloch/gocheckcompilerdirectives + [iface]: https://github.com/uudashr/iface + [ineffassign]: https://github.com/gordonklaus/ineffassign + [intrange]: https://github.com/ckaznocha/intrange + [nolintlint]: https://github.com/golangci/golangci-lint/tree/master/pkg/golinters/nolintlint/internal + [predeclared]: https://github.com/nishanths/predeclared + [recvcheck]: https://github.com/raeperd/recvcheck + [testifylint]: https://github.com/Antonboom/testifylint + [testify]: https://github.com/stretchr/testify + [unconvert]: https://github.com/mdempsky/unconvert + [unparam]: https://github.com/mvdan/unparam + [unused]: https://github.com/dominikh/go-tools/tree/master/unused + [usestdlibvars]: https://github.com/sashamelentyev/usestdlibvars + [usetesting]: https://github.com/ldez/usetesting + +### Ruleguard + +The [gocritic] linter allows you to use [ruleguard]. Ruleguard is a tool that lets you write +declarative rules that your codebase is checked against. This is a very flexible tool that allows +you to write checks for things that cannot be done with any existing linter. + +For example, you could write rules to: + +- forbid the use of specific generic types. +- forbid specific functions from certain packages. +- require that all context cancellations, deadlines, and timeouts have a cause, among many . + ## Lint Runners For projects which use Bazel, you should use [nogo] instead of [golangci-lint], as [golangci-lint] @@ -27,7 +150,9 @@ does not work well with Bazel's sandboxing. Otherwise, we recommend [golangci-lint] as the go-to lint runner for Go code, largely due to its performance in larger codebases and ability to configure and use many canonical linters at once. This repo has an example [.golangci.yml] config file -with recommended linters and settings. +with recommended minimal linters and settings. + +There is also a [.golangci-maximal.yml] file with all the additional linters enabled. golangci-lint has [various linters] available for use. The above linters are recommended as a base set, and we encourage teams to add any additional linters