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 } }