internal/lsp: handle staticcheck in didChangeConfiguration
As we have modified the ways that we control which analyzers get
executed for a given case, we have lost the behavior of enabling and
disabling staticcheck smoothly. This CL splits out the staticcheck
analyzers from the main group so that the "staticcheck" setting can
override whether or not a given staticcheck analysis is enabled.
Fixes golang/go#41311
Change-Id: I9c1695afe4a8f89cd0ee50a79e83b2f42a2c20cb
Reviewed-on: https://go-review.googlesource.com/c/tools/+/254038
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
diff --git a/gopls/internal/hooks/analysis.go b/gopls/internal/hooks/analysis.go
index 2f263c7..4eb042d 100644
--- a/gopls/internal/hooks/analysis.go
+++ b/gopls/internal/hooks/analysis.go
@@ -5,6 +5,7 @@
package hooks
import (
+ "golang.org/x/tools/go/analysis"
"golang.org/x/tools/internal/lsp/source"
"honnef.co/go/tools/simple"
"honnef.co/go/tools/staticcheck"
@@ -12,23 +13,31 @@
)
func updateAnalyzers(options *source.Options) {
- if options.Staticcheck {
- for _, a := range simple.Analyzers {
- options.AddDefaultAnalyzer(a)
- }
- for _, a := range staticcheck.Analyzers {
- switch a.Name {
- case "SA5009":
- // This check conflicts with the vet printf check (golang/go#34494).
- case "SA5011":
- // This check relies on facts from dependencies, which
- // we don't currently compute.
- default:
- options.AddDefaultAnalyzer(a)
- }
- }
- for _, a := range stylecheck.Analyzers {
- options.AddDefaultAnalyzer(a)
+ var analyzers []*analysis.Analyzer
+ for _, a := range simple.Analyzers {
+ analyzers = append(analyzers, a)
+ }
+ for _, a := range staticcheck.Analyzers {
+ switch a.Name {
+ case "SA5009":
+ // This check conflicts with the vet printf check (golang/go#34494).
+ case "SA5011":
+ // This check relies on facts from dependencies, which
+ // we don't currently compute.
+ default:
+ analyzers = append(analyzers, a)
}
}
+ for _, a := range stylecheck.Analyzers {
+ analyzers = append(analyzers, a)
+ }
+ // Always add hooks for all available analyzers, but disable them if the
+ // user does not have staticcheck enabled (they may enable it later on).
+ for _, a := range analyzers {
+ addStaticcheckAnalyzer(options, a)
+ }
+}
+
+func addStaticcheckAnalyzer(options *source.Options, a *analysis.Analyzer) {
+ options.StaticcheckAnalyzers[a.Name] = source.Analyzer{Analyzer: a, Enabled: true}
}
diff --git a/gopls/internal/regtest/configuration_test.go b/gopls/internal/regtest/configuration_test.go
new file mode 100644
index 0000000..47dfc42
--- /dev/null
+++ b/gopls/internal/regtest/configuration_test.go
@@ -0,0 +1,45 @@
+// Copyright 2020 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package regtest
+
+import (
+ "testing"
+
+ "golang.org/x/tools/internal/lsp"
+ "golang.org/x/tools/internal/lsp/fake"
+)
+
+// Test that enabling and disabling produces the expected results of showing
+// and hiding staticcheck analysis results.
+func TestChangeConfiguration(t *testing.T) {
+ const files = `
+-- go.mod --
+module mod.com
+
+go 1.12
+-- a/a.go --
+package a
+
+// NotThisVariable should really start with ThisVariable.
+const ThisVariable = 7
+`
+ run(t, files, func(t *testing.T, env *Env) {
+ env.Await(
+ CompletedWork(lsp.DiagnosticWorkTitle(lsp.FromInitialWorkspaceLoad), 1),
+ )
+ env.OpenFile("a/a.go")
+ env.Await(
+ CompletedWork(lsp.DiagnosticWorkTitle(lsp.FromDidOpen), 1),
+ NoDiagnostics("a/a.go"),
+ )
+ cfg := &fake.EditorConfig{}
+ *cfg = env.Editor.Config
+ cfg.EnableStaticcheck = true
+ env.changeConfiguration(t, cfg)
+ env.Await(
+ DiagnosticAt("a/a.go", 2, 0),
+ )
+ })
+}
diff --git a/gopls/internal/regtest/wrappers.go b/gopls/internal/regtest/wrappers.go
index b84175f..7ac01a8 100644
--- a/gopls/internal/regtest/wrappers.go
+++ b/gopls/internal/regtest/wrappers.go
@@ -266,6 +266,15 @@
return actions
}
+func (e *Env) changeConfiguration(t *testing.T, config *fake.EditorConfig) {
+ e.Editor.Config = *config
+ if err := e.Editor.Server.DidChangeConfiguration(e.Ctx, &protocol.DidChangeConfigurationParams{
+ // gopls currently ignores the Settings field
+ }); err != nil {
+ t.Fatal(err)
+ }
+}
+
// ChangeEnv modifies the editor environment and reconfigures the LSP client.
// TODO: extend this to "ChangeConfiguration", once we refactor the way editor
// configuration is defined.
diff --git a/internal/lsp/cache/view.go b/internal/lsp/cache/view.go
index 945cdd4..5154ef7 100644
--- a/internal/lsp/cache/view.go
+++ b/internal/lsp/cache/view.go
@@ -18,6 +18,7 @@
"path/filepath"
"reflect"
"regexp"
+ "sort"
"strings"
"sync"
"time"
@@ -285,10 +286,29 @@
func minorOptionsChange(a, b source.Options) bool {
// Check if any of the settings that modify our understanding of files have been changed
- if !reflect.DeepEqual(a.Env, b.Env) {
+ mapEnv := func(env []string) map[string]string {
+ m := make(map[string]string, len(env))
+ for _, x := range env {
+ split := strings.SplitN(x, "=", 2)
+ if len(split) != 2 {
+ continue
+ }
+ m[split[0]] = split[1]
+ }
+ return m
+ }
+ aEnv := mapEnv(a.Env)
+ bEnv := mapEnv(b.Env)
+ if !reflect.DeepEqual(aEnv, bEnv) {
return false
}
- if !reflect.DeepEqual(a.BuildFlags, b.BuildFlags) {
+ aBuildFlags := make([]string, len(a.BuildFlags))
+ bBuildFlags := make([]string, len(b.BuildFlags))
+ copy(aBuildFlags, a.BuildFlags)
+ copy(bBuildFlags, b.BuildFlags)
+ sort.Strings(aBuildFlags)
+ sort.Strings(bBuildFlags)
+ if !reflect.DeepEqual(aBuildFlags, bBuildFlags) {
return false
}
// the rest of the options are benign
diff --git a/internal/lsp/code_action.go b/internal/lsp/code_action.go
index 7fb3a5c..7ad924d 100644
--- a/internal/lsp/code_action.go
+++ b/internal/lsp/code_action.go
@@ -320,7 +320,7 @@
func diagnosticToAnalyzer(snapshot source.Snapshot, src, msg string) (analyzer *source.Analyzer) {
// Make sure that the analyzer we found is enabled.
defer func() {
- if analyzer != nil && !analyzer.Enabled(snapshot.View()) {
+ if analyzer != nil && !analyzer.IsEnabled(snapshot.View()) {
analyzer = nil
}
}()
@@ -349,7 +349,7 @@
func convenienceFixes(ctx context.Context, snapshot source.Snapshot, pkg source.Package, uri span.URI, rng protocol.Range) ([]protocol.CodeAction, error) {
var analyzers []*analysis.Analyzer
for _, a := range snapshot.View().Options().ConvenienceAnalyzers {
- if !a.Enabled(snapshot.View()) {
+ if !a.IsEnabled(snapshot.View()) {
continue
}
if a.Command == nil {
diff --git a/internal/lsp/source/diagnostics.go b/internal/lsp/source/diagnostics.go
index 320d39b..9f1b091 100644
--- a/internal/lsp/source/diagnostics.go
+++ b/internal/lsp/source/diagnostics.go
@@ -121,6 +121,9 @@
for k, v := range snapshot.View().Options().DefaultAnalyzers {
analyzers[k] = v
}
+ for k, v := range snapshot.View().Options().StaticcheckAnalyzers {
+ analyzers[k] = v
+ }
return analyzers
}
@@ -202,7 +205,7 @@
func analyses(ctx context.Context, snapshot Snapshot, reports map[VersionedFileIdentity][]*Diagnostic, pkg Package, analyses map[string]Analyzer) error {
var analyzers []*analysis.Analyzer
for _, a := range analyses {
- if !a.Enabled(snapshot.View()) {
+ if !a.IsEnabled(snapshot.View()) {
continue
}
analyzers = append(analyzers, a.Analyzer)
diff --git a/internal/lsp/source/options.go b/internal/lsp/source/options.go
index a1a415c..c70c2a5 100644
--- a/internal/lsp/source/options.go
+++ b/internal/lsp/source/options.go
@@ -11,7 +11,6 @@
"strings"
"time"
- "golang.org/x/tools/go/analysis"
"golang.org/x/tools/go/analysis/passes/asmdecl"
"golang.org/x/tools/go/analysis/passes/assign"
"golang.org/x/tools/go/analysis/passes/atomic"
@@ -122,6 +121,7 @@
DefaultAnalyzers: defaultAnalyzers(),
TypeErrorAnalyzers: typeErrorAnalyzers(),
ConvenienceAnalyzers: convenienceAnalyzers(),
+ StaticcheckAnalyzers: map[string]Analyzer{},
GoDiff: true,
},
}
@@ -209,13 +209,10 @@
DefaultAnalyzers map[string]Analyzer
TypeErrorAnalyzers map[string]Analyzer
ConvenienceAnalyzers map[string]Analyzer
+ StaticcheckAnalyzers map[string]Analyzer
GofumptFormat func(ctx context.Context, src []byte) ([]byte, error)
}
-func (o Options) AddDefaultAnalyzer(a *analysis.Analyzer) {
- o.DefaultAnalyzers[a.Name] = Analyzer{Analyzer: a, enabled: true}
-}
-
// ExperimentalOptions defines configuration for features under active
// development. WARNING: This configuration will be changed in the future. It
// only exists while these features are under development.
@@ -758,17 +755,22 @@
// snapshot.
func EnabledAnalyzers(snapshot Snapshot) (analyzers []Analyzer) {
for _, a := range snapshot.View().Options().DefaultAnalyzers {
- if a.Enabled(snapshot.View()) {
+ if a.IsEnabled(snapshot.View()) {
analyzers = append(analyzers, a)
}
}
for _, a := range snapshot.View().Options().TypeErrorAnalyzers {
- if a.Enabled(snapshot.View()) {
+ if a.IsEnabled(snapshot.View()) {
analyzers = append(analyzers, a)
}
}
for _, a := range snapshot.View().Options().ConvenienceAnalyzers {
- if a.Enabled(snapshot.View()) {
+ if a.IsEnabled(snapshot.View()) {
+ analyzers = append(analyzers, a)
+ }
+ }
+ for _, a := range snapshot.View().Options().StaticcheckAnalyzers {
+ if a.IsEnabled(snapshot.View()) {
analyzers = append(analyzers, a)
}
}
@@ -781,23 +783,23 @@
Analyzer: fillreturns.Analyzer,
FixesError: fillreturns.FixesError,
HighConfidence: true,
- enabled: true,
+ Enabled: true,
},
nonewvars.Analyzer.Name: {
Analyzer: nonewvars.Analyzer,
FixesError: nonewvars.FixesError,
- enabled: true,
+ Enabled: true,
},
noresultvalues.Analyzer.Name: {
Analyzer: noresultvalues.Analyzer,
FixesError: noresultvalues.FixesError,
- enabled: true,
+ Enabled: true,
},
undeclaredname.Analyzer.Name: {
Analyzer: undeclaredname.Analyzer,
FixesError: undeclaredname.FixesError,
Command: CommandUndeclaredName,
- enabled: true,
+ Enabled: true,
},
}
}
@@ -807,7 +809,7 @@
fillstruct.Analyzer.Name: {
Analyzer: fillstruct.Analyzer,
Command: CommandFillStruct,
- enabled: true,
+ Enabled: true,
},
}
}
@@ -815,40 +817,40 @@
func defaultAnalyzers() map[string]Analyzer {
return map[string]Analyzer{
// The traditional vet suite:
- asmdecl.Analyzer.Name: {Analyzer: asmdecl.Analyzer, enabled: true},
- assign.Analyzer.Name: {Analyzer: assign.Analyzer, enabled: true},
- atomic.Analyzer.Name: {Analyzer: atomic.Analyzer, enabled: true},
- atomicalign.Analyzer.Name: {Analyzer: atomicalign.Analyzer, enabled: true},
- bools.Analyzer.Name: {Analyzer: bools.Analyzer, enabled: true},
- buildtag.Analyzer.Name: {Analyzer: buildtag.Analyzer, enabled: true},
- cgocall.Analyzer.Name: {Analyzer: cgocall.Analyzer, enabled: true},
- composite.Analyzer.Name: {Analyzer: composite.Analyzer, enabled: true},
- copylock.Analyzer.Name: {Analyzer: copylock.Analyzer, enabled: true},
- errorsas.Analyzer.Name: {Analyzer: errorsas.Analyzer, enabled: true},
- httpresponse.Analyzer.Name: {Analyzer: httpresponse.Analyzer, enabled: true},
- loopclosure.Analyzer.Name: {Analyzer: loopclosure.Analyzer, enabled: true},
- lostcancel.Analyzer.Name: {Analyzer: lostcancel.Analyzer, enabled: true},
- nilfunc.Analyzer.Name: {Analyzer: nilfunc.Analyzer, enabled: true},
- printf.Analyzer.Name: {Analyzer: printf.Analyzer, enabled: true},
- shift.Analyzer.Name: {Analyzer: shift.Analyzer, enabled: true},
- stdmethods.Analyzer.Name: {Analyzer: stdmethods.Analyzer, enabled: true},
- structtag.Analyzer.Name: {Analyzer: structtag.Analyzer, enabled: true},
- tests.Analyzer.Name: {Analyzer: tests.Analyzer, enabled: true},
- unmarshal.Analyzer.Name: {Analyzer: unmarshal.Analyzer, enabled: true},
- unreachable.Analyzer.Name: {Analyzer: unreachable.Analyzer, enabled: true},
- unsafeptr.Analyzer.Name: {Analyzer: unsafeptr.Analyzer, enabled: true},
- unusedresult.Analyzer.Name: {Analyzer: unusedresult.Analyzer, enabled: true},
+ asmdecl.Analyzer.Name: {Analyzer: asmdecl.Analyzer, Enabled: true},
+ assign.Analyzer.Name: {Analyzer: assign.Analyzer, Enabled: true},
+ atomic.Analyzer.Name: {Analyzer: atomic.Analyzer, Enabled: true},
+ atomicalign.Analyzer.Name: {Analyzer: atomicalign.Analyzer, Enabled: true},
+ bools.Analyzer.Name: {Analyzer: bools.Analyzer, Enabled: true},
+ buildtag.Analyzer.Name: {Analyzer: buildtag.Analyzer, Enabled: true},
+ cgocall.Analyzer.Name: {Analyzer: cgocall.Analyzer, Enabled: true},
+ composite.Analyzer.Name: {Analyzer: composite.Analyzer, Enabled: true},
+ copylock.Analyzer.Name: {Analyzer: copylock.Analyzer, Enabled: true},
+ errorsas.Analyzer.Name: {Analyzer: errorsas.Analyzer, Enabled: true},
+ httpresponse.Analyzer.Name: {Analyzer: httpresponse.Analyzer, Enabled: true},
+ loopclosure.Analyzer.Name: {Analyzer: loopclosure.Analyzer, Enabled: true},
+ lostcancel.Analyzer.Name: {Analyzer: lostcancel.Analyzer, Enabled: true},
+ nilfunc.Analyzer.Name: {Analyzer: nilfunc.Analyzer, Enabled: true},
+ printf.Analyzer.Name: {Analyzer: printf.Analyzer, Enabled: true},
+ shift.Analyzer.Name: {Analyzer: shift.Analyzer, Enabled: true},
+ stdmethods.Analyzer.Name: {Analyzer: stdmethods.Analyzer, Enabled: true},
+ structtag.Analyzer.Name: {Analyzer: structtag.Analyzer, Enabled: true},
+ tests.Analyzer.Name: {Analyzer: tests.Analyzer, Enabled: true},
+ unmarshal.Analyzer.Name: {Analyzer: unmarshal.Analyzer, Enabled: true},
+ unreachable.Analyzer.Name: {Analyzer: unreachable.Analyzer, Enabled: true},
+ unsafeptr.Analyzer.Name: {Analyzer: unsafeptr.Analyzer, Enabled: true},
+ unusedresult.Analyzer.Name: {Analyzer: unusedresult.Analyzer, Enabled: true},
// Non-vet analyzers:
- deepequalerrors.Analyzer.Name: {Analyzer: deepequalerrors.Analyzer, enabled: true},
- sortslice.Analyzer.Name: {Analyzer: sortslice.Analyzer, enabled: true},
- testinggoroutine.Analyzer.Name: {Analyzer: testinggoroutine.Analyzer, enabled: true},
- unusedparams.Analyzer.Name: {Analyzer: unusedparams.Analyzer, enabled: false},
+ deepequalerrors.Analyzer.Name: {Analyzer: deepequalerrors.Analyzer, Enabled: true},
+ sortslice.Analyzer.Name: {Analyzer: sortslice.Analyzer, Enabled: true},
+ testinggoroutine.Analyzer.Name: {Analyzer: testinggoroutine.Analyzer, Enabled: true},
+ unusedparams.Analyzer.Name: {Analyzer: unusedparams.Analyzer, Enabled: false},
// gofmt -s suite:
- simplifycompositelit.Analyzer.Name: {Analyzer: simplifycompositelit.Analyzer, enabled: true, HighConfidence: true},
- simplifyrange.Analyzer.Name: {Analyzer: simplifyrange.Analyzer, enabled: true, HighConfidence: true},
- simplifyslice.Analyzer.Name: {Analyzer: simplifyslice.Analyzer, enabled: true, HighConfidence: true},
+ simplifycompositelit.Analyzer.Name: {Analyzer: simplifycompositelit.Analyzer, Enabled: true, HighConfidence: true},
+ simplifyrange.Analyzer.Name: {Analyzer: simplifyrange.Analyzer, Enabled: true, HighConfidence: true},
+ simplifyslice.Analyzer.Name: {Analyzer: simplifyslice.Analyzer, Enabled: true, HighConfidence: true},
}
}
diff --git a/internal/lsp/source/view.go b/internal/lsp/source/view.go
index 34e6b31..099d870 100644
--- a/internal/lsp/source/view.go
+++ b/internal/lsp/source/view.go
@@ -420,7 +420,11 @@
// that let the user know how to use the analyzer.
type Analyzer struct {
Analyzer *analysis.Analyzer
- enabled bool
+
+ // Enabled reports whether the analyzer is enabled. This value can be
+ // configured per-analysis in user settings. For staticcheck analyzers,
+ // the value of the Staticcheck setting overrides this field.
+ Enabled bool
// Command is the name of the command used to invoke the suggested fixes
// for the analyzer. It is non-nil if we expect this analyzer to provide
@@ -438,11 +442,17 @@
FixesError func(msg string) bool
}
-func (a Analyzer) Enabled(view View) bool {
+func (a Analyzer) IsEnabled(view View) bool {
+ // Staticcheck analyzers can only be enabled when staticcheck is on.
+ if _, ok := view.Options().StaticcheckAnalyzers[a.Analyzer.Name]; ok {
+ if !view.Options().Staticcheck {
+ return false
+ }
+ }
if enabled, ok := view.Options().Analyses[a.Analyzer.Name]; ok {
return enabled
}
- return a.enabled
+ return a.Enabled
}
// Package represents a Go package that has been type-checked. It maintains
diff --git a/internal/lsp/tests/util.go b/internal/lsp/tests/util.go
index 83a204d..346eea3 100644
--- a/internal/lsp/tests/util.go
+++ b/internal/lsp/tests/util.go
@@ -530,17 +530,22 @@
opts.Analyses = make(map[string]bool)
}
for _, a := range opts.DefaultAnalyzers {
- if !a.Enabled(view) {
+ if !a.IsEnabled(view) {
opts.Analyses[a.Analyzer.Name] = true
}
}
for _, a := range opts.TypeErrorAnalyzers {
- if !a.Enabled(view) {
+ if !a.IsEnabled(view) {
opts.Analyses[a.Analyzer.Name] = true
}
}
for _, a := range opts.ConvenienceAnalyzers {
- if !a.Enabled(view) {
+ if !a.IsEnabled(view) {
+ opts.Analyses[a.Analyzer.Name] = true
+ }
+ }
+ for _, a := range opts.StaticcheckAnalyzers {
+ if !a.IsEnabled(view) {
opts.Analyses[a.Analyzer.Name] = true
}
}
