From e31b568ad139c49e045c12e6cae763da7512e902 Mon Sep 17 00:00:00 2001 From: Rebecca Stambler Date: Fri, 12 Jun 2020 17:10:06 -0400 Subject: [PATCH] internal/lsp: plumb fillstruct through analysis Now that fillstruct is an analyzer, we can simplify the code that calls it in code_action.go. We introduce a new class of analyzer -- convenience analyzers, which are closer to commands. These represent suggestions that won't necessarily improve the quality or correctness of your code, but they offer small helper functions for the user. This CL also combines the refactor rewrite tests with the suggested fix tests, since they are effectively the same. For now, we only support convenience analyzers when a code action was requested on the same line as the fix. I'm not sure how to otherwise handle this without bothering the user with unnecessary diagnostics. Change-Id: I7f0aa198b5ee9964a907d709bae6380093d4ef21 Reviewed-on: https://go-review.googlesource.com/c/tools/+/237687 Run-TryBot: Rebecca Stambler Reviewed-by: Heschi Kreinick TryBot-Result: Gobot Gobot --- .../lsp/analysis/fillstruct/fillstruct.go | 102 +++++------ internal/lsp/cmd/test/refactor_rewrite.go | 9 - internal/lsp/cmd/test/suggested_fix.go | 7 +- internal/lsp/code_action.go | 79 +++++++-- internal/lsp/lsp_test.go | 65 ++----- internal/lsp/source/diagnostics.go | 41 ++++- internal/lsp/source/fill_struct.go | 143 ---------------- internal/lsp/source/options.go | 2 +- internal/lsp/source/source_test.go | 1 - .../lsp/testdata/indirect/summary.txt.golden | 1 - .../lsp/primarymod/fillstruct/fill_struct.go | 46 ++--- .../fillstruct/fill_struct.go.golden | 158 +++++++++--------- .../fillstruct/fill_struct_nested.go | 30 ++-- .../fillstruct/fill_struct_nested.go.golden | 36 ++-- .../fillstruct/fill_struct_package.go | 18 +- .../fillstruct/fill_struct_package.go.golden | 24 +-- .../fillstruct/fill_struct_spaces.go | 9 + .../fillstruct/fill_struct_spaces.go.golden | 24 +-- internal/lsp/testdata/lsp/summary.txt.golden | 3 +- .../testdata/missingdep/summary.txt.golden | 1 - .../testdata/missingtwodep/summary.txt.golden | 1 - .../lsp/testdata/unused/summary.txt.golden | 1 - .../testdata/upgradedep/summary.txt.golden | 1 - internal/lsp/tests/tests.go | 23 +-- 24 files changed, 347 insertions(+), 478 deletions(-) delete mode 100644 internal/lsp/cmd/test/refactor_rewrite.go delete mode 100644 internal/lsp/source/fill_struct.go create mode 100644 internal/lsp/testdata/lsp/primarymod/fillstruct/fill_struct_spaces.go diff --git a/internal/lsp/analysis/fillstruct/fillstruct.go b/internal/lsp/analysis/fillstruct/fillstruct.go index b23a05f9cf..6ec8a35a49 100644 --- a/internal/lsp/analysis/fillstruct/fillstruct.go +++ b/internal/lsp/analysis/fillstruct/fillstruct.go @@ -8,6 +8,7 @@ package fillstruct import ( "bytes" + "fmt" "go/ast" "go/format" "go/types" @@ -53,12 +54,10 @@ func run(pass *analysis.Pass) (interface{}, error) { return } expr := n.(*ast.CompositeLit) - // TODO: Handle partially-filled structs as well. if len(expr.Elts) != 0 { return } - var file *ast.File for _, f := range pass.Files { if f.Pos() <= expr.Pos() && expr.Pos() <= f.End() { @@ -85,61 +84,62 @@ func run(pass *analysis.Pass) (interface{}, error) { } typ = typ.Underlying() - if typ == nil { + obj, ok := typ.(*types.Struct) + if !ok { + return + } + fieldCount := obj.NumFields() + if fieldCount == 0 { + return + } + var fieldSourceCode strings.Builder + for i := 0; i < fieldCount; i++ { + field := obj.Field(i) + // Ignore fields that are not accessible in the current package. + if field.Pkg() != nil && field.Pkg() != pass.Pkg && !field.Exported() { + continue + } + + label := field.Name() + value := analysisinternal.ZeroValue(pass.Fset, file, pass.Pkg, field.Type()) + if value == nil { + continue + } + var valBuf bytes.Buffer + if err := format.Node(&valBuf, pass.Fset, value); err != nil { + return + } + fieldSourceCode.WriteString("\n") + fieldSourceCode.WriteString(label) + fieldSourceCode.WriteString(" : ") + fieldSourceCode.WriteString(valBuf.String()) + fieldSourceCode.WriteString(",") + } + + if fieldSourceCode.Len() == 0 { return } - switch obj := typ.(type) { - case *types.Struct: - fieldCount := obj.NumFields() - if fieldCount == 0 { - return - } - var fieldSourceCode strings.Builder - for i := 0; i < fieldCount; i++ { - field := obj.Field(i) - // Ignore fields that are not accessible in the current package. - if field.Pkg() != nil && field.Pkg() != pass.Pkg && !field.Exported() { - continue - } + fieldSourceCode.WriteString("\n") - label := field.Name() - value := analysisinternal.ZeroValue(pass.Fset, file, pass.Pkg, field.Type()) - if value == nil { - continue - } - var valBuf bytes.Buffer - if err := format.Node(&valBuf, pass.Fset, value); err != nil { - return - } - fieldSourceCode.WriteString("\n") - fieldSourceCode.WriteString(label) - fieldSourceCode.WriteString(" : ") - fieldSourceCode.WriteString(valBuf.String()) - fieldSourceCode.WriteString(",") - } + buf := []byte(fieldSourceCode.String()) - if fieldSourceCode.Len() == 0 { - return - } - - fieldSourceCode.WriteString("\n") - - buf := []byte(fieldSourceCode.String()) - - pass.Report(analysis.Diagnostic{ - Pos: expr.Lbrace, - End: expr.Rbrace, - SuggestedFixes: []analysis.SuggestedFix{{ - Message: "Fill struct with empty values", - TextEdits: []analysis.TextEdit{{ - Pos: expr.Lbrace + 1, - End: expr.Rbrace, - NewText: buf, - }}, - }}, - }) + msg := "Fill struct with default values" + if name, ok := expr.Type.(*ast.Ident); ok { + msg = fmt.Sprintf("Fill %s with default values", name) } + pass.Report(analysis.Diagnostic{ + Pos: expr.Lbrace, + End: expr.Rbrace, + SuggestedFixes: []analysis.SuggestedFix{{ + Message: msg, + TextEdits: []analysis.TextEdit{{ + Pos: expr.Lbrace + 1, + End: expr.Rbrace, + NewText: buf, + }}, + }}, + }) }) return nil, nil } diff --git a/internal/lsp/cmd/test/refactor_rewrite.go b/internal/lsp/cmd/test/refactor_rewrite.go deleted file mode 100644 index 9d7a1d734d..0000000000 --- a/internal/lsp/cmd/test/refactor_rewrite.go +++ /dev/null @@ -1,9 +0,0 @@ -package cmdtest - -import ( - "testing" - - "golang.org/x/tools/internal/span" -) - -func (r *runner) RefactorRewrite(t *testing.T, spn span.Span, title string) {} diff --git a/internal/lsp/cmd/test/suggested_fix.go b/internal/lsp/cmd/test/suggested_fix.go index 0419e33acd..f1a478a30c 100644 --- a/internal/lsp/cmd/test/suggested_fix.go +++ b/internal/lsp/cmd/test/suggested_fix.go @@ -16,12 +16,17 @@ func (r *runner) SuggestedFix(t *testing.T, spn span.Span, actionKinds []string) uri := spn.URI() filename := uri.Filename() args := []string{"fix", "-a", fmt.Sprintf("%s", spn)} + for _, kind := range actionKinds { + if kind == "refactor.rewrite" { + t.Skip("refactor.rewrite is not yet supported on the command line") + } + } args = append(args, actionKinds...) got, _ := r.NormalizeGoplsCmd(t, args...) want := string(r.data.Golden("suggestedfix_"+tests.SpanName(spn), filename, func() ([]byte, error) { return []byte(got), nil })) if want != got { - t.Errorf("suggested fixes failed for %s, expected:\n%v\ngot:\n%v", filename, want, got) + t.Errorf("suggested fixes failed for %s: %s", filename, tests.Diff(want, got)) } } diff --git a/internal/lsp/code_action.go b/internal/lsp/code_action.go index 178b9b39fc..f611fc9022 100644 --- a/internal/lsp/code_action.go +++ b/internal/lsp/code_action.go @@ -11,10 +11,12 @@ import ( "sort" "strings" + "golang.org/x/tools/go/analysis" "golang.org/x/tools/internal/imports" "golang.org/x/tools/internal/lsp/mod" "golang.org/x/tools/internal/lsp/protocol" "golang.org/x/tools/internal/lsp/source" + "golang.org/x/tools/internal/span" ) func (s *Server) codeAction(ctx context.Context, params *protocol.CodeActionParams) ([]protocol.CodeAction, error) { @@ -111,13 +113,19 @@ func (s *Server) codeAction(ctx context.Context, params *protocol.CodeActionPara }) } } - // Check for context cancellation before processing analysis fixes. if ctx.Err() != nil { return nil, ctx.Err() } - // Retrieve any necessary analysis fixes or edits. + phs, err := snapshot.PackageHandles(ctx, fh) + if err != nil { + return nil, err + } + ph, err := source.WidestPackageHandle(phs) + if err != nil { + return nil, err + } if (wanted[protocol.QuickFix] || wanted[protocol.SourceFixAll]) && len(diagnostics) > 0 { - analysisQuickFixes, highConfidenceEdits, err := analysisFixes(ctx, snapshot, fh, diagnostics) + analysisQuickFixes, highConfidenceEdits, err := analysisFixes(ctx, snapshot, ph, diagnostics) if err != nil { return nil, err } @@ -143,11 +151,17 @@ func (s *Server) codeAction(ctx context.Context, params *protocol.CodeActionPara }) } } - fillActions, err := source.FillStruct(ctx, snapshot, fh, params.Range) - if err != nil { - return nil, err + if ctx.Err() != nil { + return nil, ctx.Err() + } + // Add any suggestions that do not necessarily fix any diagnostics. + if wanted[protocol.RefactorRewrite] { + fixes, err := convenienceFixes(ctx, snapshot, ph, uri, params.Range) + if err != nil { + return nil, err + } + codeActions = append(codeActions, fixes...) } - codeActions = append(codeActions, fillActions...) default: // Unsupported file kind for a code action. return nil, nil @@ -254,7 +268,7 @@ func importDiagnostics(fix *imports.ImportFix, diagnostics []protocol.Diagnostic return results } -func analysisFixes(ctx context.Context, snapshot source.Snapshot, fh source.FileHandle, diagnostics []protocol.Diagnostic) ([]protocol.CodeAction, []protocol.TextDocumentEdit, error) { +func analysisFixes(ctx context.Context, snapshot source.Snapshot, ph source.PackageHandle, diagnostics []protocol.Diagnostic) ([]protocol.CodeAction, []protocol.TextDocumentEdit, error) { if len(diagnostics) == 0 { return nil, nil, nil } @@ -262,16 +276,6 @@ func analysisFixes(ctx context.Context, snapshot source.Snapshot, fh source.File var codeActions []protocol.CodeAction var sourceFixAllEdits []protocol.TextDocumentEdit - phs, err := snapshot.PackageHandles(ctx, fh) - if err != nil { - return nil, nil, err - } - // We get the package that source.Diagnostics would've used. This is hack. - // TODO(golang/go#32443): The correct solution will be to cache diagnostics per-file per-snapshot. - ph, err := source.WidestPackageHandle(phs) - if err != nil { - return nil, nil, err - } for _, diag := range diagnostics { srcErr, analyzer, ok := findSourceError(ctx, snapshot, ph.ID(), diag) if !ok { @@ -342,6 +346,45 @@ func findSourceError(ctx context.Context, snapshot source.Snapshot, pkgID string return nil, source.Analyzer{}, false } +func convenienceFixes(ctx context.Context, snapshot source.Snapshot, ph source.PackageHandle, uri span.URI, rng protocol.Range) ([]protocol.CodeAction, error) { + var analyzers []*analysis.Analyzer + for _, a := range snapshot.View().Options().ConvenienceAnalyzers { + analyzers = append(analyzers, a.Analyzer) + } + diagnostics, err := snapshot.Analyze(ctx, ph.ID(), analyzers...) + if err != nil { + return nil, err + } + var codeActions []protocol.CodeAction + for _, d := range diagnostics { + // For now, only show diagnostics for matching lines. Maybe we should + // alter this behavior in the future, depending on the user experience. + if d.URI != uri { + continue + } + if d.Range.Start.Line != rng.Start.Line { + continue + } + for _, fix := range d.SuggestedFixes { + action := protocol.CodeAction{ + Title: fix.Title, + Kind: protocol.RefactorRewrite, + Edit: protocol.WorkspaceEdit{}, + } + for uri, edits := range fix.Edits { + fh, err := snapshot.GetFile(ctx, uri) + if err != nil { + return nil, err + } + docChanges := documentChanges(fh, edits) + action.Edit.DocumentChanges = append(action.Edit.DocumentChanges, docChanges...) + } + codeActions = append(codeActions, action) + } + } + return codeActions, nil +} + func documentChanges(fh source.FileHandle, edits []protocol.TextEdit) []protocol.TextDocumentEdit { return []protocol.TextDocumentEdit{ { diff --git a/internal/lsp/lsp_test.go b/internal/lsp/lsp_test.go index ff31a10ec8..cf0b4c5242 100644 --- a/internal/lsp/lsp_test.go +++ b/internal/lsp/lsp_test.go @@ -370,48 +370,6 @@ func (r *runner) Import(t *testing.T, spn span.Span) { } } -func (r *runner) RefactorRewrite(t *testing.T, spn span.Span, title string) { - uri := spn.URI() - m, err := r.data.Mapper(uri) - if err != nil { - t.Fatal(err) - } - rng, err := m.Range(spn) - if err != nil { - t.Fatal(err) - } - actions, err := r.server.CodeAction(r.ctx, &protocol.CodeActionParams{ - TextDocument: protocol.TextDocumentIdentifier{ - URI: protocol.URIFromSpanURI(uri), - }, - Range: rng, - }) - if err != nil { - t.Fatal(err) - } - for _, action := range actions { - // There may be more code actions available at spn (Span), - // we only need the one specified in the title - if action.Kind != protocol.RefactorRewrite || action.Title != title { - continue - } - res, err := applyWorkspaceEdits(r, action.Edit) - if err != nil { - t.Fatal(err) - } - for u, got := range res { - fixed := string(r.data.Golden(tests.SpanName(spn), u.Filename(), func() ([]byte, error) { - return []byte(got), nil - })) - if fixed != got { - t.Errorf("%s failed for %s, expected:\n%#v\ngot:\n%#v", title, u.Filename(), fixed, got) - } - } - return - } - t.Fatalf("expected code action %q, but got none", title) -} - func (r *runner) SuggestedFix(t *testing.T, spn span.Span, actionKinds []string) { uri := spn.URI() view, err := r.server.session.ViewOf(uri) @@ -435,19 +393,16 @@ func (r *runner) SuggestedFix(t *testing.T, spn span.Span, actionKinds []string) r.diagnostics[key.id.URI] = diags } } - var diag *source.Diagnostic + var diagnostics []protocol.Diagnostic for _, d := range r.diagnostics[uri] { // Compare the start positions rather than the entire range because // some diagnostics have a range with the same start and end position (8:1-8:1). // The current marker functionality prevents us from having a range of 0 length. if protocol.ComparePosition(d.Range.Start, rng.Start) == 0 { - diag = d + diagnostics = append(diagnostics, toProtocolDiagnostics([]*source.Diagnostic{d})...) break } } - if diag == nil { - t.Fatalf("could not get any suggested fixes for %v", spn) - } codeActionKinds := []protocol.CodeActionKind{} for _, k := range actionKinds { codeActionKinds = append(codeActionKinds, protocol.CodeActionKind(k)) @@ -456,28 +411,30 @@ func (r *runner) SuggestedFix(t *testing.T, spn span.Span, actionKinds []string) TextDocument: protocol.TextDocumentIdentifier{ URI: protocol.URIFromSpanURI(uri), }, + Range: rng, Context: protocol.CodeActionContext{ Only: codeActionKinds, - Diagnostics: toProtocolDiagnostics([]*source.Diagnostic{diag}), + Diagnostics: diagnostics, }, }) if err != nil { t.Fatal(err) } - // TODO: This test should probably be able to handle multiple code actions. - if len(actions) == 0 { - t.Fatal("no code actions returned") + // Hack: We assume that we only get one code action per range. + // TODO(rstambler): Support multiple code actions per test. + if len(actions) == 0 || len(actions) > 1 { + t.Fatalf("unexpected number of code actions, want 1, got %v", len(actions)) } res, err := applyWorkspaceEdits(r, actions[0].Edit) if err != nil { t.Fatal(err) } for u, got := range res { - fixed := string(r.data.Golden("suggestedfix_"+tests.SpanName(spn), u.Filename(), func() ([]byte, error) { + want := string(r.data.Golden("suggestedfix_"+tests.SpanName(spn), u.Filename(), func() ([]byte, error) { return []byte(got), nil })) - if fixed != got { - t.Errorf("suggested fixes failed for %s, expected:\n%#v\ngot:\n%#v", u.Filename(), fixed, got) + if want != got { + t.Errorf("suggested fixes failed for %s: %s", u.Filename(), tests.Diff(want, got)) } } } diff --git a/internal/lsp/source/diagnostics.go b/internal/lsp/source/diagnostics.go index c29f2e07d3..9fe25d5eca 100644 --- a/internal/lsp/source/diagnostics.go +++ b/internal/lsp/source/diagnostics.go @@ -130,10 +130,7 @@ func Diagnostics(ctx context.Context, snapshot Snapshot, ph PackageHandle, missi return nil, warn, ctx.Err() } // If we don't have any list or parse errors, run analyses. - analyzers := snapshot.View().Options().DefaultAnalyzers - if hadTypeErrors { - analyzers = snapshot.View().Options().TypeErrorAnalyzers - } + analyzers := pickAnalyzers(snapshot, hadTypeErrors) if err := analyses(ctx, snapshot, reports, ph, analyzers); err != nil { event.Error(ctx, "analyses failed", err, tag.Snapshot.Of(snapshot.ID()), tag.Package.Of(ph.ID())) if ctx.Err() != nil { @@ -161,6 +158,26 @@ func ignoreFile(path string) bool { return false } +func pickAnalyzers(snapshot Snapshot, hadTypeErrors bool) map[string]Analyzer { + analyzers := make(map[string]Analyzer) + + // Always run convenience analyzers. + for k, v := range snapshot.View().Options().ConvenienceAnalyzers { + analyzers[k] = v + } + // If we had type errors, only run type error analyzers. + if hadTypeErrors { + for k, v := range snapshot.View().Options().TypeErrorAnalyzers { + analyzers[k] = v + } + return analyzers + } + for k, v := range snapshot.View().Options().DefaultAnalyzers { + analyzers[k] = v + } + return analyzers +} + func FileDiagnostics(ctx context.Context, snapshot Snapshot, uri span.URI) (FileIdentity, []*Diagnostic, error) { fh, err := snapshot.GetFile(ctx, uri) if err != nil { @@ -310,6 +327,13 @@ func analyses(ctx context.Context, snapshot Snapshot, reports map[FileIdentity][ // Report diagnostics and errors from root analyzers. for _, e := range analysisErrors { + // If the diagnostic comes from a "convenience" analyzer, it is not + // meant to provide diagnostics, but rather only suggested fixes. + // Skip these types of errors in diagnostics; we will use their + // suggested fixes when providing code actions. + if isConvenienceAnalyzer(e.Category) { + continue + } // This is a bit of a hack, but clients > 3.15 will be able to grey out unnecessary code. // If we are deleting code as part of all of our suggested fixes, assume that this is dead code. // TODO(golang/go#34508): Return these codes from the diagnostics themselves. @@ -397,3 +421,12 @@ func hasUndeclaredErrors(pkg Package) bool { } return false } + +func isConvenienceAnalyzer(category string) bool { + for _, a := range DefaultOptions().ConvenienceAnalyzers { + if category == a.Analyzer.Name { + return true + } + } + return false +} diff --git a/internal/lsp/source/fill_struct.go b/internal/lsp/source/fill_struct.go deleted file mode 100644 index f7fa00108f..0000000000 --- a/internal/lsp/source/fill_struct.go +++ /dev/null @@ -1,143 +0,0 @@ -// 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 source - -import ( - "context" - "fmt" - "go/format" - "go/types" - "strings" - - "golang.org/x/tools/go/ast/astutil" - "golang.org/x/tools/internal/lsp/protocol" -) - -// FillStruct completes all of targeted struct's fields with their default values. -func FillStruct(ctx context.Context, snapshot Snapshot, fh FileHandle, protoRng protocol.Range) ([]protocol.CodeAction, error) { - pkg, pgh, err := getParsedFile(ctx, snapshot, fh, NarrowestPackageHandle) - if err != nil { - return nil, fmt.Errorf("getting file for struct fill code action: %v", err) - } - file, src, m, _, err := pgh.Cached() - if err != nil { - return nil, err - } - spn, err := m.PointSpan(protoRng.Start) - if err != nil { - return nil, err - } - spanRng, err := spn.Range(m.Converter) - if err != nil { - return nil, err - } - path, _ := astutil.PathEnclosingInterval(file, spanRng.Start, spanRng.End) - if path == nil { - return nil, nil - } - - ecl := enclosingCompositeLiteral(path, spanRng.Start, pkg.GetTypesInfo()) - if ecl == nil || !ecl.isStruct() { - return nil, nil - } - - // If in F{ Bar<> : V} or anywhere in F{Bar : V, ...} - // we should not fill the struct. - if ecl.inKey || len(ecl.cl.Elts) != 0 { - return nil, nil - } - - var codeActions []protocol.CodeAction - qfFunc := qualifier(file, pkg.GetTypes(), pkg.GetTypesInfo()) - switch obj := ecl.clType.(type) { - case *types.Struct: - fieldCount := obj.NumFields() - if fieldCount == 0 { - return nil, nil - } - var fieldSourceCode strings.Builder - for i := 0; i < fieldCount; i++ { - field := obj.Field(i) - // Ignore fields that are not accessible in the current package. - if field.Pkg() != nil && field.Pkg() != pkg.GetTypes() && !field.Exported() { - continue - } - - label := field.Name() - value := formatZeroValue(field.Type(), qfFunc) - fieldSourceCode.WriteString("\n") - fieldSourceCode.WriteString(label) - fieldSourceCode.WriteString(" : ") - fieldSourceCode.WriteString(value) - fieldSourceCode.WriteString(",") - } - - if fieldSourceCode.Len() == 0 { - return nil, nil - } - - fieldSourceCode.WriteString("\n") - - // the range of all text between '<>', inclusive. E.g. {<> ... <}> - mappedRange := newMappedRange(snapshot.View().Session().Cache().FileSet(), m, ecl.cl.Lbrace, ecl.cl.Rbrace+1) - protoRange, err := mappedRange.Range() - if err != nil { - return nil, err - } - // consider formatting from the first character of the line the lbrace is on. - // ToOffset is 1-based - beginOffset, err := m.Converter.ToOffset(int(protoRange.Start.Line)+1, 1) - if err != nil { - return nil, err - } - - endOffset, err := m.Converter.ToOffset(int(protoRange.Start.Line)+1, int(protoRange.Start.Character)+1) - if err != nil { - return nil, err - } - - // An increment to make sure the lbrace is included in the slice. - endOffset++ - // Append the edits. Then append the closing brace. - var newSourceCode strings.Builder - newSourceCode.Grow(endOffset - beginOffset + fieldSourceCode.Len() + 1) - newSourceCode.WriteString(string(src[beginOffset:endOffset])) - newSourceCode.WriteString(fieldSourceCode.String()) - newSourceCode.WriteString("}") - - buf, err := format.Source([]byte(newSourceCode.String())) - if err != nil { - return nil, err - } - - // it is guaranteed that a left brace exists. - var edit = string(buf[strings.IndexByte(string(buf), '{'):]) - - codeActions = append(codeActions, protocol.CodeAction{ - Title: "Fill struct", - Kind: protocol.RefactorRewrite, - Edit: protocol.WorkspaceEdit{ - DocumentChanges: []protocol.TextDocumentEdit{ - { - TextDocument: protocol.VersionedTextDocumentIdentifier{ - Version: fh.Version(), - TextDocumentIdentifier: protocol.TextDocumentIdentifier{ - URI: protocol.URIFromSpanURI(fh.URI()), - }, - }, - Edits: []protocol.TextEdit{ - { - Range: protoRange, - NewText: edit, - }, - }, - }, - }, - }, - }) - } - - return codeActions, nil -} diff --git a/internal/lsp/source/options.go b/internal/lsp/source/options.go index 13aec4ccd2..92c09e8343 100644 --- a/internal/lsp/source/options.go +++ b/internal/lsp/source/options.go @@ -664,7 +664,7 @@ func defaultAnalyzers() map[string]Analyzer { unsafeptr.Analyzer.Name: {Analyzer: unsafeptr.Analyzer, enabled: true}, unusedresult.Analyzer.Name: {Analyzer: unusedresult.Analyzer, enabled: true}, - // Non-vet analyzers + // 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}, diff --git a/internal/lsp/source/source_test.go b/internal/lsp/source/source_test.go index 93b4114ef4..84c6ab0403 100644 --- a/internal/lsp/source/source_test.go +++ b/internal/lsp/source/source_test.go @@ -475,7 +475,6 @@ func (r *runner) Import(t *testing.T, spn span.Span) { } func (r *runner) SuggestedFix(t *testing.T, spn span.Span, actionKinds []string) {} -func (r *runner) RefactorRewrite(t *testing.T, spn span.Span, title string) {} func (r *runner) Definition(t *testing.T, spn span.Span, d tests.Definition) { _, srcRng, err := spanToRange(r.data, d.Src) diff --git a/internal/lsp/testdata/indirect/summary.txt.golden b/internal/lsp/testdata/indirect/summary.txt.golden index fdd759352e..5c4f74a660 100644 --- a/internal/lsp/testdata/indirect/summary.txt.golden +++ b/internal/lsp/testdata/indirect/summary.txt.golden @@ -25,5 +25,4 @@ CaseSensitiveWorkspaceSymbolsCount = 0 SignaturesCount = 0 LinksCount = 0 ImplementationsCount = 0 -RefactorRewriteCount = 0 diff --git a/internal/lsp/testdata/lsp/primarymod/fillstruct/fill_struct.go b/internal/lsp/testdata/lsp/primarymod/fillstruct/fill_struct.go index 9bc5c4588e..7fd1597c90 100644 --- a/internal/lsp/testdata/lsp/primarymod/fillstruct/fill_struct.go +++ b/internal/lsp/testdata/lsp/primarymod/fillstruct/fill_struct.go @@ -1,23 +1,23 @@ -package fillstruct - -type StructA struct { - unexportedIntField int - ExportedIntField int - MapA map[int]string - Array []int - StructB -} - -type StructA2 struct { - B *StructB -} - -type StructA3 struct { - B StructB -} - -func fill() { - a := StructA{} //@refactorrewrite("}", "Fill struct") - b := StructA2{} //@refactorrewrite("}", "Fill struct") - c := StructA3{} //@refactorrewrite("}", "Fill struct") -} +package fillstruct + +type StructA struct { + unexportedIntField int + ExportedIntField int + MapA map[int]string + Array []int + StructB +} + +type StructA2 struct { + B *StructB +} + +type StructA3 struct { + B StructB +} + +func fill() { + a := StructA{} //@suggestedfix("}", "refactor.rewrite") + b := StructA2{} //@suggestedfix("}", "refactor.rewrite") + c := StructA3{} //@suggestedfix("}", "refactor.rewrite") +} diff --git a/internal/lsp/testdata/lsp/primarymod/fillstruct/fill_struct.go.golden b/internal/lsp/testdata/lsp/primarymod/fillstruct/fill_struct.go.golden index e29f04053a..13c0a0e84a 100644 --- a/internal/lsp/testdata/lsp/primarymod/fillstruct/fill_struct.go.golden +++ b/internal/lsp/testdata/lsp/primarymod/fillstruct/fill_struct.go.golden @@ -1,85 +1,85 @@ --- fill_struct_20_15 -- -package fillstruct - -type StructA struct { - unexportedIntField int - ExportedIntField int - MapA map[int]string - Array []int - StructB -} - -type StructA2 struct { - B *StructB -} - -type StructA3 struct { - B StructB -} - -func fill() { +-- suggestedfix_fill_struct_20_15 -- +package fillstruct + +type StructA struct { + unexportedIntField int + ExportedIntField int + MapA map[int]string + Array []int + StructB +} + +type StructA2 struct { + B *StructB +} + +type StructA3 struct { + B StructB +} + +func fill() { a := StructA{ - unexportedIntField: 0, - ExportedIntField: 0, - MapA: nil, - Array: nil, - StructB: StructB{}, - } //@refactorrewrite("}", "Fill struct") - b := StructA2{} //@refactorrewrite("}", "Fill struct") - c := StructA3{} //@refactorrewrite("}", "Fill struct") -} +unexportedIntField : 0, +ExportedIntField : 0, +MapA : nil, +Array : nil, +StructB : StructB{}, +} //@suggestedfix("}", "refactor.rewrite") + b := StructA2{} //@suggestedfix("}", "refactor.rewrite") + c := StructA3{} //@suggestedfix("}", "refactor.rewrite") +} --- fill_struct_21_16 -- -package fillstruct - -type StructA struct { - unexportedIntField int - ExportedIntField int - MapA map[int]string - Array []int - StructB -} - -type StructA2 struct { - B *StructB -} - -type StructA3 struct { - B StructB -} - -func fill() { - a := StructA{} //@refactorrewrite("}", "Fill struct") +-- suggestedfix_fill_struct_21_16 -- +package fillstruct + +type StructA struct { + unexportedIntField int + ExportedIntField int + MapA map[int]string + Array []int + StructB +} + +type StructA2 struct { + B *StructB +} + +type StructA3 struct { + B StructB +} + +func fill() { + a := StructA{} //@suggestedfix("}", "refactor.rewrite") b := StructA2{ - B: nil, - } //@refactorrewrite("}", "Fill struct") - c := StructA3{} //@refactorrewrite("}", "Fill struct") -} +B : nil, +} //@suggestedfix("}", "refactor.rewrite") + c := StructA3{} //@suggestedfix("}", "refactor.rewrite") +} --- fill_struct_22_16 -- -package fillstruct - -type StructA struct { - unexportedIntField int - ExportedIntField int - MapA map[int]string - Array []int - StructB -} - -type StructA2 struct { - B *StructB -} - -type StructA3 struct { - B StructB -} - -func fill() { - a := StructA{} //@refactorrewrite("}", "Fill struct") - b := StructA2{} //@refactorrewrite("}", "Fill struct") +-- suggestedfix_fill_struct_22_16 -- +package fillstruct + +type StructA struct { + unexportedIntField int + ExportedIntField int + MapA map[int]string + Array []int + StructB +} + +type StructA2 struct { + B *StructB +} + +type StructA3 struct { + B StructB +} + +func fill() { + a := StructA{} //@suggestedfix("}", "refactor.rewrite") + b := StructA2{} //@suggestedfix("}", "refactor.rewrite") c := StructA3{ - B: StructB{}, - } //@refactorrewrite("}", "Fill struct") -} +B : StructB{}, +} //@suggestedfix("}", "refactor.rewrite") +} diff --git a/internal/lsp/testdata/lsp/primarymod/fillstruct/fill_struct_nested.go b/internal/lsp/testdata/lsp/primarymod/fillstruct/fill_struct_nested.go index 0c4c1d88c2..79eb84b747 100644 --- a/internal/lsp/testdata/lsp/primarymod/fillstruct/fill_struct_nested.go +++ b/internal/lsp/testdata/lsp/primarymod/fillstruct/fill_struct_nested.go @@ -1,15 +1,15 @@ -package fillstruct - -type StructB struct { - StructC -} - -type StructC struct { - unexportedInt int -} - -func nested() { - c := StructB{ - StructC: StructC{}, //@refactorrewrite("}", "Fill struct") - } -} +package fillstruct + +type StructB struct { + StructC +} + +type StructC struct { + unexportedInt int +} + +func nested() { + c := StructB{ + StructC: StructC{}, //@suggestedfix("}", "refactor.rewrite") + } +} diff --git a/internal/lsp/testdata/lsp/primarymod/fillstruct/fill_struct_nested.go.golden b/internal/lsp/testdata/lsp/primarymod/fillstruct/fill_struct_nested.go.golden index 74c1e6a624..43a7b5b19c 100644 --- a/internal/lsp/testdata/lsp/primarymod/fillstruct/fill_struct_nested.go.golden +++ b/internal/lsp/testdata/lsp/primarymod/fillstruct/fill_struct_nested.go.golden @@ -1,19 +1,19 @@ --- fill_struct_nested_13_20 -- -package fillstruct - -type StructB struct { - StructC -} - -type StructC struct { - unexportedInt int -} - -func nested() { - c := StructB{ - StructC: StructC{ - unexportedInt: 0, - }, //@refactorrewrite("}", "Fill struct") - } -} +-- suggestedfix_fill_struct_nested_13_20 -- +package fillstruct + +type StructB struct { + StructC +} + +type StructC struct { + unexportedInt int +} + +func nested() { + c := StructB{ + StructC: StructC{ +unexportedInt : 0, +}, //@suggestedfix("}", "refactor.rewrite") + } +} diff --git a/internal/lsp/testdata/lsp/primarymod/fillstruct/fill_struct_package.go b/internal/lsp/testdata/lsp/primarymod/fillstruct/fill_struct_package.go index e7b30e53aa..336a375180 100644 --- a/internal/lsp/testdata/lsp/primarymod/fillstruct/fill_struct_package.go +++ b/internal/lsp/testdata/lsp/primarymod/fillstruct/fill_struct_package.go @@ -1,9 +1,9 @@ -package fillstruct - -import ( - data "golang.org/x/tools/internal/lsp/fillstruct/data" -) - -func unexported() { - a := data.A{} //@refactorrewrite("}", "Fill struct") -} +package fillstruct + +import ( + "golang.org/x/tools/internal/lsp/fillstruct/data" +) + +func unexported() { + a := data.A{} //@suggestedfix("}", "refactor.rewrite") +} diff --git a/internal/lsp/testdata/lsp/primarymod/fillstruct/fill_struct_package.go.golden b/internal/lsp/testdata/lsp/primarymod/fillstruct/fill_struct_package.go.golden index df33786e6c..f11a5afe52 100644 --- a/internal/lsp/testdata/lsp/primarymod/fillstruct/fill_struct_package.go.golden +++ b/internal/lsp/testdata/lsp/primarymod/fillstruct/fill_struct_package.go.golden @@ -1,13 +1,13 @@ --- fill_struct_package_8_14 -- -package fillstruct - -import ( - data "golang.org/x/tools/internal/lsp/fillstruct/data" -) - -func unexported() { - a := data.A{ - ExportedInt: 0, - } //@refactorrewrite("}", "Fill struct") -} +-- suggestedfix_fill_struct_package_8_14 -- +package fillstruct + +import ( + "golang.org/x/tools/internal/lsp/fillstruct/data" +) + +func unexported() { + a := data.A{ +ExportedInt : 0, +} //@suggestedfix("}", "refactor.rewrite") +} diff --git a/internal/lsp/testdata/lsp/primarymod/fillstruct/fill_struct_spaces.go b/internal/lsp/testdata/lsp/primarymod/fillstruct/fill_struct_spaces.go new file mode 100644 index 0000000000..d5d1bbba5c --- /dev/null +++ b/internal/lsp/testdata/lsp/primarymod/fillstruct/fill_struct_spaces.go @@ -0,0 +1,9 @@ +package fillstruct + +type StructD struct { + ExportedIntField int +} + +func spaces() { + d := StructD{} //@suggestedfix("}", "refactor.rewrite") +} diff --git a/internal/lsp/testdata/lsp/primarymod/fillstruct/fill_struct_spaces.go.golden b/internal/lsp/testdata/lsp/primarymod/fillstruct/fill_struct_spaces.go.golden index c938b501ae..33643a6f43 100644 --- a/internal/lsp/testdata/lsp/primarymod/fillstruct/fill_struct_spaces.go.golden +++ b/internal/lsp/testdata/lsp/primarymod/fillstruct/fill_struct_spaces.go.golden @@ -1,13 +1,13 @@ --- fill_struct_spaces_10_1 -- -package fillstruct - -type StructB struct { - ExportedIntField int -} - -func spaces() { - b := StructB{ - ExportedIntField: 0, - } -} +-- suggestedfix_fill_struct_spaces_8_15 -- +package fillstruct + +type StructD struct { + ExportedIntField int +} + +func spaces() { + d := StructD{ +ExportedIntField : 0, +} //@suggestedfix("}", "refactor.rewrite") +} diff --git a/internal/lsp/testdata/lsp/summary.txt.golden b/internal/lsp/testdata/lsp/summary.txt.golden index 26df609c26..74536e9276 100644 --- a/internal/lsp/testdata/lsp/summary.txt.golden +++ b/internal/lsp/testdata/lsp/summary.txt.golden @@ -11,7 +11,7 @@ DiagnosticsCount = 44 FoldingRangesCount = 2 FormatCount = 6 ImportCount = 8 -SuggestedFixCount = 6 +SuggestedFixCount = 12 DefinitionsCount = 53 TypeDefinitionsCount = 2 HighlightsCount = 60 @@ -25,5 +25,4 @@ CaseSensitiveWorkspaceSymbolsCount = 2 SignaturesCount = 32 LinksCount = 8 ImplementationsCount = 14 -RefactorRewriteCount = 5 diff --git a/internal/lsp/testdata/missingdep/summary.txt.golden b/internal/lsp/testdata/missingdep/summary.txt.golden index fdd759352e..5c4f74a660 100644 --- a/internal/lsp/testdata/missingdep/summary.txt.golden +++ b/internal/lsp/testdata/missingdep/summary.txt.golden @@ -25,5 +25,4 @@ CaseSensitiveWorkspaceSymbolsCount = 0 SignaturesCount = 0 LinksCount = 0 ImplementationsCount = 0 -RefactorRewriteCount = 0 diff --git a/internal/lsp/testdata/missingtwodep/summary.txt.golden b/internal/lsp/testdata/missingtwodep/summary.txt.golden index a061dc661a..96ac4750a8 100644 --- a/internal/lsp/testdata/missingtwodep/summary.txt.golden +++ b/internal/lsp/testdata/missingtwodep/summary.txt.golden @@ -25,5 +25,4 @@ CaseSensitiveWorkspaceSymbolsCount = 0 SignaturesCount = 0 LinksCount = 0 ImplementationsCount = 0 -RefactorRewriteCount = 0 diff --git a/internal/lsp/testdata/unused/summary.txt.golden b/internal/lsp/testdata/unused/summary.txt.golden index fdd759352e..5c4f74a660 100644 --- a/internal/lsp/testdata/unused/summary.txt.golden +++ b/internal/lsp/testdata/unused/summary.txt.golden @@ -25,5 +25,4 @@ CaseSensitiveWorkspaceSymbolsCount = 0 SignaturesCount = 0 LinksCount = 0 ImplementationsCount = 0 -RefactorRewriteCount = 0 diff --git a/internal/lsp/testdata/upgradedep/summary.txt.golden b/internal/lsp/testdata/upgradedep/summary.txt.golden index 90c373043f..79042cc6c8 100644 --- a/internal/lsp/testdata/upgradedep/summary.txt.golden +++ b/internal/lsp/testdata/upgradedep/summary.txt.golden @@ -25,5 +25,4 @@ CaseSensitiveWorkspaceSymbolsCount = 0 SignaturesCount = 0 LinksCount = 4 ImplementationsCount = 0 -RefactorRewriteCount = 0 diff --git a/internal/lsp/tests/tests.go b/internal/lsp/tests/tests.go index 2d7eeab458..850ec229b3 100644 --- a/internal/lsp/tests/tests.go +++ b/internal/lsp/tests/tests.go @@ -57,7 +57,6 @@ type FoldingRanges []span.Span type Formats []span.Span type Imports []span.Span type SuggestedFixes map[span.Span][]string -type RefactorRewriteActions map[span.Span]string type Definitions map[span.Span]Definition type Implementations map[span.Span][]span.Span type Highlights map[span.Span][]span.Span @@ -88,7 +87,6 @@ type Data struct { Formats Formats Imports Imports SuggestedFixes SuggestedFixes - RefactorRewrite RefactorRewriteActions Definitions Definitions Implementations Implementations Highlights Highlights @@ -142,7 +140,6 @@ type Tests interface { CaseSensitiveWorkspaceSymbols(*testing.T, string, []protocol.SymbolInformation, map[string]struct{}) SignatureHelp(*testing.T, span.Span, *protocol.SignatureHelp) Link(*testing.T, span.URI, []Link) - RefactorRewrite(*testing.T, span.Span, string) } type Definition struct { @@ -219,6 +216,8 @@ func DefaultOptions() source.Options { source.Go: { protocol.SourceOrganizeImports: true, protocol.QuickFix: true, + protocol.RefactorRewrite: true, + protocol.SourceFixAll: true, }, source.Mod: { protocol.SourceOrganizeImports: true, @@ -296,7 +295,6 @@ func Load(t testing.TB, exporter packagestest.Exporter, dir string) []*Data { CaseSensitiveWorkspaceSymbols: make(WorkspaceSymbols), Signatures: make(Signatures), Links: make(Links), - RefactorRewrite: make(RefactorRewriteActions), t: t, dir: folder, @@ -421,7 +419,6 @@ func Load(t testing.TB, exporter packagestest.Exporter, dir string) []*Data { "signature": datum.collectSignatures, "link": datum.collectLinks, "suggestedfix": datum.collectSuggestedFixes, - "refactorrewrite": datum.collectRefactorRewrite, }); err != nil { t.Fatal(err) } @@ -599,17 +596,6 @@ func Run(t *testing.T, tests Tests, data *Data) { } }) - t.Run("RefactorRewrite", func(t *testing.T) { - t.Helper() - - for spn, title := range data.RefactorRewrite { - t.Run(SpanName(spn), func(t *testing.T) { - t.Helper() - tests.RefactorRewrite(t, spn, title) - }) - } - }) - t.Run("SuggestedFix", func(t *testing.T) { t.Helper() for spn, actionKinds := range data.SuggestedFixes { @@ -827,7 +813,6 @@ func checkData(t *testing.T, data *Data) { fmt.Fprintf(buf, "SignaturesCount = %v\n", len(data.Signatures)) fmt.Fprintf(buf, "LinksCount = %v\n", linksCount) fmt.Fprintf(buf, "ImplementationsCount = %v\n", len(data.Implementations)) - fmt.Fprintf(buf, "RefactorRewriteCount = %v\n", len(data.RefactorRewrite)) want := string(data.Golden("summary", summaryFile, func() ([]byte, error) { return buf.Bytes(), nil @@ -1037,10 +1022,6 @@ func (data *Data) collectSuggestedFixes(spn span.Span, actionKind string) { data.SuggestedFixes[spn] = append(data.SuggestedFixes[spn], actionKind) } -func (data *Data) collectRefactorRewrite(spn span.Span, title string) { - data.RefactorRewrite[spn] = title -} - func (data *Data) collectDefinitions(src, target span.Span) { data.Definitions[src] = Definition{ Src: src,