Skip to content

Commit 796b347

Browse files
stamblerrestamblerre
committed
[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 <[email protected]> Run-TryBot: Rebecca Stambler <[email protected]> TryBot-Result: Gobot Gobot <[email protected]> Reviewed-by: Robert Findley <[email protected]>
1 parent 33aa62c commit 796b347

File tree

9 files changed

+175
-71
lines changed

9 files changed

+175
-71
lines changed

‎gopls/internal/hooks/analysis.go‎

Lines changed: 26 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -5,30 +5,39 @@
55
package hooks
66

77
import (
8+
"golang.org/x/tools/go/analysis"
89
"golang.org/x/tools/internal/lsp/source"
910
"honnef.co/go/tools/simple"
1011
"honnef.co/go/tools/staticcheck"
1112
"honnef.co/go/tools/stylecheck"
1213
)
1314

1415
func updateAnalyzers(options *source.Options) {
15-
if options.StaticCheck {
16-
for _, a := range simple.Analyzers {
17-
options.AddDefaultAnalyzer(a)
18-
}
19-
for _, a := range staticcheck.Analyzers {
20-
switch a.Name {
21-
case "SA5009":
22-
// This check conflicts with the vet printf check (golang/go#34494).
23-
case "SA5011":
24-
// This check relies on facts from dependencies, which
25-
// we don't currently compute.
26-
default:
27-
options.AddDefaultAnalyzer(a)
28-
}
29-
}
30-
for _, a := range stylecheck.Analyzers {
31-
options.AddDefaultAnalyzer(a)
16+
var analyzers []*analysis.Analyzer
17+
for _, a := range simple.Analyzers {
18+
analyzers = append(analyzers, a)
19+
}
20+
for _, a := range staticcheck.Analyzers {
21+
switch a.Name {
22+
case "SA5009":
23+
// This check conflicts with the vet printf check (golang/go#34494).
24+
case "SA5011":
25+
// This check relies on facts from dependencies, which
26+
// we don't currently compute.
27+
default:
28+
analyzers = append(analyzers, a)
3229
}
3330
}
31+
for _, a := range stylecheck.Analyzers {
32+
analyzers = append(analyzers, a)
33+
}
34+
// Always add hooks for all available analyzers, but disable them if the
35+
// user does not have staticcheck enabled (they may enable it later on).
36+
for _, a := range analyzers {
37+
addStaticcheckAnalyzer(options, a)
38+
}
39+
}
40+
41+
func addStaticcheckAnalyzer(options *source.Options, a *analysis.Analyzer) {
42+
options.StaticcheckAnalyzers[a.Name] = source.Analyzer{Analyzer: a, Enabled: true}
3443
}

‎gopls/internal/regtest/configuration_test.go‎

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
// Copyright 2020 The Go Authors. All rights reserved.
2+
// Use of this source code is governed by a BSD-style
3+
// license that can be found in the LICENSE file.
4+
5+
package regtest
6+
7+
import (
8+
"testing"
9+
10+
"golang.org/x/tools/internal/lsp"
11+
"golang.org/x/tools/internal/lsp/fake"
12+
)
13+
14+
// Test that enabling and disabling produces the expected results of showing
15+
// and hiding staticcheck analysis results.
16+
func TestChangeConfiguration(t *testing.T) {
17+
const files = `
18+
-- go.mod --
19+
module mod.com
20+
21+
go 1.12
22+
-- a/a.go --
23+
package a
24+
25+
// NotThisVariable should really start with ThisVariable.
26+
const ThisVariable = 7
27+
`
28+
run(t, files, func(t *testing.T, env *Env) {
29+
env.Await(
30+
CompletedWork(lsp.DiagnosticWorkTitle(lsp.FromInitialWorkspaceLoad), 1),
31+
)
32+
env.OpenFile("a/a.go")
33+
env.Await(
34+
CompletedWork(lsp.DiagnosticWorkTitle(lsp.FromDidOpen), 1),
35+
NoDiagnostics("a/a.go"),
36+
)
37+
cfg := &fake.EditorConfig{}
38+
*cfg = env.Editor.Config
39+
cfg.EnableStaticcheck = true
40+
env.changeConfiguration(t, cfg)
41+
env.Await(
42+
DiagnosticAt("a/a.go", 2, 0),
43+
)
44+
})
45+
}

‎gopls/internal/regtest/wrappers.go‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -266,6 +266,15 @@ func (e *Env) CodeAction(path string) []protocol.CodeAction {
266266
return actions
267267
}
268268

269+
func (e *Env) changeConfiguration(t *testing.T, config *fake.EditorConfig) {
270+
e.Editor.Config = *config
271+
if err := e.Editor.Server.DidChangeConfiguration(e.Ctx, &protocol.DidChangeConfigurationParams{
272+
// gopls currently ignores the Settings field
273+
}); err != nil {
274+
t.Fatal(err)
275+
}
276+
}
277+
269278
// ChangeEnv modifies the editor environment and reconfigures the LSP client.
270279
// TODO: extend this to "ChangeConfiguration", once we refactor the way editor
271280
// configuration is defined.

‎internal/lsp/cache/view.go‎

Lines changed: 23 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import (
1616
"path"
1717
"path/filepath"
1818
"reflect"
19+
"sort"
1920
"strings"
2021
"sync"
2122
"time"
@@ -252,10 +253,30 @@ func (v *View) Options() source.Options {
252253

253254
func minorOptionsChange(a, b source.Options) bool {
254255
// Check if any of the settings that modify our understanding of files have been changed
255-
if !reflect.DeepEqual(a.Env, b.Env) {
256+
mapEnv := func(env []string) map[string]string {
257+
m := make(map[string]string, len(env))
258+
for _, x := range env {
259+
split := strings.SplitN(x, "=", 2)
260+
if len(split) != 2 {
261+
continue
262+
}
263+
m[split[0]] = split[1]
264+
265+
}
266+
return m
267+
}
268+
aEnv := mapEnv(a.Env)
269+
bEnv := mapEnv(b.Env)
270+
if !reflect.DeepEqual(aEnv, bEnv) {
256271
return false
257272
}
258-
if !reflect.DeepEqual(a.BuildFlags, b.BuildFlags) {
273+
aBuildFlags := make([]string, len(a.BuildFlags))
274+
bBuildFlags := make([]string, len(b.BuildFlags))
275+
copy(aBuildFlags, a.BuildFlags)
276+
copy(bBuildFlags, b.BuildFlags)
277+
sort.Strings(aBuildFlags)
278+
sort.Strings(bBuildFlags)
279+
if !reflect.DeepEqual(aBuildFlags, bBuildFlags) {
259280
return false
260281
}
261282
// the rest of the options are benign

‎internal/lsp/code_action.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -316,7 +316,7 @@ func findSourceError(ctx context.Context, snapshot source.Snapshot, pkgID string
316316
func diagnosticToAnalyzer(snapshot source.Snapshot, src, msg string) (analyzer *source.Analyzer) {
317317
// Make sure that the analyzer we found is enabled.
318318
defer func() {
319-
if analyzer != nil && !analyzer.Enabled(snapshot.View()) {
319+
if analyzer != nil && !analyzer.IsEnabled(snapshot.View()) {
320320
analyzer = nil
321321
}
322322
}()
@@ -345,7 +345,7 @@ func diagnosticToAnalyzer(snapshot source.Snapshot, src, msg string) (analyzer *
345345
func convenienceFixes(ctx context.Context, snapshot source.Snapshot, pkg source.Package, uri span.URI, rng protocol.Range) ([]protocol.CodeAction, error) {
346346
var analyzers []*analysis.Analyzer
347347
for _, a := range snapshot.View().Options().ConvenienceAnalyzers {
348-
if !a.Enabled(snapshot.View()) {
348+
if !a.IsEnabled(snapshot.View()) {
349349
continue
350350
}
351351
if a.Command == nil {

‎internal/lsp/source/diagnostics.go‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -121,6 +121,9 @@ func pickAnalyzers(snapshot Snapshot, hadTypeErrors bool) map[string]Analyzer {
121121
for k, v := range snapshot.View().Options().DefaultAnalyzers {
122122
analyzers[k] = v
123123
}
124+
for k, v := range snapshot.View().Options().StaticcheckAnalyzers {
125+
analyzers[k] = v
126+
}
124127
return analyzers
125128
}
126129

@@ -202,7 +205,7 @@ func diagnostics(ctx context.Context, snapshot Snapshot, reports map[VersionedFi
202205
func analyses(ctx context.Context, snapshot Snapshot, reports map[VersionedFileIdentity][]*Diagnostic, pkg Package, analyses map[string]Analyzer) error {
203206
var analyzers []*analysis.Analyzer
204207
for _, a := range analyses {
205-
if !a.Enabled(snapshot.View()) {
208+
if !a.IsEnabled(snapshot.View()) {
206209
continue
207210
}
208211
analyzers = append(analyzers, a.Analyzer)

‎internal/lsp/source/options.go‎

Lines changed: 45 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,6 @@ import (
1111
"regexp"
1212
"time"
1313

14-
"golang.org/x/tools/go/analysis"
1514
"golang.org/x/tools/go/analysis/passes/asmdecl"
1615
"golang.org/x/tools/go/analysis/passes/assign"
1716
"golang.org/x/tools/go/analysis/passes/atomic"
@@ -120,6 +119,7 @@ func DefaultOptions() Options {
120119
DefaultAnalyzers: defaultAnalyzers(),
121120
TypeErrorAnalyzers: typeErrorAnalyzers(),
122121
ConvenienceAnalyzers: convenienceAnalyzers(),
122+
StaticcheckAnalyzers: map[string]Analyzer{},
123123
GoDiff: true,
124124
},
125125
}
@@ -259,13 +259,10 @@ type Hooks struct {
259259
DefaultAnalyzers map[string]Analyzer
260260
TypeErrorAnalyzers map[string]Analyzer
261261
ConvenienceAnalyzers map[string]Analyzer
262+
StaticcheckAnalyzers map[string]Analyzer
262263
GofumptFormat func(ctx context.Context, src []byte) ([]byte, error)
263264
}
264265

265-
func (o Options) AddDefaultAnalyzer(a *analysis.Analyzer) {
266-
o.DefaultAnalyzers[a.Name] = Analyzer{Analyzer: a, enabled: true}
267-
}
268-
269266
// ExperimentalOptions defines configuration for features under active
270267
// development. WARNING: This configuration will be changed in the future. It
271268
// only exists while these features are under development.
@@ -680,17 +677,22 @@ func (r *OptionResult) setString(s *string) {
680677
// snapshot.
681678
func EnabledAnalyzers(snapshot Snapshot) (analyzers []Analyzer) {
682679
for _, a := range snapshot.View().Options().DefaultAnalyzers {
683-
if a.Enabled(snapshot.View()) {
680+
if a.IsEnabled(snapshot.View()) {
684681
analyzers = append(analyzers, a)
685682
}
686683
}
687684
for _, a := range snapshot.View().Options().TypeErrorAnalyzers {
688-
if a.Enabled(snapshot.View()) {
685+
if a.IsEnabled(snapshot.View()) {
689686
analyzers = append(analyzers, a)
690687
}
691688
}
692689
for _, a := range snapshot.View().Options().ConvenienceAnalyzers {
693-
if a.Enabled(snapshot.View()) {
690+
if a.IsEnabled(snapshot.View()) {
691+
analyzers = append(analyzers, a)
692+
}
693+
}
694+
for _, a := range snapshot.View().Options().StaticcheckAnalyzers {
695+
if a.IsEnabled(snapshot.View()) {
694696
analyzers = append(analyzers, a)
695697
}
696698
}
@@ -703,23 +705,23 @@ func typeErrorAnalyzers() map[string]Analyzer {
703705
Analyzer: fillreturns.Analyzer,
704706
FixesError: fillreturns.FixesError,
705707
HighConfidence: true,
706-
enabled: true,
708+
Enabled: true,
707709
},
708710
nonewvars.Analyzer.Name: {
709711
Analyzer: nonewvars.Analyzer,
710712
FixesError: nonewvars.FixesError,
711-
enabled: true,
713+
Enabled: true,
712714
},
713715
noresultvalues.Analyzer.Name: {
714716
Analyzer: noresultvalues.Analyzer,
715717
FixesError: noresultvalues.FixesError,
716-
enabled: true,
718+
Enabled: true,
717719
},
718720
undeclaredname.Analyzer.Name: {
719721
Analyzer: undeclaredname.Analyzer,
720722
FixesError: undeclaredname.FixesError,
721723
Command: CommandUndeclaredName,
722-
enabled: true,
724+
Enabled: true,
723725
},
724726
}
725727
}
@@ -729,48 +731,48 @@ func convenienceAnalyzers() map[string]Analyzer {
729731
fillstruct.Analyzer.Name: {
730732
Analyzer: fillstruct.Analyzer,
731733
Command: CommandFillStruct,
732-
enabled: true,
734+
Enabled: true,
733735
},
734736
}
735737
}
736738

737739
func defaultAnalyzers() map[string]Analyzer {
738740
return map[string]Analyzer{
739741
// The traditional vet suite:
740-
asmdecl.Analyzer.Name: {Analyzer: asmdecl.Analyzer, enabled: true},
741-
assign.Analyzer.Name: {Analyzer: assign.Analyzer, enabled: true},
742-
atomic.Analyzer.Name: {Analyzer: atomic.Analyzer, enabled: true},
743-
atomicalign.Analyzer.Name: {Analyzer: atomicalign.Analyzer, enabled: true},
744-
bools.Analyzer.Name: {Analyzer: bools.Analyzer, enabled: true},
745-
buildtag.Analyzer.Name: {Analyzer: buildtag.Analyzer, enabled: true},
746-
cgocall.Analyzer.Name: {Analyzer: cgocall.Analyzer, enabled: true},
747-
composite.Analyzer.Name: {Analyzer: composite.Analyzer, enabled: true},
748-
copylock.Analyzer.Name: {Analyzer: copylock.Analyzer, enabled: true},
749-
errorsas.Analyzer.Name: {Analyzer: errorsas.Analyzer, enabled: true},
750-
httpresponse.Analyzer.Name: {Analyzer: httpresponse.Analyzer, enabled: true},
751-
loopclosure.Analyzer.Name: {Analyzer: loopclosure.Analyzer, enabled: true},
752-
lostcancel.Analyzer.Name: {Analyzer: lostcancel.Analyzer, enabled: true},
753-
nilfunc.Analyzer.Name: {Analyzer: nilfunc.Analyzer, enabled: true},
754-
printf.Analyzer.Name: {Analyzer: printf.Analyzer, enabled: true},
755-
shift.Analyzer.Name: {Analyzer: shift.Analyzer, enabled: true},
756-
stdmethods.Analyzer.Name: {Analyzer: stdmethods.Analyzer, enabled: true},
757-
structtag.Analyzer.Name: {Analyzer: structtag.Analyzer, enabled: true},
758-
tests.Analyzer.Name: {Analyzer: tests.Analyzer, enabled: true},
759-
unmarshal.Analyzer.Name: {Analyzer: unmarshal.Analyzer, enabled: true},
760-
unreachable.Analyzer.Name: {Analyzer: unreachable.Analyzer, enabled: true},
761-
unsafeptr.Analyzer.Name: {Analyzer: unsafeptr.Analyzer, enabled: true},
762-
unusedresult.Analyzer.Name: {Analyzer: unusedresult.Analyzer, enabled: true},
742+
asmdecl.Analyzer.Name: {Analyzer: asmdecl.Analyzer, Enabled: true},
743+
assign.Analyzer.Name: {Analyzer: assign.Analyzer, Enabled: true},
744+
atomic.Analyzer.Name: {Analyzer: atomic.Analyzer, Enabled: true},
745+
atomicalign.Analyzer.Name: {Analyzer: atomicalign.Analyzer, Enabled: true},
746+
bools.Analyzer.Name: {Analyzer: bools.Analyzer, Enabled: true},
747+
buildtag.Analyzer.Name: {Analyzer: buildtag.Analyzer, Enabled: true},
748+
cgocall.Analyzer.Name: {Analyzer: cgocall.Analyzer, Enabled: true},
749+
composite.Analyzer.Name: {Analyzer: composite.Analyzer, Enabled: true},
750+
copylock.Analyzer.Name: {Analyzer: copylock.Analyzer, Enabled: true},
751+
errorsas.Analyzer.Name: {Analyzer: errorsas.Analyzer, Enabled: true},
752+
httpresponse.Analyzer.Name: {Analyzer: httpresponse.Analyzer, Enabled: true},
753+
loopclosure.Analyzer.Name: {Analyzer: loopclosure.Analyzer, Enabled: true},
754+
lostcancel.Analyzer.Name: {Analyzer: lostcancel.Analyzer, Enabled: true},
755+
nilfunc.Analyzer.Name: {Analyzer: nilfunc.Analyzer, Enabled: true},
756+
printf.Analyzer.Name: {Analyzer: printf.Analyzer, Enabled: true},
757+
shift.Analyzer.Name: {Analyzer: shift.Analyzer, Enabled: true},
758+
stdmethods.Analyzer.Name: {Analyzer: stdmethods.Analyzer, Enabled: true},
759+
structtag.Analyzer.Name: {Analyzer: structtag.Analyzer, Enabled: true},
760+
tests.Analyzer.Name: {Analyzer: tests.Analyzer, Enabled: true},
761+
unmarshal.Analyzer.Name: {Analyzer: unmarshal.Analyzer, Enabled: true},
762+
unreachable.Analyzer.Name: {Analyzer: unreachable.Analyzer, Enabled: true},
763+
unsafeptr.Analyzer.Name: {Analyzer: unsafeptr.Analyzer, Enabled: true},
764+
unusedresult.Analyzer.Name: {Analyzer: unusedresult.Analyzer, Enabled: true},
763765

764766
// Non-vet analyzers:
765-
deepequalerrors.Analyzer.Name: {Analyzer: deepequalerrors.Analyzer, enabled: true},
766-
sortslice.Analyzer.Name: {Analyzer: sortslice.Analyzer, enabled: true},
767-
testinggoroutine.Analyzer.Name: {Analyzer: testinggoroutine.Analyzer, enabled: true},
768-
unusedparams.Analyzer.Name: {Analyzer: unusedparams.Analyzer, enabled: false},
767+
deepequalerrors.Analyzer.Name: {Analyzer: deepequalerrors.Analyzer, Enabled: true},
768+
sortslice.Analyzer.Name: {Analyzer: sortslice.Analyzer, Enabled: true},
769+
testinggoroutine.Analyzer.Name: {Analyzer: testinggoroutine.Analyzer, Enabled: true},
770+
unusedparams.Analyzer.Name: {Analyzer: unusedparams.Analyzer, Enabled: false},
769771

770772
// gofmt -s suite:
771-
simplifycompositelit.Analyzer.Name: {Analyzer: simplifycompositelit.Analyzer, enabled: true, HighConfidence: true},
772-
simplifyrange.Analyzer.Name: {Analyzer: simplifyrange.Analyzer, enabled: true, HighConfidence: true},
773-
simplifyslice.Analyzer.Name: {Analyzer: simplifyslice.Analyzer, enabled: true, HighConfidence: true},
773+
simplifycompositelit.Analyzer.Name: {Analyzer: simplifycompositelit.Analyzer, Enabled: true, HighConfidence: true},
774+
simplifyrange.Analyzer.Name: {Analyzer: simplifyrange.Analyzer, Enabled: true, HighConfidence: true},
775+
simplifyslice.Analyzer.Name: {Analyzer: simplifyslice.Analyzer, Enabled: true, HighConfidence: true},
774776
}
775777
}
776778

‎internal/lsp/source/view.go‎

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -420,7 +420,11 @@ const (
420420
// that let the user know how to use the analyzer.
421421
type Analyzer struct {
422422
Analyzer *analysis.Analyzer
423-
enabled bool
423+
424+
// Enabled reports whether the analyzer is enabled. This value can be
425+
// configured per-analysis in user settings. For staticcheck analyzers,
426+
// the value of the Staticcheck setting overrides this field.
427+
Enabled bool
424428

425429
// Command is the name of the command used to invoke the suggested fixes
426430
// for the analyzer. It is non-nil if we expect this analyzer to provide
@@ -438,11 +442,17 @@ type Analyzer struct {
438442
FixesError func(msg string) bool
439443
}
440444

441-
func (a Analyzer) Enabled(view View) bool {
445+
func (a Analyzer) IsEnabled(view View) bool {
446+
// Staticcheck analyzers can only be enabled when staticcheck is on.
447+
if _, ok := view.Options().StaticcheckAnalyzers[a.Analyzer.Name]; ok {
448+
if !view.Options().StaticCheck {
449+
return false
450+
}
451+
}
442452
if enabled, ok := view.Options().UserEnabledAnalyses[a.Analyzer.Name]; ok {
443453
return enabled
444454
}
445-
return a.enabled
455+
return a.Enabled
446456
}
447457

448458
// Package represents a Go package that has been type-checked. It maintains

0 commit comments

Comments
 (0)