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: respect staticcheck config file #36373

Open
llimllib opened this issue Jan 3, 2020 · 16 comments
Open

x/tools/gopls: respect staticcheck config file #36373

llimllib opened this issue Jan 3, 2020 · 16 comments
Labels
FeatureRequest gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@llimllib
Copy link

llimllib commented Jan 3, 2020

What version of Go are you using (go version)?

$ go version
go version go1.13 darwin/amd64

Does this issue reproduce with the latest release?

govim is using commit f13409bb, which is pretty new, and it reproduces

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN="/Users/llimllib/go/bin"
GOCACHE="/Users/llimllib/Library/Caches/go-build"
GOENV="/Users/llimllib/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/llimllib/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/Cellar/go/1.13/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.13/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/llimllib/code/tools-golang/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/r4/mc760j7j6xjdgr5p5hxk_xrw0000gq/T/go-build877572449=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

I don't know how to use gopls directly, so here's how to reproduce through govim

  1. Create a directory
  2. Create a staticcheck.conf file with these contents:
checks = ["all", "-ST1005"]
  1. Create a main.go file:
package main

import "fmt"

func main() error {
	return fmt.Errorf("This capitalized error should be ignored but isn't")
}
  1. Note that running staticcheck . or staticcheck main.go do not raise any errors
  2. Turn on vim, with govim installed and staticcheck enabled via call govim#config#Set("Staticcheck", 1)
  3. Note that govim flags error ST1005 on line 6

Reading the gopls logs from govim shows:

[Trace - 10:10:46.572 AM] Received notification 'textDocument/publishDiagnostics'.
Params: {"uri":"file:///private/tmp/govim-bug/main.go","version":1,"diagnostics":[{"range":{"start":{"line":5,"character":18},"end":{"line":5,"character":18}},"severity":2,"source":"ST1005","message":"error strings should not be capitalized","tags":[1]}]}

As you can see from the config, ST1005 ought to be ignored for this file.

(I feel pretty certain that govim isn't doing anything wrong in how it launches or communicates with gopls? But if I've got this error filed on the wrong side of that divide, I apologize)

What did you expect to see?

No ST1005 errors flagged on main.go

What did you see instead?

An ST1005 error flagged on main.go

@gopherbot gopherbot added this to the Unreleased milestone Jan 3, 2020
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Jan 3, 2020
@gopherbot
Copy link

Thank you for filing a gopls issue! Please take a look at the Troubleshooting guide, and make sure that you have provided all of the relevant information here.

@gopherbot gopherbot added the gopls Issues related to the Go language server, gopls. label Jan 3, 2020
@OneOfOne
Copy link
Contributor

OneOfOne commented Jan 3, 2020

Not sure if vim supports json config for gopls, however the way to do this in vscode is:

	"gopls": {
		"experimentalDisabledAnalyses": ["ST1000", "ST1003"],
		"staticcheck": true
	}

Which is based on https://github.com/golang/tools/blob/master/gopls/doc/settings.md

@llimllib
Copy link
Author

llimllib commented Jan 3, 2020

I read that page but missed that flag! I will look into setting that flag in govim.

It seems to me that the issue is still worthwhile because not checking the config file seems a surprising behavior?

@OneOfOne
Copy link
Contributor

OneOfOne commented Jan 3, 2020

It might be because gopls imports staticcheck directly rather than invoking the external binary, but not 100% sure.

@ianthehat
Copy link

Yes, gopls uses the static check analyzeers directly, and does not have or support all the features of the staticcheck binary. It runs the checkers in a very different way and will probably never fully overlap. There are some things in the config that it might be reasonable to converge, but it needs to be done in a way that works for non staticcheck analyzers too, so it needs some thought.

@adityaU
Copy link

adityaU commented May 9, 2020

#36373 (comment)

this is not working anymore. Are there still ways to disable individual checks of staticcheck ?

@OneOfOne
Copy link
Contributor

OneOfOne commented May 9, 2020

@adityaU yep, the field was changed a little:

"gopls": {
		"staticcheck": true,
		"analyses": {
			"ST1000": false,
			"ST1003": false
		}
// other options
	}

@adityaU
Copy link

adityaU commented May 9, 2020

@OneOfOne Thank you so much. :)

@stamblerre stamblerre changed the title x/tools/gopls: staticcheck ignores config file x/tools/gopls: respect staticcheck config file Jul 23, 2020
@sam-github
Copy link

It also doesn't appear to respect line based directives, like https://staticcheck.io/docs/#line-based-linter-directives, or does it have a different syntax for that?

@stamblerre stamblerre added this to To Do in gopls on-deck Feb 28, 2021
@stamblerre stamblerre moved this from To Do to P3 in gopls on-deck Aug 12, 2021
@GAZ082
Copy link

GAZ082 commented Oct 31, 2021

"gopls": {
		"staticcheck": true,
		"analyses": {
			"ST1000": false,
			"ST1003": false
		}
// other options
	}

Looks like this is not working anymore?

Version: 1.61.2
Commit: 6cba118ac49a1b88332f312a8f67186f7f3c1643
Date: 2021-10-19T14:58:13.605Z
Electron: 13.5.1
Chrome: 91.0.4472.164
Node.js: 14.16.0
V8: 9.1.269.39-electron.0
OS: Linux x64 5.12.18-1-ck-nehalem

@findleyr
Copy link
Contributor

findleyr commented Apr 5, 2022

@GAZ082 sorry for the slow response, but disabling individual analyses should definitely still work.

@findleyr
Copy link
Contributor

findleyr commented Apr 5, 2022

Looking into this now, it seems like we should actually already be loading the config file via the config.Analyzer, which is required by analyzers that use configuration, but that is failing somehow...

Or maybe it isn't. Warrants investigation.

If it does work, we could theoretically inject our view of the filesystem (including overlays) into config.Analyzer.Run, but something tells me that's a bad idea -- we can check with dominikh once we understand the current status better.

CC @dle8

@dominikh
Copy link
Member

dominikh commented Apr 6, 2022

In Staticcheck, there are two parts to configuration. Configuration that gets used by individual analyzes via config.Analyzer, and configuration that controls the staticcheck command as a whole. The former should just work in gopls. The latter requires explicit support by the analysis runner. Importantly, the checks option is handled by the analysis runner, not by the individual analyzes.

@findleyr
Copy link
Contributor

findleyr commented Apr 6, 2022

@dominikh is the checks option the only gap in configuration? In that case, assuming the integration via config.Analyzer works, it may not be worth changing the current behavior. For example, it looks like the current staticcheck API for using the config is via Runner.Run, which we wouldn't be able to use*. If my understanding is correct, then in order to fully honor the staticcheck config we'd have to interpret it ourselves, which I'd prefer not to do.

*truth be told, I'm not actually sure whether it's a bad idea to use Runner.Run, given that it has its own caching, does full analysis of the package graph, and holding onto the staticcheck IR doubles our memory footprint... but that is a separate discussion.

@dominikh
Copy link
Member

dominikh commented Apr 6, 2022

is the checks option the only gap in configuration?

It currently is, yes.

If my understanding is correct, then in order to fully honor the staticcheck config we'd have to interpret it ourselves, which I'd prefer not to do.

That is my understanding, too.

I'm not actually sure whether it's a bad idea to use Runner.Run

It would seem weird to use two different analysis runners IMO. Also, Runner manages its own concurrency and is designed for throughput. I'm not sure how well that would fit into gopls.

@findleyr
Copy link
Contributor

findleyr commented Apr 6, 2022

is the checks option the only gap in configuration?

It currently is, yes.

Thanks. In that case, I think we should not do anything here other than verify that config.Analyzer is WAI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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
No open projects
Development

No branches or pull requests