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: add the shadow analyser #43245

Closed
ainar-g opened this issue Dec 17, 2020 · 5 comments
Closed

x/tools/gopls: add the shadow analyser #43245

ainar-g opened this issue Dec 17, 2020 · 5 comments

Comments

@ainar-g
Copy link
Contributor

@ainar-g ainar-g commented Dec 17, 2020

I would like to request the ability to enable the shadow analyser in my gopls. I think it's a very useful analyser, and I tend to add it to my project's CI/CD pipelines anyways, so it would be nice to be able to immediately see when I accidentally introduce shadowed variables into my code or when I can fix an existing shadowing issue.

A proposed settings object could look something like this:

{
  // …
, "analyses": {
    // …
  , "shadow": "strict"
  }
}

The possible values are:

  • "", disable the analyser, the default value.
  • "nonstrict", enable without the strict option.
  • "strict", enable with the strict option.
@ainar-g
Copy link
Contributor Author

@ainar-g ainar-g commented Dec 19, 2020

I'm trying my hand at this, but it seems like implementing the API that I've proposed could be tricky, since .../source.UserOptions.Analyzers is currently a map[string]bool, and changing the type of the values there from a bool to some kind of sum-type-thru-interface-type could introduce a lot of changes. If there is an existing mechanism of configuring options for analysers, I couldn't find it.

So, I could add two boolean options, "shadow":true and "shadowstrict":true, and make the latter override the former. Or, I could add an "analyzerFlags" setting:

{
  // …
, "analyzerFlags": {
  // …
  , "shadow": ["--strict"]
  }
}

Or, there may be an option (heh) I don't see.

How should I proceed?

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Dec 21, 2020

I think the path that fits best with LSP would be a map[string]interface{} where each value is the set of options for the given analyzer. However, I think the current strategy of passing flags to the analyzer will be not be compatible with our analysis runner, so there may need to be some new API for passing options to analyzers (cc @matloob).

As a starting point, could we add the shadow analyzer as off-by-default and with strict turned off?

@gopherbot
Copy link

@gopherbot gopherbot commented Dec 22, 2020

Change https://golang.org/cl/279395 mentions this issue: internal/lsp/source: add the shadow analyzer

@ainar-g
Copy link
Contributor Author

@ainar-g ainar-g commented Dec 22, 2020

@stamblerre, done.

Probably unrelated, but while working on the change I've found this go:generate directive which doesn't work on my machine:

//go:generate go run golang.org/x/tools/internal/lsp/source/genapijson -output api_json.go

The error I got was:

$ go generate ./internal/lsp/source/
go: finding module for package golang.org/x/tools/internal/lsp/source/genapijson
no matching versions for query "latest"
internal/lsp/source/options.go:67: running "go": exit status 1
@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Dec 23, 2020

Thanks for reporting that--I believe it's outdated. /cc @heschik

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants