From 588974899118f3738f8530f091d7640c918994d7 Mon Sep 17 00:00:00 2001 From: Ian Cottrell Date: Thu, 10 Oct 2019 20:48:16 -0400 Subject: [PATCH] internal/lsp: use options hooks to install diff driver Change-Id: I2f94c2a68d0036a47ccac3fce07cf9f3b784d443 Reviewed-on: https://go-review.googlesource.com/c/tools/+/200558 Run-TryBot: Ian Cottrell Reviewed-by: Rebecca Stambler --- cmd/gopls/main.go | 2 +- gopls/internal/hooks/analysis.go | 12 +++--- gopls/internal/hooks/hooks.go | 12 +++--- gopls/main.go | 3 +- gopls/test/gopls_test.go | 5 +-- internal/lsp/cache/analysis.go | 65 ----------------------------- internal/lsp/cache/cache.go | 16 +++---- internal/lsp/cache/session.go | 5 ++- internal/lsp/cache/view.go | 7 ---- internal/lsp/cmd/cmd.go | 8 +++- internal/lsp/cmd/cmd_test.go | 4 +- internal/lsp/cmd/test/check.go | 2 +- internal/lsp/cmd/test/cmdtest.go | 4 +- internal/lsp/cmd/test/definition.go | 2 +- internal/lsp/cmd/test/format.go | 2 +- internal/lsp/cmd/test/rename.go | 2 +- internal/lsp/lsp_test.go | 2 +- internal/lsp/source/diagnostics.go | 2 +- internal/lsp/source/options.go | 59 ++++++++++++++++++++++++++ internal/lsp/source/source_test.go | 2 +- internal/lsp/source/view.go | 3 -- 21 files changed, 104 insertions(+), 115 deletions(-) delete mode 100644 internal/lsp/cache/analysis.go diff --git a/cmd/gopls/main.go b/cmd/gopls/main.go index d8ab33705b..6b6e735492 100644 --- a/cmd/gopls/main.go +++ b/cmd/gopls/main.go @@ -19,5 +19,5 @@ import ( func main() { debug.Version += "-cmd.gopls" - tool.Main(context.Background(), cmd.New("gopls-legacy", "", nil), os.Args[1:]) + tool.Main(context.Background(), cmd.New("gopls-legacy", "", nil, nil), os.Args[1:]) } diff --git a/gopls/internal/hooks/analysis.go b/gopls/internal/hooks/analysis.go index ef385a36ad..ca437adf69 100644 --- a/gopls/internal/hooks/analysis.go +++ b/gopls/internal/hooks/analysis.go @@ -5,24 +5,22 @@ 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" "honnef.co/go/tools/stylecheck" ) -func updateAnalyzers(v source.View, analyzers []*analysis.Analyzer) []*analysis.Analyzer { - if v.Options().StaticCheck { +func updateAnalyzers(options *source.Options) { + if options.StaticCheck { for _, a := range simple.Analyzers { - analyzers = append(analyzers, a) + options.Analyzers = append(options.Analyzers, a) } for _, a := range staticcheck.Analyzers { - analyzers = append(analyzers, a) + options.Analyzers = append(options.Analyzers, a) } for _, a := range stylecheck.Analyzers { - analyzers = append(analyzers, a) + options.Analyzers = append(options.Analyzers, a) } } - return analyzers } diff --git a/gopls/internal/hooks/hooks.go b/gopls/internal/hooks/hooks.go index f9127dc167..9037c3b345 100644 --- a/gopls/internal/hooks/hooks.go +++ b/gopls/internal/hooks/hooks.go @@ -8,12 +8,12 @@ package hooks // import "golang.org/x/tools/gopls/internal/hooks" import ( - "context" - - "golang.org/x/tools/internal/lsp/cache" + "golang.org/x/tools/internal/lsp/source" ) -func Install(ctx context.Context) context.Context { - cache.UpdateAnalyzers = updateAnalyzers - return ctx +func Options(options *source.Options) { + if options.GoDiff { + options.ComputeEdits = ComputeEdits + } + updateAnalyzers(options) } diff --git a/gopls/main.go b/gopls/main.go index 31d356ee13..0b4d889918 100644 --- a/gopls/main.go +++ b/gopls/main.go @@ -19,6 +19,5 @@ import ( func main() { ctx := context.Background() - ctx = hooks.Install(ctx) - tool.Main(ctx, cmd.New("gopls", "", nil), os.Args[1:]) + tool.Main(ctx, cmd.New("gopls", "", nil, hooks.Options), os.Args[1:]) } diff --git a/gopls/test/gopls_test.go b/gopls/test/gopls_test.go index 5245579277..15ecfa4cbe 100644 --- a/gopls/test/gopls_test.go +++ b/gopls/test/gopls_test.go @@ -5,7 +5,6 @@ package gopls_test import ( - "context" "os" "testing" @@ -30,9 +29,7 @@ func testCommandLine(t *testing.T, exporter packagestest.Exporter) { if stat, err := os.Stat(testdata); err != nil || !stat.IsDir() { t.Skip("testdata directory not present") } - ctx := context.Background() - hooks.Install(ctx) data := tests.Load(t, exporter, testdata) defer data.Exported.Cleanup() - tests.Run(t, cmdtest.NewRunner(exporter, data, tests.Context(t)), data) + tests.Run(t, cmdtest.NewRunner(exporter, data, tests.Context(t), hooks.Options), data) } diff --git a/internal/lsp/cache/analysis.go b/internal/lsp/cache/analysis.go deleted file mode 100644 index 21a294fedc..0000000000 --- a/internal/lsp/cache/analysis.go +++ /dev/null @@ -1,65 +0,0 @@ -// Copyright 2019 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 cache - -import ( - "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" - "golang.org/x/tools/go/analysis/passes/atomicalign" - "golang.org/x/tools/go/analysis/passes/bools" - "golang.org/x/tools/go/analysis/passes/buildtag" - "golang.org/x/tools/go/analysis/passes/cgocall" - "golang.org/x/tools/go/analysis/passes/composite" - "golang.org/x/tools/go/analysis/passes/copylock" - "golang.org/x/tools/go/analysis/passes/httpresponse" - "golang.org/x/tools/go/analysis/passes/loopclosure" - "golang.org/x/tools/go/analysis/passes/lostcancel" - "golang.org/x/tools/go/analysis/passes/nilfunc" - "golang.org/x/tools/go/analysis/passes/printf" - "golang.org/x/tools/go/analysis/passes/shift" - "golang.org/x/tools/go/analysis/passes/sortslice" - "golang.org/x/tools/go/analysis/passes/stdmethods" - "golang.org/x/tools/go/analysis/passes/structtag" - "golang.org/x/tools/go/analysis/passes/tests" - "golang.org/x/tools/go/analysis/passes/unmarshal" - "golang.org/x/tools/go/analysis/passes/unreachable" - "golang.org/x/tools/go/analysis/passes/unsafeptr" - "golang.org/x/tools/go/analysis/passes/unusedresult" - "golang.org/x/tools/internal/lsp/source" -) - -var defaultAnalyzers = []*analysis.Analyzer{ - // The traditional vet suite: - asmdecl.Analyzer, - assign.Analyzer, - atomic.Analyzer, - atomicalign.Analyzer, - bools.Analyzer, - buildtag.Analyzer, - cgocall.Analyzer, - composite.Analyzer, - copylock.Analyzer, - httpresponse.Analyzer, - loopclosure.Analyzer, - lostcancel.Analyzer, - nilfunc.Analyzer, - printf.Analyzer, - shift.Analyzer, - stdmethods.Analyzer, - structtag.Analyzer, - tests.Analyzer, - unmarshal.Analyzer, - unreachable.Analyzer, - unsafeptr.Analyzer, - unusedresult.Analyzer, - // Non-vet analyzers - sortslice.Analyzer, -} - -var UpdateAnalyzers = func(v source.View, analyzers []*analysis.Analyzer) []*analysis.Analyzer { - return analyzers -} diff --git a/internal/lsp/cache/cache.go b/internal/lsp/cache/cache.go index 764d077add..e3a1396954 100644 --- a/internal/lsp/cache/cache.go +++ b/internal/lsp/cache/cache.go @@ -18,21 +18,23 @@ import ( "golang.org/x/tools/internal/span" ) -func New() source.Cache { +func New(options func(*source.Options)) source.Cache { index := atomic.AddInt64(&cacheIndex, 1) c := &cache{ - fs: &nativeFileSystem{}, - id: strconv.FormatInt(index, 10), - fset: token.NewFileSet(), + fs: &nativeFileSystem{}, + id: strconv.FormatInt(index, 10), + fset: token.NewFileSet(), + options: options, } debug.AddCache(debugCache{c}) return c } type cache struct { - fs source.FileSystem - id string - fset *token.FileSet + fs source.FileSystem + id string + fset *token.FileSet + options func(*source.Options) store memoize.Store } diff --git a/internal/lsp/cache/session.go b/internal/lsp/cache/session.go index 5a41423fdc..805e8b5748 100644 --- a/internal/lsp/cache/session.go +++ b/internal/lsp/cache/session.go @@ -110,7 +110,10 @@ func (s *session) NewView(ctx context.Context, name string, folder span.URI, opt } v.snapshot.view = v - v.analyzers = UpdateAnalyzers(v, defaultAnalyzers) + if v.session.cache.options != nil { + v.session.cache.options(&v.options) + } + // Preemptively build the builtin package, // so we immediately add builtin.go to the list of ignored files. v.buildBuiltinPackage(ctx) diff --git a/internal/lsp/cache/view.go b/internal/lsp/cache/view.go index c840a8182b..436ae3a535 100644 --- a/internal/lsp/cache/view.go +++ b/internal/lsp/cache/view.go @@ -15,7 +15,6 @@ import ( "strings" "sync" - "golang.org/x/tools/go/analysis" "golang.org/x/tools/go/packages" "golang.org/x/tools/internal/imports" "golang.org/x/tools/internal/lsp/debug" @@ -80,8 +79,6 @@ type view struct { // ignoredURIs is the set of URIs of files that we ignore. ignoredURIsMu sync.Mutex ignoredURIs map[span.URI]struct{} - - analyzers []*analysis.Analyzer } func (v *view) Session() source.Session { @@ -396,10 +393,6 @@ func (v *view) findFile(uri span.URI) (viewFile, error) { return nil, nil } -func (v *view) Analyzers() []*analysis.Analyzer { - return v.analyzers -} - func (f *fileBase) addURI(uri span.URI) int { f.uris = append(f.uris, uri) return len(f.uris) diff --git a/internal/lsp/cmd/cmd.go b/internal/lsp/cmd/cmd.go index eff20ab351..d74a5af6eb 100644 --- a/internal/lsp/cmd/cmd.go +++ b/internal/lsp/cmd/cmd.go @@ -65,15 +65,19 @@ type Application struct { // Control ocagent export of telemetry OCAgent string `flag:"ocagent" help:"The address of the ocagent, or off"` + + // PrepareOptions is called to update the options when a new view is built. + // It is primarily to allow the behavior of gopls to be modified by hooks. + PrepareOptions func(*source.Options) } // Returns a new Application ready to run. -func New(name, wd string, env []string) *Application { +func New(name, wd string, env []string, options func(*source.Options)) *Application { if wd == "" { wd, _ = os.Getwd() } app := &Application{ - cache: cache.New(), + cache: cache.New(options), name: name, wd: wd, env: env, diff --git a/internal/lsp/cmd/cmd_test.go b/internal/lsp/cmd/cmd_test.go index 1440459779..af04e8dc5a 100644 --- a/internal/lsp/cmd/cmd_test.go +++ b/internal/lsp/cmd/cmd_test.go @@ -32,7 +32,7 @@ func TestCommandLine(t *testing.T) { func testCommandLine(t *testing.T, exporter packagestest.Exporter) { data := tests.Load(t, exporter, "../testdata") defer data.Exported.Cleanup() - tests.Run(t, cmdtest.NewRunner(exporter, data, tests.Context(t)), data) + tests.Run(t, cmdtest.NewRunner(exporter, data, tests.Context(t), nil), data) } func TestDefinitionHelpExample(t *testing.T) { @@ -54,7 +54,7 @@ func TestDefinitionHelpExample(t *testing.T) { fmt.Sprintf("%v:#%v", thisFile, cmd.ExampleOffset)} { args := append(baseArgs, query) got := cmdtest.CaptureStdOut(t, func() { - _ = tool.Run(tests.Context(t), cmd.New("gopls-test", "", nil), args) + _ = tool.Run(tests.Context(t), cmd.New("gopls-test", "", nil, nil), args) }) if !expect.MatchString(got) { t.Errorf("test with %v\nexpected:\n%s\ngot:\n%s", args, expect, got) diff --git a/internal/lsp/cmd/test/check.go b/internal/lsp/cmd/test/check.go index 39dd3fb58c..c368998753 100644 --- a/internal/lsp/cmd/test/check.go +++ b/internal/lsp/cmd/test/check.go @@ -22,7 +22,7 @@ func (r *runner) Diagnostics(t *testing.T, uri span.URI, want []source.Diagnosti } fname := uri.Filename() args := []string{"-remote=internal", "check", fname} - app := cmd.New("gopls-test", r.data.Config.Dir, r.data.Exported.Config.Env) + app := cmd.New("gopls-test", r.data.Config.Dir, r.data.Exported.Config.Env, r.options) out := CaptureStdOut(t, func() { _ = tool.Run(r.ctx, app, args) }) diff --git a/internal/lsp/cmd/test/cmdtest.go b/internal/lsp/cmd/test/cmdtest.go index 13607f2a67..8aa8638f6b 100644 --- a/internal/lsp/cmd/test/cmdtest.go +++ b/internal/lsp/cmd/test/cmdtest.go @@ -26,13 +26,15 @@ type runner struct { exporter packagestest.Exporter data *tests.Data ctx context.Context + options func(*source.Options) } -func NewRunner(exporter packagestest.Exporter, data *tests.Data, ctx context.Context) tests.Tests { +func NewRunner(exporter packagestest.Exporter, data *tests.Data, ctx context.Context, options func(*source.Options)) tests.Tests { return &runner{ exporter: exporter, data: data, ctx: ctx, + options: options, } } diff --git a/internal/lsp/cmd/test/definition.go b/internal/lsp/cmd/test/definition.go index 6bbcdd15be..e91b51193b 100644 --- a/internal/lsp/cmd/test/definition.go +++ b/internal/lsp/cmd/test/definition.go @@ -54,7 +54,7 @@ func (r *runner) Definition(t *testing.T, spn span.Span, d tests.Definition) { uri := d.Src.URI() args = append(args, fmt.Sprint(d.Src)) got := CaptureStdOut(t, func() { - app := cmd.New("gopls-test", r.data.Config.Dir, r.data.Exported.Config.Env) + app := cmd.New("gopls-test", r.data.Config.Dir, r.data.Exported.Config.Env, r.options) _ = tool.Run(r.ctx, app, args) }) got = normalizePaths(r.data, got) diff --git a/internal/lsp/cmd/test/format.go b/internal/lsp/cmd/test/format.go index 0418b3104e..355f0d9cf7 100644 --- a/internal/lsp/cmd/test/format.go +++ b/internal/lsp/cmd/test/format.go @@ -33,7 +33,7 @@ func (r *runner) Format(t *testing.T, spn span.Span) { //TODO: our error handling differs, for now just skip unformattable files t.Skip("Unformattable file") } - app := cmd.New("gopls-test", r.data.Config.Dir, r.data.Config.Env) + app := cmd.New("gopls-test", r.data.Config.Dir, r.data.Config.Env, r.options) got := CaptureStdOut(t, func() { _ = tool.Run(r.ctx, app, append([]string{"-remote=internal", "format"}, filename)) }) diff --git a/internal/lsp/cmd/test/rename.go b/internal/lsp/cmd/test/rename.go index c5297b7683..9b227b9bd9 100644 --- a/internal/lsp/cmd/test/rename.go +++ b/internal/lsp/cmd/test/rename.go @@ -16,7 +16,7 @@ import ( func (r *runner) Rename(t *testing.T, spn span.Span, newText string) { filename := spn.URI().Filename() goldenTag := newText + "-rename" - app := cmd.New("gopls-test", r.data.Config.Dir, r.data.Config.Env) + app := cmd.New("gopls-test", r.data.Config.Dir, r.data.Config.Env, r.options) loc := fmt.Sprintf("%v", spn) var err error got := CaptureStdOut(t, func() { diff --git a/internal/lsp/lsp_test.go b/internal/lsp/lsp_test.go index 4b2856cf65..283d4588e9 100644 --- a/internal/lsp/lsp_test.go +++ b/internal/lsp/lsp_test.go @@ -48,7 +48,7 @@ func testLSP(t *testing.T, exporter packagestest.Exporter) { data := tests.Load(t, exporter, "testdata") defer data.Exported.Cleanup() - cache := cache.New() + cache := cache.New(nil) session := cache.NewSession(ctx) options := tests.DefaultOptions() session.SetOptions(options) diff --git a/internal/lsp/source/diagnostics.go b/internal/lsp/source/diagnostics.go index 04c4737a6b..a528bac4bd 100644 --- a/internal/lsp/source/diagnostics.go +++ b/internal/lsp/source/diagnostics.go @@ -305,7 +305,7 @@ func singleDiagnostic(uri span.URI, format string, a ...interface{}) map[span.UR func runAnalyses(ctx context.Context, view View, cph CheckPackageHandle, disabledAnalyses map[string]struct{}, report func(a *analysis.Analyzer, diag analysis.Diagnostic) error) error { var analyzers []*analysis.Analyzer - for _, a := range view.Analyzers() { + for _, a := range view.Options().Analyzers { if _, ok := disabledAnalyses[a.Name]; ok { continue } diff --git a/internal/lsp/source/options.go b/internal/lsp/source/options.go index c89967df43..e1bc782b90 100644 --- a/internal/lsp/source/options.go +++ b/internal/lsp/source/options.go @@ -9,6 +9,30 @@ import ( "os" "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" + "golang.org/x/tools/go/analysis/passes/atomicalign" + "golang.org/x/tools/go/analysis/passes/bools" + "golang.org/x/tools/go/analysis/passes/buildtag" + "golang.org/x/tools/go/analysis/passes/cgocall" + "golang.org/x/tools/go/analysis/passes/composite" + "golang.org/x/tools/go/analysis/passes/copylock" + "golang.org/x/tools/go/analysis/passes/httpresponse" + "golang.org/x/tools/go/analysis/passes/loopclosure" + "golang.org/x/tools/go/analysis/passes/lostcancel" + "golang.org/x/tools/go/analysis/passes/nilfunc" + "golang.org/x/tools/go/analysis/passes/printf" + "golang.org/x/tools/go/analysis/passes/shift" + "golang.org/x/tools/go/analysis/passes/sortslice" + "golang.org/x/tools/go/analysis/passes/stdmethods" + "golang.org/x/tools/go/analysis/passes/structtag" + "golang.org/x/tools/go/analysis/passes/tests" + "golang.org/x/tools/go/analysis/passes/unmarshal" + "golang.org/x/tools/go/analysis/passes/unreachable" + "golang.org/x/tools/go/analysis/passes/unsafeptr" + "golang.org/x/tools/go/analysis/passes/unusedresult" "golang.org/x/tools/internal/lsp/diff" "golang.org/x/tools/internal/lsp/diff/myers" "golang.org/x/tools/internal/lsp/protocol" @@ -43,6 +67,7 @@ var ( Budget: 100 * time.Millisecond, }, ComputeEdits: myers.ComputeEdits, + Analyzers: defaultAnalyzers, } ) @@ -57,6 +82,7 @@ type Options struct { DisabledAnalyses map[string]struct{} StaticCheck bool + GoDiff bool WatchFileChanges bool InsertTextFormat protocol.InsertTextFormat @@ -76,6 +102,8 @@ type Options struct { Completion CompletionOptions ComputeEdits diff.ComputeEdits + + Analyzers []*analysis.Analyzer } type CompletionOptions struct { @@ -246,6 +274,9 @@ func (o *Options) set(name string, value interface{}) OptionResult { case "staticcheck": result.setBool(&o.StaticCheck) + case "go-diff": + result.setBool(&o.GoDiff) + // Deprecated settings. case "wantSuggestedFixes": result.State = OptionDeprecated @@ -290,3 +321,31 @@ func (r *OptionResult) setBool(b *bool) { *b = v } } + +var defaultAnalyzers = []*analysis.Analyzer{ + // The traditional vet suite: + asmdecl.Analyzer, + assign.Analyzer, + atomic.Analyzer, + atomicalign.Analyzer, + bools.Analyzer, + buildtag.Analyzer, + cgocall.Analyzer, + composite.Analyzer, + copylock.Analyzer, + httpresponse.Analyzer, + loopclosure.Analyzer, + lostcancel.Analyzer, + nilfunc.Analyzer, + printf.Analyzer, + shift.Analyzer, + stdmethods.Analyzer, + structtag.Analyzer, + tests.Analyzer, + unmarshal.Analyzer, + unreachable.Analyzer, + unsafeptr.Analyzer, + unusedresult.Analyzer, + // Non-vet analyzers + sortslice.Analyzer, +} diff --git a/internal/lsp/source/source_test.go b/internal/lsp/source/source_test.go index 360c019067..933dcb41f1 100644 --- a/internal/lsp/source/source_test.go +++ b/internal/lsp/source/source_test.go @@ -48,7 +48,7 @@ func testSource(t *testing.T, exporter packagestest.Exporter) { data := tests.Load(t, exporter, "../testdata") defer data.Exported.Cleanup() - cache := cache.New() + cache := cache.New(nil) session := cache.NewSession(ctx) options := tests.DefaultOptions() options.Env = data.Config.Env diff --git a/internal/lsp/source/view.go b/internal/lsp/source/view.go index fa85bc0d15..d23d7cb537 100644 --- a/internal/lsp/source/view.go +++ b/internal/lsp/source/view.go @@ -240,9 +240,6 @@ type View interface { // This function does not correctly invalidate the view when needed. SetOptions(Options) - // Analyzers returns the set of Analyzers active for this view. - Analyzers() []*analysis.Analyzer - // CheckPackageHandles returns the CheckPackageHandles for the packages // that this file belongs to. CheckPackageHandles(ctx context.Context, f File) (Snapshot, []CheckPackageHandle, error)