From 12acdaf91bc02b5dac019389171eb761eecfc375 Mon Sep 17 00:00:00 2001 From: Steven Littiebrant Date: Sat, 30 Jan 2021 03:18:12 +0000 Subject: [PATCH] Document disabled lints, especially `unhandled-error` With this, `revive.toml` mentions all currently-available lints. Some seem desirable, but they have more lint failures than I feel like tackling at the moment :) So they're just documented for now. --- `unhandled-error` I'd *love* to enable immediately, but ~300 warnings is far too many. Newly-introduced violations of other lints would just be lost in the noise. Reducing (or rarely whitelisting) these seems necessary before enabling, which IMO should be considered fairly high priority. Missing error handling is pretty frequently a source of hard-to-identify misbehavior, and a lint like this is the only feasible way to catch many cases. It's just far too hard to reliably catch during code review (as evidence: 300 cases!), especially when the returned value(s) are not assigned to anything. --- revive.toml | 41 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/revive.toml b/revive.toml index ee92c3882cb..fdca1a5cea3 100644 --- a/revive.toml +++ b/revive.toml @@ -29,6 +29,10 @@ warningCode = 0 #### higher value stuff +# this is basically errcheck, warns on errs that are not checked. +# strongly desired, but disabled due to 300 failures (to be tackled incrementally). +# [rule.unhandled-error] + # general defer gotchas. # # in particular: "recover" warns about unsafe use of recover(). @@ -55,3 +59,40 @@ arguments=[["loop","method-call","recover","return"]] [rule.unconditional-recursion] # probably a good idea [rule.unreachable-code] # code simplifier [rule.waitgroup-by-value] # correct use of sync code, important + +#### unused utilities + +# [rule.file-header] # could possibly replace `copyright -verifyOnly`? +# [rule.imports-blacklist] # simple way to ban imports / enforce wrappers, likely useful + +#### disabled but maybe desirable + +# [rule.bare-return] # probably beneficial as it's slightly error-prone, but 2,000 failures +# [rule.bool-literal-in-expr] # minor code simplifier, few failures +# [rule.confusing-results] # maybe beneficial, only a few failures +# [rule.deep-exit] # probably a good idea in most code, some failures, but not trivial to adopt +# [rule.duplicated-imports] # minor, but may be worthwhile. failures are weird but harmless +# [rule.early-return] # minor code simplifier, a handful of failures +# [rule.get-return] # existing failures are intentional + desirable, but in principle it's a fine idea +# [rule.import-shadowing] # probably beneficial, but 750 failures +# [rule.redefines-builtin-id] # probably beneficial, few failures +# [rule.struct-tag] # probably beneficial, a few failures +# [rule.superfluous-else] # minor code simplifier, a few failures +# [rule.unexported-naming] # probably beneficial, but 300 failures +# [rule.unused-parameter] # minor code simplifier / clarifier, but 250 failures +# [rule.unused-receiver] # minor code simplifier / clarifier, but 500 failures + +#### probably undesirable + +# [rule.add-constant] # extremely noisy. 18,000 failures, overwhelmingly for tests or 0/1 which seem totally fine +# [rule.argument-limit] # too arbitrary +# [rule.cognitive-complexity] # dubious value, but possibly interesting +# [rule.confusing-naming] # dubious value, ~50 failures +# [rule.cyclomatic] # dubious value, but possibly interesting +# [rule.empty-block] # easily noticed in code review, but also warns on documented no-op branches, which seem fine +# [rule.empty-lines] # low value, many failures +# [rule.flag-parameter] # interesting, but very noisy +# [rule.function-result-limit] # too arbitrary, easily noticed in code review +# [rule.line-length-limit] # too arbitrary +# [rule.max-public-structs] # too arbitrary +# [rule.unnecessary-stmt] # dubious value