Go-tools: -ignore should be un-deprecated

[jadekler]

I'm a maintainer for https://github.com/google/google-api-go-client and https://github.com/GoogleCloudPlatform/google-cloud-go/, and we use staticcheck with -ignore for several things.

The recent large change has declared -ignore deprecated in favour of in-file linter directives (let me know if my assessment is incorrect, please). I urge you to reconsider the deprecation part of that plan:

• Library owners are less willing to put extra comments into our files, since these linter comments make the code less readable. Apart from the maintainability / readability concerns within our team, users depend on being able to easily grok our source code.
• In our repos, we often have to ignore wide ranges of files (e.g. gen). It's not reasonable to go adding a line to thousands of methods, or hundreds of files.

Btw, I have one more qualm. You start the "Ignoring Problems" section with,

In general, you shouldn't have to ignore problems reported by staticcheck.

Actually, I would argue that in general you should go into any static analysis / linting tool with the expectation that you _should_ have to ignore problems. There are numerous categories files that don't make sense to use linters in - examples, tests, and code gen being obvious ones. Furthermore, when maintaining a library, you will constantly be ignoring rules telling you to use more-up-to-date / non-deprecated code (unfortunately, we must support old version of Go / dependencies and can't do that), or to change some part of our API to be better (unfortunately, our API surface is stable and immutable).

These are two easy reasons, but there are many others we've encountered. Again, in general I think people should go into using this tool with the expectation that it won't be a perfect fit, and I urge you to set that expectation.

[dominikh]

Your assessment is correct, but I disagree with your arguments.

Library owners are less willing to put extra comments into our files, since these linter comments make the code less readable. Apart from the maintainability / readability concerns within our team, users depend on being able to easily grok our source code.

I would argue that -ignore is harder to maintain. Ignores are detached from the relevant code, will not be kept up to date, and have a tendency to ignore more problems than is intended. A file can have both a false positive and a true positive for the same check.

I've tried to design these comments to be the least intrusive possible. In particular, they require a human-readable reason. If your code is weird enough to trigger a false positive (that can't trivially be fixed in staticcheck), then it's probably weird enough to be commented for human readers, too.

In our repos, we often have to ignore wide ranges of files (e.g. gen). It's not reasonable to go adding a line to thousands of methods, or hundreds of files.

I would think that this only required editing very few files; those defining the code generator. Code generators are encouraged to emit the appropriate linter directives, in particular file-wide ones.

Alternately, if said code exists in isolated packages, then configuration files can be used instead.

Actually, I would argue that in general you should go into any static analysis / linting tool with the expectation that you should have to ignore problems. There are numerous categories files that don't make sense to use linters in - examples, tests, and code gen being obvious ones

As far as the S and SA categories of checks are concerned, I am trying very hard to maintain approximately zero false positives. Plenty of our checks are aware of the code's context (examples, tests, …) and act appropriately. Whenever this is not the case, it is considered a bug to be fixed (c.f. c98cc0732156069250a6a9708543a187726b2c31 for example)

The new ST category is vastly more opinionated, but it is also expected of users to enable precisely those checks that fit the codebase as a whole. And even here, we are open to context-sensitive exceptions. Users are also free to not fail code reviews or CI on ST checks (or any checks, for that matter), much like golint is recommended to be used.

Furthermore, when maintaining a library, you will constantly be ignoring rules telling you to use more-up-to-date / non-deprecated code (unfortunately, we must support old version of Go / dependencies and can't do that)

Staticcheck has the -go flag for targeting a minimum Go version. This disables both deprecation warnings as well as simplifications that don't apply to said Go version.

or to change some part of our API to be better (unfortunately, our API surface is stable and immutable).

I can only think of some style checks that make any suggestions regarding public APIs. In general, our checks don't have many opinions on APIs.

Again, in general I think people should go into using this tool with the expectation that it won't be a perfect fit, and I urge you to set that expectation.

On the contrary, it is very much my goal to _not_ write yet another noisy linter where 80% of reports are useless. The need to ignore reports should be minimal (~0 for S* and SA*, ~0 for ST if configured correctly). And if you do encounter any false positives, or noisy positives, I encourage you to report these. They're usually treated as bugs.

I am interested in making the experience of using staticcheck as pain-free as possible, and welcome your input. My focus, however, is on making it pain-free by default, and to not compromise maintainability by having data related to code be detached from said code. This is the main motivation behind deprecating the -ignore flag. Instead of simply undeprecating it, I would much rather work towards finding better alternatives.

[ainar-g]

Let me respectfully provide some counter-arguments.

Library owners are less willing to put extra comments into our files, since these linter comments make the code less readable.

If you look at the way linter directives are described in the docs, they always provide context, like:

//lint:ignore SA4000 we want to make sure that no two results of errors.New are ever the same

I do not find that such directives hurt readability, and in fact think that they might assist the reader in understanding the code.

In our repos, we often have to ignore wide ranges of files (e.g. gen). It's not reasonable to go adding a line to thousands of methods, or hundreds of files.

If you use the standard generated file header format, it should already exclude generated files, IIRC. And if it doesn't you can use file-ignore directives.

I also disagree that cases where you have to ignore linters are that numerous. A friend has told me that he made his >50K SLOC Go codebase pass staticcheck and most other linters cleanly in just a couple of hours, fixing a few minor bugs in the process. Examples and tests should be correct, and I've already covered generated code above. I do agree that deprecation warnings should sometimes be ignored, but only temporarily.

[dominikh]

Unfortunately there hasn't been any progress on this issue in several months, and my opinion hasn't changed, either. I am going to close this issue. If anyone does come up with new arguments I'll happily reopen the issue. If people have concrete false or noisy positives that need to be fixed I encourage you to file separate issues.