From 7cc24c2ba3c55853b2af5e779e48f61198e734f4 Mon Sep 17 00:00:00 2001 From: Robert Findley Date: Wed, 30 Mar 2022 16:45:36 -0400 Subject: [PATCH] gopls: upgrade staticcheck to v0.3.0 Upgrade staticcheck to v0.3.0 to pick up support for generics. Since this version only supports Go 1.17+, increase the version at which we install staticcheck to 1.17 (from 1.15). This change is likely to cause confusion for users on Go 1.16, so show a warning when setting the "staticcheck" option to an unsupported value. Slightly refactor our setting of options along the way. Fixes golang/go#52159 Change-Id: Id9b4cee340e31988c64ca712d98573343aaf5848 Reviewed-on: https://go-review.googlesource.com/c/tools/+/396974 Trust: Robert Findley Run-TryBot: Robert Findley Trust: Peter Weinberger Reviewed-by: Peter Weinberger gopls-CI: kokoro --- gopls/go.mod | 5 +- gopls/go.sum | 6 +- gopls/internal/hooks/analysis.go | 6 +- .../{analysis_115.go => analysis_117.go} | 8 +- gopls/internal/hooks/licenses_test.go | 2 +- .../regtest/diagnostics/diagnostics_test.go | 6 +- .../regtest/misc/configuration_test.go | 37 ++++++- internal/lsp/fake/editor.go | 6 - internal/lsp/general.go | 34 ++---- internal/lsp/regtest/expectation.go | 8 +- internal/lsp/source/options.go | 103 +++++++++++------- internal/lsp/source/options_test.go | 7 +- 12 files changed, 143 insertions(+), 85 deletions(-) rename gopls/internal/hooks/{analysis_115.go => analysis_117.go} (64%) diff --git a/gopls/go.mod b/gopls/go.mod index ed04fd0b7f..3cbb061415 100644 --- a/gopls/go.mod +++ b/gopls/go.mod @@ -9,9 +9,9 @@ require ( github.com/sergi/go-diff v1.1.0 golang.org/x/mod v0.6.0-dev.0.20220106191415-9b9b3d81d5e3 golang.org/x/sys v0.0.0-20220209214540-3681064d5158 - golang.org/x/tools v0.1.9 + golang.org/x/tools v0.1.11-0.20220330174940-8e193c2ba95e golang.org/x/vuln v0.0.0-20220324005316-18fd808f5c7f - honnef.co/go/tools v0.2.2 + honnef.co/go/tools v0.3.0 mvdan.cc/gofumpt v0.3.0 mvdan.cc/xurls/v2 v2.4.0 ) @@ -19,6 +19,7 @@ require ( require ( github.com/BurntSushi/toml v1.0.0 // indirect github.com/google/safehtml v0.0.2 // indirect + golang.org/x/exp/typeparams v0.0.0-20220218215828-6cf2b201936e // indirect golang.org/x/sync v0.0.0-20210220032951-036812b2e83c // indirect golang.org/x/text v0.3.7 // indirect golang.org/x/xerrors v0.0.0-20200804184101-5ec99f83aff1 // indirect diff --git a/gopls/go.sum b/gopls/go.sum index 6900b6c652..8e623bebb5 100644 --- a/gopls/go.sum +++ b/gopls/go.sum @@ -1,4 +1,5 @@ github.com/BurntSushi/toml v0.3.1/go.mod h1:xHWCNGjB5oqiDr8zfno3MHue2Ht5sIBksp03qcyfWMU= +github.com/BurntSushi/toml v0.4.1/go.mod h1:CxXYINrC8qIiEnFrOxCa7Jy5BFHlXnUU2pbicEuybxQ= github.com/BurntSushi/toml v1.0.0 h1:dtDWrepsVPfW9H/4y7dDgFc2MBUSeJhlaDtK13CxFlU= github.com/BurntSushi/toml v1.0.0/go.mod h1:CxXYINrC8qIiEnFrOxCa7Jy5BFHlXnUU2pbicEuybxQ= github.com/client9/misspell v0.3.4 h1:ta993UF76GwbvJcIo3Y68y/M3WxlpEHPWIGDkJYwzJI= @@ -40,6 +41,8 @@ github.com/yuin/goldmark v1.4.1/go.mod h1:mwnBkeHKe2W/ZEtQ+71ViKU8L12m81fl3OWwC1 golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACkg1iLfiJU5Ep61QUkGW8qpdssI0+w= golang.org/x/crypto v0.0.0-20191011191535-87dc89f01550/go.mod h1:yigFU9vqHzYiE8UmvKecakEJjdnWj3jj499lnFckfCI= golang.org/x/crypto v0.0.0-20210921155107-089bfa567519/go.mod h1:GvvjBRRGRdwPK5ydBHafDWAxML/pGHZbMvKqRZ5+Abc= +golang.org/x/exp/typeparams v0.0.0-20220218215828-6cf2b201936e h1:qyrTQ++p1afMkO4DPEeLGq/3oTsdlvdH4vqZUBWzUKM= +golang.org/x/exp/typeparams v0.0.0-20220218215828-6cf2b201936e/go.mod h1:AbB0pIl9nAr9wVwH+Z2ZpaocVmF5I4GyWCDIsVjR0bk= golang.org/x/mod v0.5.1/go.mod h1:5OXOZSfqPIIbmVBIIKWRFfZjPR0E5r58TLhUjH0a2Ro= golang.org/x/mod v0.6.0-dev.0.20211013180041-c96bc1413d57/go.mod h1:3p9vT2HGsQu2K1YbXdKPJLVgG5VJdoTa1poYQBtP1AY= golang.org/x/mod v0.6.0-dev.0.20220106191415-9b9b3d81d5e3 h1:kQgndtyPBW/JIYERgdxfwMYh3AVStj88WQTlNDi2a+o= @@ -77,8 +80,9 @@ gopkg.in/errgo.v2 v2.1.0/go.mod h1:hNsd1EY+bozCKY1Ytp96fpM3vjJbqLJn88ws8XvfDNI= gopkg.in/yaml.v2 v2.2.2/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI= gopkg.in/yaml.v2 v2.2.4 h1:/eiJrUcujPVeJ3xlSWaiNi3uSVmDGBK1pDHUHAnao1I= gopkg.in/yaml.v2 v2.2.4/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI= -honnef.co/go/tools v0.2.2 h1:MNh1AVMyVX23VUHE2O27jm6lNj3vjO5DexS4A1xvnzk= honnef.co/go/tools v0.2.2/go.mod h1:lPVVZ2BS5TfnjLyizF7o7hv7j9/L+8cZY2hLyjP9cGY= +honnef.co/go/tools v0.3.0 h1:2LdYUZ7CIxnYgskbUZfY7FPggmqnh6shBqfWa8Tn3XU= +honnef.co/go/tools v0.3.0/go.mod h1:vlRD9XErLMGT+mDuofSr0mMMquscM/1nQqtRSsh6m70= mvdan.cc/gofumpt v0.3.0 h1:kTojdZo9AcEYbQYhGuLf/zszYthRdhDNDUi2JKTxas4= mvdan.cc/gofumpt v0.3.0/go.mod h1:0+VyGZWleeIj5oostkOex+nDBA0eyavuDnDusAJ8ylo= mvdan.cc/unparam v0.0.0-20211214103731-d0ef000c54e5 h1:Jh3LAeMt1eGpxomyu3jVkmVZWW2MxZ1qIIV2TZ/nRio= diff --git a/gopls/internal/hooks/analysis.go b/gopls/internal/hooks/analysis.go index f1d166b097..51048991d5 100644 --- a/gopls/internal/hooks/analysis.go +++ b/gopls/internal/hooks/analysis.go @@ -2,8 +2,8 @@ // Use of this source code is governed by a BSD-style // license that can be found in the LICENSE file. -//go:build go1.15 -// +build go1.15 +//go:build go1.17 +// +build go1.17 package hooks @@ -18,6 +18,8 @@ import ( ) func updateAnalyzers(options *source.Options) { + options.StaticcheckSupported = true + mapSeverity := func(severity lint.Severity) protocol.DiagnosticSeverity { switch severity { case lint.SeverityError: diff --git a/gopls/internal/hooks/analysis_115.go b/gopls/internal/hooks/analysis_117.go similarity index 64% rename from gopls/internal/hooks/analysis_115.go rename to gopls/internal/hooks/analysis_117.go index 187e522188..02f9170ab6 100644 --- a/gopls/internal/hooks/analysis_115.go +++ b/gopls/internal/hooks/analysis_117.go @@ -2,11 +2,13 @@ // Use of this source code is governed by a BSD-style // license that can be found in the LICENSE file. -//go:build !go1.15 -// +build !go1.15 +//go:build !go1.17 +// +build !go1.17 package hooks import "golang.org/x/tools/internal/lsp/source" -func updateAnalyzers(_ *source.Options) {} +func updateAnalyzers(options *source.Options) { + options.StaticcheckSupported = false +} diff --git a/gopls/internal/hooks/licenses_test.go b/gopls/internal/hooks/licenses_test.go index bed229535a..3b61d348d9 100644 --- a/gopls/internal/hooks/licenses_test.go +++ b/gopls/internal/hooks/licenses_test.go @@ -17,7 +17,7 @@ import ( func TestLicenses(t *testing.T) { // License text differs for older Go versions because staticcheck isn't // supported for those versions. - testenv.NeedsGo1Point(t, 15) + testenv.NeedsGo1Point(t, 17) if runtime.GOOS != "linux" && runtime.GOOS != "darwin" { t.Skip("generating licenses only works on Unixes") diff --git a/gopls/internal/regtest/diagnostics/diagnostics_test.go b/gopls/internal/regtest/diagnostics/diagnostics_test.go index c18dfbf91a..433b462349 100644 --- a/gopls/internal/regtest/diagnostics/diagnostics_test.go +++ b/gopls/internal/regtest/diagnostics/diagnostics_test.go @@ -1265,7 +1265,11 @@ func main() { ` WithOptions( - EditorConfig{EnableStaticcheck: true}, + EditorConfig{ + Settings: map[string]interface{}{ + "staticcheck": true, + }, + }, ).Run(t, files, func(t *testing.T, env *Env) { env.OpenFile("main.go") var d protocol.PublishDiagnosticsParams diff --git a/gopls/internal/regtest/misc/configuration_test.go b/gopls/internal/regtest/misc/configuration_test.go index 17116adaac..d9cce96a43 100644 --- a/gopls/internal/regtest/misc/configuration_test.go +++ b/gopls/internal/regtest/misc/configuration_test.go @@ -16,8 +16,10 @@ import ( // Test that enabling and disabling produces the expected results of showing // and hiding staticcheck analysis results. func TestChangeConfiguration(t *testing.T) { - // Staticcheck only supports Go versions > 1.14. - testenv.NeedsGo1Point(t, 15) + // Staticcheck only supports Go versions >= 1.17. + // Note: keep this in sync with TestStaticcheckWarning. Below this version we + // should get an error when setting staticcheck configuration. + testenv.NeedsGo1Point(t, 17) const files = ` -- go.mod -- @@ -40,10 +42,39 @@ var FooErr = errors.New("foo") ) cfg := &fake.EditorConfig{} *cfg = env.Editor.Config - cfg.EnableStaticcheck = true + cfg.Settings = map[string]interface{}{ + "staticcheck": true, + } env.ChangeConfiguration(t, cfg) env.Await( DiagnosticAt("a/a.go", 5, 4), ) }) } + +func TestStaticcheckWarning(t *testing.T) { + // Note: keep this in sync with TestChangeConfiguration. + testenv.SkipAfterGo1Point(t, 16) + + const files = ` +-- go.mod -- +module mod.com + +go 1.12 +-- a/a.go -- +package a + +import "errors" + +// FooErr should be called ErrFoo (ST1012) +var FooErr = errors.New("foo") +` + + WithOptions(EditorConfig{ + Settings: map[string]interface{}{ + "staticcheck": true, + }, + }).Run(t, files, func(t *testing.T, env *Env) { + env.Await(ShownMessage("staticcheck is not supported")) + }) +} diff --git a/internal/lsp/fake/editor.go b/internal/lsp/fake/editor.go index 5bce5609f8..55592414f3 100644 --- a/internal/lsp/fake/editor.go +++ b/internal/lsp/fake/editor.go @@ -101,9 +101,6 @@ type EditorConfig struct { // To explicitly send no workspace folders, use an empty (non-nil) slice. WorkspaceFolders []string - // EnableStaticcheck enables staticcheck analyzers. - EnableStaticcheck bool - // AllExperiments sets the "allExperiments" configuration, which enables // all of gopls's opt-in settings. AllExperiments bool @@ -258,9 +255,6 @@ func (e *Editor) configuration() map[string]interface{} { if e.Config.SymbolStyle != nil { config["symbolStyle"] = *e.Config.SymbolStyle } - if e.Config.EnableStaticcheck { - config["staticcheck"] = true - } if e.Config.AllExperiments { config["allExperiments"] = true } diff --git a/internal/lsp/general.go b/internal/lsp/general.go index a3662efd0d..1e5063559c 100644 --- a/internal/lsp/general.go +++ b/internal/lsp/general.go @@ -412,35 +412,25 @@ func (s *Server) eventuallyShowMessage(ctx context.Context, msg *protocol.ShowMe func (s *Server) handleOptionResults(ctx context.Context, results source.OptionResults) error { for _, result := range results { - if result.Error != nil { - msg := &protocol.ShowMessageParams{ + var msg *protocol.ShowMessageParams + switch result.Error.(type) { + case nil: + // nothing to do + case *source.SoftError: + msg = &protocol.ShowMessageParams{ + Type: protocol.Warning, + Message: result.Error.Error(), + } + default: + msg = &protocol.ShowMessageParams{ Type: protocol.Error, Message: result.Error.Error(), } - if err := s.eventuallyShowMessage(ctx, msg); err != nil { - return err - } } - switch result.State { - case source.OptionUnexpected: - msg := &protocol.ShowMessageParams{ - Type: protocol.Error, - Message: fmt.Sprintf("unexpected gopls setting %q", result.Name), - } + if msg != nil { if err := s.eventuallyShowMessage(ctx, msg); err != nil { return err } - case source.OptionDeprecated: - msg := fmt.Sprintf("gopls setting %q is deprecated", result.Name) - if result.Replacement != "" { - msg = fmt.Sprintf("%s, use %q instead", msg, result.Replacement) - } - if err := s.eventuallyShowMessage(ctx, &protocol.ShowMessageParams{ - Type: protocol.Warning, - Message: msg, - }); err != nil { - return err - } } } return nil diff --git a/internal/lsp/regtest/expectation.go b/internal/lsp/regtest/expectation.go index 50602ce6b6..35402352c3 100644 --- a/internal/lsp/regtest/expectation.go +++ b/internal/lsp/regtest/expectation.go @@ -152,12 +152,12 @@ func NoShowMessage() SimpleExpectation { } } -// ShownMessage asserts that the editor has received a ShownMessage with the -// given title. -func ShownMessage(title string) SimpleExpectation { +// ShownMessage asserts that the editor has received a ShowMessageRequest +// containing the given substring. +func ShownMessage(containing string) SimpleExpectation { check := func(s State) Verdict { for _, m := range s.showMessage { - if strings.Contains(m.Message, title) { + if strings.Contains(m.Message, containing) { return Met } } diff --git a/internal/lsp/source/options.go b/internal/lsp/source/options.go index 1d85636ca4..592a09841f 100644 --- a/internal/lsp/source/options.go +++ b/internal/lsp/source/options.go @@ -10,6 +10,7 @@ import ( "io" "path/filepath" "regexp" + "runtime" "strings" "sync" "time" @@ -463,10 +464,20 @@ func (u *UserOptions) SetEnvSlice(env []string) { // Hooks contains configuration that is provided to the Gopls command by the // main package. type Hooks struct { + // LicensesText holds third party licenses for software used by gopls. LicensesText string - GoDiff bool + + // TODO(rfindley): is this even necessary? + GoDiff bool + + // Whether staticcheck is supported. + StaticcheckSupported bool + + // ComputeEdits is used to compute edits between file versions. ComputeEdits diff.ComputeEdits - URLRegexp *regexp.Regexp + + // URLRegexp is used to find urls in comments and strings. + URLRegexp *regexp.Regexp // GofumptFormat allows the gopls module to wire-in a call to // gofumpt/format.Source. langVersion and modulePath are used for some @@ -615,9 +626,6 @@ type OptionResult struct { Name string Value interface{} Error error - - State OptionState - Replacement string } type OptionState int @@ -703,11 +711,12 @@ func (o *Options) Clone() *Options { ClientOptions: o.ClientOptions, InternalOptions: o.InternalOptions, Hooks: Hooks{ - GoDiff: o.GoDiff, - ComputeEdits: o.ComputeEdits, - GofumptFormat: o.GofumptFormat, - URLRegexp: o.URLRegexp, - Govulncheck: o.Govulncheck, + GoDiff: o.GoDiff, + StaticcheckSupported: o.StaticcheckSupported, + ComputeEdits: o.ComputeEdits, + GofumptFormat: o.GofumptFormat, + URLRegexp: o.URLRegexp, + Govulncheck: o.Govulncheck, }, ServerOptions: o.ServerOptions, UserOptions: o.UserOptions, @@ -915,12 +924,19 @@ func (o *Options) set(name string, value interface{}, seen map[string]struct{}) // codelens is deprecated, but still works for now. // TODO(rstambler): Remove this for the gopls/v0.7.0 release. if name == "codelens" { - result.State = OptionDeprecated - result.Replacement = "codelenses" + result.deprecated("codelenses") } case "staticcheck": - result.setBool(&o.Staticcheck) + if v, ok := result.asBool(); ok { + o.Staticcheck = v + if v && !o.StaticcheckSupported { + // Warn if the user is trying to enable staticcheck, but staticcheck is + // unsupported. + result.Error = fmt.Errorf("applying setting %q: staticcheck is not supported at %s\n"+ + "\trebuild gopls with a more recent version of Go", result.Name, runtime.Version()) + } + } case "local": result.setString(&o.Local) @@ -946,11 +962,11 @@ func (o *Options) set(name string, value interface{}, seen map[string]struct{}) case "experimentalPostfixCompletions": result.setBool(&o.ExperimentalPostfixCompletions) - case "experimentalWorkspaceModule": + case "experimentalWorkspaceModule": // TODO(rfindley): suggest go.work on go1.18+ result.setBool(&o.ExperimentalWorkspaceModule) - case "experimentalTemplateSupport": // remove after June 2022 - result.State = OptionDeprecated + case "experimentalTemplateSupport": // TODO(pjw): remove after June 2022 + result.deprecated("") case "templateExtensions": if iexts, ok := value.([]interface{}); ok { @@ -968,8 +984,7 @@ func (o *Options) set(name string, value interface{}, seen map[string]struct{}) result.errorf(fmt.Sprintf("unexpected type %T not []string", value)) case "experimentalDiagnosticsDelay", "diagnosticsDelay": if name == "experimentalDiagnosticsDelay" { - result.State = OptionDeprecated - result.Replacement = "diagnosticsDelay" + result.deprecated("diagnosticsDelay") } result.setDuration(&o.DiagnosticsDelay) @@ -994,48 +1009,41 @@ func (o *Options) set(name string, value interface{}, seen map[string]struct{}) // Replaced settings. case "experimentalDisabledAnalyses": - result.State = OptionDeprecated - result.Replacement = "analyses" + result.deprecated("analyses") case "disableDeepCompletion": - result.State = OptionDeprecated - result.Replacement = "deepCompletion" + result.deprecated("deepCompletion") case "disableFuzzyMatching": - result.State = OptionDeprecated - result.Replacement = "fuzzyMatching" + result.deprecated("fuzzyMatching") case "wantCompletionDocumentation": - result.State = OptionDeprecated - result.Replacement = "completionDocumentation" + result.deprecated("completionDocumentation") case "wantUnimportedCompletions": - result.State = OptionDeprecated - result.Replacement = "completeUnimported" + result.deprecated("completeUnimported") case "fuzzyMatching": - result.State = OptionDeprecated - result.Replacement = "matcher" + result.deprecated("matcher") case "caseSensitiveCompletion": - result.State = OptionDeprecated - result.Replacement = "matcher" + result.deprecated("matcher") // Deprecated settings. case "wantSuggestedFixes": - result.State = OptionDeprecated + result.deprecated("") case "noIncrementalSync": - result.State = OptionDeprecated + result.deprecated("") case "watchFileChanges": - result.State = OptionDeprecated + result.deprecated("") case "go-diff": - result.State = OptionDeprecated + result.deprecated("") default: - result.State = OptionUnexpected + result.unexpected() } return result } @@ -1045,6 +1053,27 @@ func (r *OptionResult) errorf(msg string, values ...interface{}) { r.Error = errors.Errorf(prefix+msg, values...) } +// A SoftError is an error that does not affect the functionality of gopls. +type SoftError struct { + msg string +} + +func (e *SoftError) Error() string { + return e.msg +} + +func (r *OptionResult) deprecated(replacement string) { + msg := fmt.Sprintf("gopls setting %q is deprecated", r.Name) + if replacement != "" { + msg = fmt.Sprintf("%s, use %q instead", msg, replacement) + } + r.Error = &SoftError{msg} +} + +func (r *OptionResult) unexpected() { + r.Error = fmt.Errorf("unexpected gopls setting %q", r.Name) +} + func (r *OptionResult) asBool() (bool, bool) { b, ok := r.Value.(bool) if !ok { diff --git a/internal/lsp/source/options_test.go b/internal/lsp/source/options_test.go index f8260c1dd3..dfc464e8c3 100644 --- a/internal/lsp/source/options_test.go +++ b/internal/lsp/source/options_test.go @@ -44,9 +44,10 @@ func TestSetOption(t *testing.T) { check: func(o Options) bool { return o.CompletionBudget == 2*time.Second }, }, { - name: "staticcheck", - value: true, - check: func(o Options) bool { return o.Staticcheck == true }, + name: "staticcheck", + value: true, + check: func(o Options) bool { return o.Staticcheck == true }, + wantError: true, // o.StaticcheckSupported is unset }, { name: "codelenses",