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>

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

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),
+		)
+	})
+}

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.

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

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 {

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)

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

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

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