Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
OneOfOne opened this issue Jan 23, 2022 · 15 comments
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) FeatureRequest gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.

Comments

@OneOfOne
Copy link
Contributor

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.

@gopherbot gopherbot added Tools This label describes issues relating to any tools in the x/tools repository. gopls Issues related to the Go language server, gopls. labels Jan 23, 2022
@gopherbot gopherbot added this to the Unreleased milestone Jan 23, 2022
@findleyr
Copy link
Contributor

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?

@OneOfOne
Copy link
Contributor Author

@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.

@findleyr
Copy link
Contributor

findleyr commented Jan 23, 2022

@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.

@OneOfOne
Copy link
Contributor Author

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

@robpike
Copy link
Contributor

robpike commented Jan 24, 2022

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.

@findleyr findleyr changed the title x/tools/gopls: respect nolint x/tools/gopls: provide a way to suppress analysis diagnostics for certain files Jan 24, 2022
@timothy-king
Copy link
Contributor

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?

@OneOfOne
Copy link
Contributor Author

@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.

@robpike
Copy link
Contributor

robpike commented Jan 24, 2022

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.

@OneOfOne
Copy link
Contributor Author

OneOfOne commented Jan 24, 2022 via email

@findleyr
Copy link
Contributor

@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.

@hyangah
Copy link
Contributor

hyangah commented Jan 24, 2022

@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.

@OneOfOne
Copy link
Contributor Author

@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.

@timothy-king
Copy link
Contributor

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.)

@findleyr findleyr modified the milestones: Unreleased, gopls/unplanned Jan 24, 2022
@adonovan adonovan added the Analysis Issues related to static analysis (vet, x/tools/go/analysis) label Apr 22, 2023
@gopherbot
Copy link

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

@adonovan
Copy link
Member

adonovan commented Oct 24, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) FeatureRequest gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

No branches or pull requests

7 participants