Golang x/tools/gopls: provide a way to suppress analysis diagnostics for certain files

gopls version

❯ gopls -v version
Build info
----------
golang.org/x/tools/gopls master
    golang.org/x/tools/gopls@(devel)
    github.com/BurntSushi/toml@v0.4.1 h1:GaI7EiDXDRfa8VshkTj7Fym7ha+y8/XxIgD2okUIjLw=
    github.com/google/go-cmp@v0.5.6 h1:BKbKCqvP6I+rmFHt06ZmyQtvB8xAkWdhFyr0ZUNZcxQ=
    github.com/sergi/go-diff@v1.1.0 h1:we8PVUC3FE2uYfodKH/nBHMSetSfHDR6scGdBi+erh0=
    golang.org/x/mod@v0.5.1 h1:OJxoQ/rynoF0dcCdI7cLPktw/hR2cueqYfjm43oqK38=
    golang.org/x/sync@v0.0.0-20210220032951-036812b2e83c h1:5KslGYwFpkhGh+Q16bwMP3cOontH8FOep7tGV86Y7SQ=
    golang.org/x/sys@v0.0.0-20211019181941-9d821ace8654 h1:id054HUawV2/6IGm2IV8KZQjqtwAOo2CYlOToYqa0d0=
    golang.org/x/text@v0.3.7 h1:olpwvP2KacW1ZWvsR7uQhoyTYvKAupfQrRGBFM352Gk=
    golang.org/x/tools@v0.1.7 => ../
    golang.org/x/xerrors@v0.0.0-20200804184101-5ec99f83aff1 h1:go1bK/D/BFZV2I8cIQd1NKEZ+0owSTG1fDTci4IqFcE=
    honnef.co/go/tools@v0.2.1 h1:/EPr//+UMMXwMTkXvCCoaJDq8cpjMO80Ou+L4PDo2mY=
    mvdan.cc/gofumpt@v0.1.1 h1:bi/1aS/5W00E2ny5q65w9SnKpWEF/UIOqDYBILpo9rA=
    mvdan.cc/xurls/v2@v2.3.0 h1:59Olnbt67UKpxF1EwVBopJvkSUBmgtb468E4GVWIZ1I=

Is there a way to stop the linter for just one function or file?

In some of our tests we're bad bad things intentionally and it just clutters the problems pane.

Comment From: findleyr

No, there isn't a way to do this, currently.

Based on the issue subject, are you asking for something akin to the golangci-lint //nolint directive?

Comment From: OneOfOne

@findleyr correct, i'd even be happy if it worked for a single line instead of a while function, the spam is real and I'd rather not turn off the specific linter for the whole project.

Comment From: findleyr

@OneOfOne would you want this just for lint/analysis errors, or would you want it for all diagnostics (including "compiler" errors). I am sympathetic to wanting to suppress lint errors, but I don't think we should ever suppress errors from the compiler (the parser/type checker). If there is a compiler error many other gopls features (autocompletion, jump to definition, etc.) may not function correctly, and it would be confusing if this happens silently.

Comment From: OneOfOne

Only lint/analysis of course, compiler errors are important.

Comment From: robpike

My two cents:

Compiler errors are important, but linter errors are not. Do not decorate your code with annotations to silence a defective tool. Instead, use better tools and depend on ones that are reliable. It is just bad practice to require your build to pass through a capricious tool like a linter.

If you insist on using a linter, find a way for your local build process to disable it when required. Don't require externally-supported tools to honor your process, and don't require source code add marks to disable one too. That slippery slope leads to a valley of toxic muck.

Comment From: timothy-king

and it just clutters the problems pane.

Can you describe what the problem is in more detail? What I have managed to grasp so far is that gopls is reporting accurately on intentionally buggy code. This does not sound like a problem yet so I think I am missing something. Maybe a brief description of how the workspace is setup? Also what is the test?

Comment From: OneOfOne

@timothy-king @robpike this is about gopls' linter/analysis not the compiler, to be more specific, we have some tests that that trigger string literal contains Unicode format characters, consider using escape sequences instead ST1018 for example, we know it's buggy, but it's intentional to test our codebase.

Comment From: robpike

I understand the problem you've set yourself, I'm just asking that we don't end up with annotated source code as a solution. Instead, for your particular situation, construct a way to avoid failing on the linter error for that broken code. In other words, it's your problem to solve, not the Go team's, in my obviously biased opinion. Gopls should not make bad code easier to work on. That just contradicts its purpose.

Comment From: OneOfOne

I'd argue that you can already do that by turning off the guilty linter rule with gopls options, but instead of turning it off for a specific function or line like I requested, it'd apply to the whole codebase which can lead to actual problems.

On Mon, Jan 24, 2022, 2:49 AM Rob Pike @.***> wrote:

I understand the problem you've set yourself, I'm just asking that we don't end up with annotated source code as a solution. Instead, for your particular situation, construct a way to avoid failing on the linter error for that broken code. In other words, it's your problem to solve, not the Go team's, in my obviously biased opinion. Gopls should not make bad code easier to work on. That just contradicts its purpose.

— Reply to this email directly, view it on GitHub https://github.com/golang/go/issues/50764#issuecomment-1019853919, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAIHY6Z6LOMJGPAI6JMDYKTUXUHATANCNFSM5MSZTM2A . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you were mentioned.Message ID: @.***>

Comment From: findleyr

@robpike I fully agree about avoiding annotated source code, and independent of the solution I don't think we should couple ourselves to golangci-lint -- that is bound to be problematic.

However, I do think at a high level it is reasonable to request that gopls provide a mechanism to disable analyses for certain files or directories. Per your point:

If you insist on using a linter, find a way for your local build process to disable it when required.

Many users primarily consume static analysis through their editor, where the analog of this advice is to find a way to disable the linter where required in the development process. Right now gopls only provides an on/off switch, and I can understand how in certain codebases, perhaps in the process of incremental cleanup, it could be desirable to disable/enable linters for portions of the codebase.

With all that said, gopls has long avoided any sort of per-directory configuration, and I'd like to keep avoiding it as long as possible, until the benefit is unambiguous.

With respect to this request, it seems possible that no additional gopls configuration is required. The default set of linters gopls uses are the cmd/vet analyzers + a few additional analyzers that we deem to have a very low rate of false positives. However, we allow our users to enable more opinionated linters, included those from staticcheck. ST1018, is a staticcheck style check.

Staticcheck has its own mechanism for per-directory configuration, which we have a long-standing request to support (#36373). I'd rather we do that than build an analogous mechanism for gopls.

Comment From: hyangah

@OneOfOne I empathize with your situation. I still want to understand whether existing exception rules gopls already supports can be used to address your use case - i.e., disabling analysis on certain files.

The go command has established its own exception patterns - directory and file names that begin with "." or "_" are ignored, and files in testdata directories are excluded. Gopls's analysis honors them (if not, that's a bug :-)). If you believe ST1018 is still a useful check and you need the exception only for files used for testing, couldn't the problematic code be in testdata? That will give you added benefits such as making it clear that the problematic code are for testing.

Comment From: OneOfOne

@hyangah I guess it's possible to load the tests cases from a json file or something, however I stand by my original request since gopls took over the job of all the linters, and it's fairly standard in most linters to support a way to turn it off per file or function.

Comment From: timothy-king

2cents: For testing with string constants that violate ST1018, I recommend putting this data into a file in testdata/ and reading it from the file within the test, or using a //go:embed directive. (Turning off this checker in particular would disable checking on Unicode control characters and formatting characters in .go files. Things like the Trojan Source paper would make me wary of turning this off for any code I maintained. I am definitely out of my depth on Unicode here so I would personally be cautious. But YMMV.)

Comment From: gopherbot

Change https://go.dev/cl/489835 mentions this issue: go/analysis/passes/unusedresult: support annotations

Comment From: adonovan

I think it is possible to distinguish two different approaches to annotations.

In the first approach, an annotation declares "suppress diagnostics of class X in this function/file". Such annotations are used to work around linters that inherently produce spurious findings, and I agree that they seem likely to encourage littering (@robpike's slippery slope to the Valley of the Drums).

In the second approach, annotations provide specific additional information to the linter that improve its precision, so that it can emit more true positives and fewer false negatives. For example, the printf checker can tell that a function delegates to fmt.Printf and treat it as "printf-like" for analytic purposes. But a func or interface type such as these:

type logf func(format string, args ...any) // printf-like

type Logger interface {
    Logf(format string, args ...any) // printf-like
}

has no single body from which the analysis can make this deduction. An annotation--such as the completely hypothetical comment notation above--could assert "these abstract functions behave like printf". This records helpful design intent for the reader, and a small number of annotations enables the tool to check essentially 100% of printf-like calls accurately. This seems like progress.

It might not always be clear whether an annotation is a "suppression" or a "lemma". Lemmas are intimately tied to particular analyses; they should record specific non-obvious design information that is helpful to the reader; and they should have high leverage (a few annotations go along way); but suppressions are just a generic way to hush the tool, like swatting flies as you see them instead of preventing them from breeding.

Comment From: silverwind

A //nolint:gopls directive would be great to have, should be parsed as follows:

//nolint - ignore line
//nolint:gopls - ignore line
//nolint:gopls,staticcheck - ignore line
//nolint:staticcheck - raise

Comment From: qdm12

How about adding a setting to the gopls configuration to have exclude rules? A bit like golangci-lint's .golangci.yml exclude rules. That way the Go code is not polluted, and these exclude rules are specified elsewhere (i.e. in vscode's settings.json).

For my case, I have that line from a 'deprecated' function which I really need and there is no replacement, and gopls has been complaining about it for years now, and I occasionally forget what the warning is and go check out that file every few weeks... A little single exclude rule for gopls would solve that time wasted every few weeks.

Comment From: Johnlon

Is there a way to suppress my custom linter on a line by line/function etc basis like other linters? I can't deploy my linter without it.

Seems like a cirital gap - any update.

I wan't //nolint:mylintername

Comment From: adonovan

Is there a way to suppress my custom linter on a line by line/function etc basis like other linters? I can't deploy my linter without it.

Seems like a cirital gap - any update.

I wan't //nolint:mylintername

See my comment above for why I don't think we should take that approach.

Exclude rules in a personal configuration file are more palatable, but it seems to me that the need to exclude this particular deprecation check is a sign that deprecations shouldn't be reported as diagnostics in the first place.

Comment From: silverwind

How about adding a setting to the gopls configuration to have exclude rules? A bit like golangci-lint's .golangci.yml exclude rules. That way the Go code is not polluted, and these exclude rules are specified elsewhere (i.e. in vscode's settings.json).

Imho, inline comment are clearly superiour than config because config often is abused and set too broadly, for example regex _test.go$ when the intention is to just disable a single rule in a single test file.

Another benefit of inline comments is that linters can report on nolint comments that are not actually triggering a rule, which is also very important when those rules change.

Comment From: Johnlon

it's obvious we need a built-in inline suppression mech just like other tools in go and other languages ecosystems.

not sure why here is any debate.

currently my own rule has code for end of line comment suppressions but I haven't coded for comments on line above or on function or file level.

Comment From: meling

I’ve noticed that the VSCode Problems pane lists lint diagnostics as Errors. Is there a reason why lint Warnings are showing up as Errors in VSCode? That seems like an unnecessary source of friction.

I have my gopls configured with:

    "staticcheck": true

Moreover, I’m using a //lint:ignore SAxxxx reason directive meant for staticcheck. However, it seems like gopls is getting involved here—am I missing something?

According to the gopls analyzers documentation, we should consult staticcheck’s documentation for analyzer details. Which I did, and the annotation I used is valid for staticcheck, confirmed by running staticcheck on the command line.

I’m using this annotation in just one specific corner case—I’m definitely not advocating for widespread use of such annotations.

My concern is that gopls should respect staticcheck’s handling of these annotations to prevent confusion. If someone configures gopls to use staticcheck, they shouldn’t find their valid staticcheck annotations ignored.

Comment From: podocarp

I am encountering the same problem and was surprised that gopls doesn't obey the annotations of staticcheck. I'm not sure how these two interplay, but it seems to be a little weird this is the case. Shouldn't these annotations come for free if you just pass the file to staticheck and return its results? Or is gopls only passing in the parsed AST to reduce overhead? In that case it would also be possible to simply not strip the comments away.

Comment From: findleyr

@meling @podocarp it looks like staticcheck's suppression mechanism is implemented by its analysis driver, not individual analyzers. Since gopls has its own driver, these annotations are not honored. It's not related to stripping comments; staticcheck analyzers see everything.

It would not be a good idea to reimplement this logic in gopls; perhaps staticcheck push this logic into the analyzer, or offer a library call to wrap an analyzer in one that honors staticcheck suppression comments.

CC @dominikh for opinions.

Comment From: dominikh

I'll look into it. I can't yet say which of the two suggested options I'd go for, but I'm leaning towards the wrapper.

Comment From: silverwind

staticcheck is just one linter in the go ecosystem, golangci-lint is another. They both use different annotations to ignore issues with inline comments:

https://staticcheck.dev/docs/configuration/#maintenance-of-linter-directives https://golangci-lint.run/usage/false-positives/#nolint-directive

For a seamless integration against both linters, i recommend supporting both formats:

//lint:ignore gopls
//nolint:gopls

Comment From: findleyr

@silverwind as discussed above, we're not planning to add general suppression for diagnostics from gopls.

However, IMO honoring staticcheck directives is necessary for our staticcheck integration, and it is a bug that they are not respected.

Comment From: silverwind

Still sounds like unfair favorititsm if you are supporting one linter, but not the other.

Comment From: findleyr

Still sounds like unfair favorititsm if you are supporting one linter, but not the other.

@silverwind sorry I don't understand this comment. Gopls does not integrate golangci-lint. If you are seeing diagnostics from golangci-lint, they are coming from an external integration, and that integration should honor golangci-lint suppressions.

Comment From: findleyr

Perhaps you are referring to the fact that gopls integrates with staticcheck, and not golangci-lint? In that case: we cannot link arbitrarily many linters into gopls. I think the path forward is to allow others to make gopls' linting extensible, so that it can run external (or perhaps even workspace-specific) linters. This is something we're considering (#59869).

But that is not directly relevant to this issue. My perspective is that: - Any warnings from gopls analyzers should be directly actionable, and no suppression mechanism should be needed. Otherwise they do not meet our standards for false positives. - Our staticcheck integration should honor existing staticcheck suppression mechanisms. It is a bug that it does not.

Comment From: silverwind

Maybe to explain my use case further. I'm running gopls check <files> as a CLI linting tool and I was looking for a way to suppress errors there as well as in any editor integrations. Currently I run gopls check <files> | grep -v PATTERN to suppress certain errors.

I'm aware that linters like staticcheck and golangci-lint probably have their own LSPs, independant of gopls. I don't know if/how gopls integrates against staticcheck.

Comment From: findleyr

@silverwind which errors do you suppress? We have been more permissive with 'hint' level diagnostics, since most clients make them much less prominent. However, we probably neglected to filter them out of the command line.

Comment From: silverwind

The script that does it is here. Currently I'm suppressing deprecation warnings.

For various reasons, these deprecations can not be fixed without considerable effort, which were not feasible to do at the time I introduced the gopls check into that codebase.

Comment From: gopherbot

Change https://go.dev/cl/651616 mentions this issue: gopls: add a -level flag for gopls check

Comment From: dominikh

But that is not directly relevant to this issue. My perspective is that:

* Any warnings from gopls analyzers should be directly actionable, and no suppression mechanism should be needed. Otherwise they do not meet our standards for false positives.

* Our staticcheck integration should honor existing staticcheck suppression mechanisms. It is a bug that it does not.

We could also discuss reducing the set of Staticcheck analyses that run in gopls (or in Staticcheck, for that matter). Staticcheck has rules very similar to vet/gopls for how many false positives are tolerated, which really is "none, unless your code is extremely special.". Its ignore directives really only exist to tolerate extraordinary code without having to twist the code to conform to the analyses' rules. I'm frankly shocked by how important this issue seems to be to people and makes me worry that the rate of false positives is too high.

Comment From: findleyr

Thanks @dominikh.

I wonder if all of the problematic diagnostics are actually info or hint level severity. We have recently taken the stance that it is OK to produce hint level diagnostics, since they are almost invisible in many editors (in VS Code they do not appear in the problems tab, and the underline is very subtle). The "deprecated" diagnostics @silverwind was referring to were hints, and we weren't suppressing them in the gopls check subcommand (but are now).

Some concrete examples of staticcheck diagnostics requiring suppression would be helpful here.

Comment From: silverwind

The "deprecated" diagnostics @silverwind was referring to were hints, and we weren't suppressing them in the gopls check subcommand (but are now).

Generally I prefer to treat all warnings/hints as errors when running code checking tools, but I guess I do appreciate the change. I'd appreciate it even more if gopls check would have an option to include/exclude hints.

Comment From: findleyr

I'd appreciate it even more if gopls check would have an option to include/exclude hints.

I added this. gopls -check -severity=hint has this effect.

Comment From: adonovan

Related: - https://github.com/golang/go/issues/72008

Comment From: frankli0324

I personally find it confusing to have a subset of functionalities of a linting tool wrapped inside another tool (language server). I would vote for having options to choose from various linting tools directly if I had a chance.

gopls could provide an independent set of linting rules and, maybe even better integration with other gopls functions, but I think it's easier to understand if user can choose whether they want gopls linter(runTool(gopls, [check])) or staticcheck (runTool(staticcheck)) at extension level, not at language server level.

right now the extension runs staticcheck, and gopls lints the code again with staticcheck rules, which made me confused when I see "staticcheck" in go extension output emmitting no errors, and still getting linting warning underlines which was supposed to be generated by staticcheck(this confuses me the most), while gopls outputs no logs in vscode indicating that it did any linting at all.

Comment From: adonovan

I personally find it confusing to have a subset of functionalities of a linting tool wrapped inside another tool (language server). I would vote for having options to choose from various linting tools directly if I had a chance.

The advantage of running analyzers within gopls is tighter integration: analyses are efficiently and incrementally run in real-time after each edit, they observe the exact state of each editor buffer even with unsaved edits, and they are all driven by a single configuration. Our long term goal is that there is no need for vscode-go to execute staticcheck directly, though there are a number of features provided by the staticcheck driver that gopls does not provide (such as the subject of this issue).

Comment From: frankli0324

yeah I mentioned:

maybe even better integration with other gopls functions

I agree it would be more efficient, but the switch from other linters to gopls linter should be "atomic", right now there's a "data race"(conflict)

using staticcheck/golangci-lint/gopls linter should be an "either or", or at least user gets to choose the set of linters they want to use

Comment From: adonovan

@h9jiang Can we provide better guidance in the VS Code Go extension so that users aren't confused by the redundant duplication between lintOnSave=staticcheck and gopls' invocation of gopls's analyzers? (See also https://github.com/golang/go/issues/74273#issuecomment-2992671935.)