From 97363e29fc9b716e0d1e7c28a1098c5db06248f6 Mon Sep 17 00:00:00 2001 From: Rebecca Stambler Date: Thu, 10 Sep 2020 16:48:35 -0400 Subject: [PATCH] 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 gopls-CI: kokoro TryBot-Result: Gobot Gobot Reviewed-by: Robert Findley --- gopls/internal/hooks/analysis.go | 43 ++++++---- gopls/internal/regtest/configuration_test.go | 45 ++++++++++ gopls/internal/regtest/wrappers.go | 9 ++ internal/lsp/cache/view.go | 24 +++++- internal/lsp/code_action.go | 4 +- internal/lsp/source/diagnostics.go | 5 +- internal/lsp/source/options.go | 88 ++++++++++---------- internal/lsp/source/view.go | 16 +++- internal/lsp/tests/util.go | 11 ++- 9 files changed, 174 insertions(+), 71 deletions(-) create mode 100644 gopls/internal/regtest/configuration_test.go diff --git a/gopls/internal/hooks/analysis.go b/gopls/internal/hooks/analysis.go index 2f263c76be..4eb042d3a1 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 @@ import ( ) 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 0000000000..47dfc421ea --- /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 b84175f3d9..7ac01a886a 100644 --- a/gopls/internal/regtest/wrappers.go +++ b/gopls/internal/regtest/wrappers.go @@ -266,6 +266,15 @@ func (e *Env) CodeAction(path string) []protocol.CodeAction { 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 945cdd4fd0..5154ef76f5 100644 --- a/internal/lsp/cache/view.go +++ b/internal/lsp/cache/view.go @@ -18,6 +18,7 @@ import ( "path/filepath" "reflect" "regexp" + "sort" "strings" "sync" "time" @@ -285,10 +286,29 @@ func (v *View) Options() source.Options { 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 7fb3a5c33c..7ad924dfe2 100644 --- a/internal/lsp/code_action.go +++ b/internal/lsp/code_action.go @@ -320,7 +320,7 @@ func findSourceError(ctx context.Context, snapshot source.Snapshot, pkgID string 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 diagnosticToAnalyzer(snapshot source.Snapshot, src, msg string) (analyzer * 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 320d39bcec..9f1b091f11 100644 --- a/internal/lsp/source/diagnostics.go +++ b/internal/lsp/source/diagnostics.go @@ -121,6 +121,9 @@ func pickAnalyzers(snapshot Snapshot, hadTypeErrors bool) map[string]Analyzer { 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 diagnostics(ctx context.Context, snapshot Snapshot, reports map[VersionedFi 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 a1a415cd56..c70c2a543b 100644 --- a/internal/lsp/source/options.go +++ b/internal/lsp/source/options.go @@ -11,7 +11,6 @@ import ( "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 @@ func DefaultOptions() Options { DefaultAnalyzers: defaultAnalyzers(), TypeErrorAnalyzers: typeErrorAnalyzers(), ConvenienceAnalyzers: convenienceAnalyzers(), + StaticcheckAnalyzers: map[string]Analyzer{}, GoDiff: true, }, } @@ -209,13 +209,10 @@ type Hooks struct { 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 @@ func (r *OptionResult) setString(s *string) { // 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 @@ func typeErrorAnalyzers() map[string]Analyzer { 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 @@ func convenienceAnalyzers() map[string]Analyzer { fillstruct.Analyzer.Name: { Analyzer: fillstruct.Analyzer, Command: CommandFillStruct, - enabled: true, + Enabled: true, }, } } @@ -815,40 +817,40 @@ func convenienceAnalyzers() map[string]Analyzer { 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 34e6b317ec..099d87014e 100644 --- a/internal/lsp/source/view.go +++ b/internal/lsp/source/view.go @@ -420,7 +420,11 @@ const ( // 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 @@ type Analyzer struct { 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 83a204d0ac..346eea3e98 100644 --- a/internal/lsp/tests/util.go +++ b/internal/lsp/tests/util.go @@ -530,17 +530,22 @@ func EnableAllAnalyzers(view source.View, opts *source.Options) { 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 } }