[gopls-release-branch.0.5] 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/+/254427
Trust: Rebecca Stambler <rstambler@golang.org>
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
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 f46a93f..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 bfb68ab..f3479f8 100644
--- a/internal/lsp/cache/view.go
+++ b/internal/lsp/cache/view.go
@@ -16,6 +16,7 @@
"path"
"path/filepath"
"reflect"
+ "sort"
"strings"
"sync"
"time"
@@ -252,10 +253,30 @@
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 d9b90f8..bb76ff9 100644
--- a/internal/lsp/code_action.go
+++ b/internal/lsp/code_action.go
@@ -316,7 +316,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
}
}()
@@ -345,7 +345,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 dd026cb..924a7ff 100644
--- a/internal/lsp/source/options.go
+++ b/internal/lsp/source/options.go
@@ -11,7 +11,6 @@
"regexp"
"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"
@@ -120,6 +119,7 @@
DefaultAnalyzers: defaultAnalyzers(),
TypeErrorAnalyzers: typeErrorAnalyzers(),
ConvenienceAnalyzers: convenienceAnalyzers(),
+ StaticcheckAnalyzers: map[string]Analyzer{},
GoDiff: true,
},
}
@@ -259,13 +259,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.
@@ -680,17 +677,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)
}
}
@@ -703,23 +705,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,
},
}
}
@@ -729,7 +731,7 @@
fillstruct.Analyzer.Name: {
Analyzer: fillstruct.Analyzer,
Command: CommandFillStruct,
- enabled: true,
+ Enabled: true,
},
}
}
@@ -737,40 +739,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 e463491..445d48c 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().UserEnabledAnalyses[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 fbef835..caa45cb 100644
--- a/internal/lsp/tests/util.go
+++ b/internal/lsp/tests/util.go
@@ -530,17 +530,22 @@
opts.UserEnabledAnalyses = make(map[string]bool)
}
for _, a := range opts.DefaultAnalyzers {
- if !a.Enabled(view) {
+ if !a.IsEnabled(view) {
opts.UserEnabledAnalyses[a.Analyzer.Name] = true
}
}
for _, a := range opts.TypeErrorAnalyzers {
- if !a.Enabled(view) {
+ if !a.IsEnabled(view) {
opts.UserEnabledAnalyses[a.Analyzer.Name] = true
}
}
for _, a := range opts.ConvenienceAnalyzers {
- if !a.Enabled(view) {
+ if !a.IsEnabled(view) {
+ opts.UserEnabledAnalyses[a.Analyzer.Name] = true
+ }
+ }
+ for _, a := range opts.StaticcheckAnalyzers {
+ if !a.IsEnabled(view) {
opts.UserEnabledAnalyses[a.Analyzer.Name] = true
}
}
