From bacac14996a49a5a3033fb09bb344f8beed596ee Mon Sep 17 00:00:00 2001 From: Viktor Blomqvist Date: Wed, 12 Jul 2023 17:44:34 +0200 Subject: [PATCH] gopls/internal/lsp/source: Add SuggestedFix for embeddirective Analyzer Give the embeddirective source.Analyzer a Fix that can add missing "embed" imports. Since not all reports from the analyzer is this diagnostic, give source.Analyzer an IsFixable method to filter diagnostics. Updates golang/go#50262 Change-Id: I24abdd2d1a88fc5cabd3197ef438c4da041a6a9c Reviewed-on: https://go-review.googlesource.com/c/tools/+/509056 Run-TryBot: Robert Findley gopls-CI: kokoro TryBot-Result: Gopher Robot Reviewed-by: Robert Findley Reviewed-by: Hyang-Ah Hana Kim --- .../analysis/embeddirective/embeddirective.go | 9 +++- gopls/internal/lsp/cache/errors.go | 23 +++++---- gopls/internal/lsp/source/fix.go | 51 +++++++++++++++++++ gopls/internal/lsp/source/options.go | 7 ++- gopls/internal/lsp/source/view.go | 12 +++++ .../lsp/testdata/embeddirective/embed.txt | 1 + .../lsp/testdata/embeddirective/fix_import.go | 18 +++++++ .../embeddirective/fix_import.go.golden | 21 ++++++++ .../internal/lsp/testdata/summary.txt.golden | 2 +- .../lsp/testdata/summary_go1.18.txt.golden | 2 +- .../lsp/testdata/summary_go1.21.txt.golden | 2 +- .../regtest/diagnostics/analysis_test.go | 40 +++++++++++++++ 12 files changed, 172 insertions(+), 16 deletions(-) create mode 100644 gopls/internal/lsp/testdata/embeddirective/embed.txt create mode 100644 gopls/internal/lsp/testdata/embeddirective/fix_import.go create mode 100644 gopls/internal/lsp/testdata/embeddirective/fix_import.go.golden diff --git a/gopls/internal/lsp/analysis/embeddirective/embeddirective.go b/gopls/internal/lsp/analysis/embeddirective/embeddirective.go index dc9df21ff2a..8cc4ea57443 100644 --- a/gopls/internal/lsp/analysis/embeddirective/embeddirective.go +++ b/gopls/internal/lsp/analysis/embeddirective/embeddirective.go @@ -2,7 +2,8 @@ // Use of this source code is governed by a BSD-style // license that can be found in the LICENSE file. -// Package embeddirective defines an Analyzer that validates import for //go:embed directive. +// Package embeddirective defines an Analyzer that validates //go:embed directives. +// The analyzer defers fixes to it's parent source.Analyzer. package embeddirective import ( @@ -26,6 +27,10 @@ var Analyzer = &analysis.Analyzer{ RunDespiteErrors: true, } +// source.fixedByImportingEmbed relies on this message to filter +// out fixable diagnostics from this Analyzer. +const MissingImportMessage = `must import "embed" when using go:embed directives` + func run(pass *analysis.Pass) (interface{}, error) { for _, f := range pass.Files { comments := embedDirectiveComments(f) @@ -51,7 +56,7 @@ func run(pass *analysis.Pass) (interface{}, error) { } if !hasEmbedImport { - report(`must import "embed" when using go:embed directives`) + report(MissingImportMessage) } spec := nextVarSpec(c, f) diff --git a/gopls/internal/lsp/cache/errors.go b/gopls/internal/lsp/cache/errors.go index 0e553bbba73..e252ee2930c 100644 --- a/gopls/internal/lsp/cache/errors.go +++ b/gopls/internal/lsp/cache/errors.go @@ -339,17 +339,20 @@ func toSourceDiagnostic(srcAnalyzer *source.Analyzer, gobDiag *gobDiagnostic) *s } diag := &source.Diagnostic{ - URI: gobDiag.Location.URI.SpanURI(), - Range: gobDiag.Location.Range, - Severity: severity, - Code: gobDiag.Code, - CodeHref: gobDiag.CodeHref, - Source: source.AnalyzerErrorKind(gobDiag.Source), - Message: gobDiag.Message, - Related: related, - SuggestedFixes: fixes, - Tags: srcAnalyzer.Tag, + URI: gobDiag.Location.URI.SpanURI(), + Range: gobDiag.Location.Range, + Severity: severity, + Code: gobDiag.Code, + CodeHref: gobDiag.CodeHref, + Source: source.AnalyzerErrorKind(gobDiag.Source), + Message: gobDiag.Message, + Related: related, + Tags: srcAnalyzer.Tag, + } + if srcAnalyzer.FixesDiagnostic(diag) { + diag.SuggestedFixes = fixes } + // If the fixes only delete code, assume that the diagnostic is reporting dead code. if onlyDeletions(fixes) { diag.Tags = append(diag.Tags, protocol.Unnecessary) diff --git a/gopls/internal/lsp/source/fix.go b/gopls/internal/lsp/source/fix.go index cb8e5a3cd76..f9d901c196c 100644 --- a/gopls/internal/lsp/source/fix.go +++ b/gopls/internal/lsp/source/fix.go @@ -13,10 +13,12 @@ import ( "golang.org/x/tools/go/analysis" "golang.org/x/tools/gopls/internal/bug" + "golang.org/x/tools/gopls/internal/lsp/analysis/embeddirective" "golang.org/x/tools/gopls/internal/lsp/analysis/fillstruct" "golang.org/x/tools/gopls/internal/lsp/analysis/undeclaredname" "golang.org/x/tools/gopls/internal/lsp/protocol" "golang.org/x/tools/gopls/internal/span" + "golang.org/x/tools/internal/imports" ) type ( @@ -41,6 +43,7 @@ const ( ExtractFunction = "extract_function" ExtractMethod = "extract_method" InvertIfCondition = "invert_if_condition" + AddEmbedImport = "add_embed_import" ) // suggestedFixes maps a suggested fix command id to its handler. @@ -52,6 +55,7 @@ var suggestedFixes = map[string]SuggestedFixFunc{ ExtractMethod: singleFile(extractMethod), InvertIfCondition: singleFile(invertIfCondition), StubMethods: stubSuggestedFixFunc, + AddEmbedImport: addEmbedImport, } // singleFile calls analyzers that expect inputs for a single file @@ -138,3 +142,50 @@ func ApplyFix(ctx context.Context, fix string, snapshot Snapshot, fh FileHandle, } return edits, nil } + +// fixedByImportingEmbed returns true if diag can be fixed by addEmbedImport. +func fixedByImportingEmbed(diag *Diagnostic) bool { + if diag == nil { + return false + } + return diag.Message == embeddirective.MissingImportMessage +} + +// addEmbedImport adds a missing embed "embed" import with blank name. +func addEmbedImport(ctx context.Context, snapshot Snapshot, fh FileHandle, rng protocol.Range) (*token.FileSet, *analysis.SuggestedFix, error) { + pkg, pgf, err := NarrowestPackageForFile(ctx, snapshot, fh.URI()) + if err != nil { + return nil, nil, fmt.Errorf("narrow pkg: %w", err) + } + + // Like source.AddImport, but with _ as Name and using our pgf. + protoEdits, err := ComputeOneImportFixEdits(snapshot, pgf, &imports.ImportFix{ + StmtInfo: imports.ImportInfo{ + ImportPath: "embed", + Name: "_", + }, + FixType: imports.AddImport, + }) + if err != nil { + return nil, nil, fmt.Errorf("compute edits: %w", err) + } + + var edits []analysis.TextEdit + for _, e := range protoEdits { + start, end, err := pgf.RangePos(e.Range) + if err != nil { + return nil, nil, fmt.Errorf("map range: %w", err) + } + edits = append(edits, analysis.TextEdit{ + Pos: start, + End: end, + NewText: []byte(e.NewText), + }) + } + + fix := &analysis.SuggestedFix{ + Message: "Add embed import", + TextEdits: edits, + } + return pkg.FileSet(), fix, nil +} diff --git a/gopls/internal/lsp/source/options.go b/gopls/internal/lsp/source/options.go index 267a7d3a47f..4c3301457a7 100644 --- a/gopls/internal/lsp/source/options.go +++ b/gopls/internal/lsp/source/options.go @@ -1574,8 +1574,13 @@ func defaultAnalyzers() map[string]*Analyzer { unusedparams.Analyzer.Name: {Analyzer: unusedparams.Analyzer, Enabled: false}, unusedwrite.Analyzer.Name: {Analyzer: unusedwrite.Analyzer, Enabled: false}, useany.Analyzer.Name: {Analyzer: useany.Analyzer, Enabled: false}, - embeddirective.Analyzer.Name: {Analyzer: embeddirective.Analyzer, Enabled: true}, timeformat.Analyzer.Name: {Analyzer: timeformat.Analyzer, Enabled: true}, + embeddirective.Analyzer.Name: { + Analyzer: embeddirective.Analyzer, + Enabled: true, + Fix: AddEmbedImport, + fixesDiagnostic: fixedByImportingEmbed, + }, // gofmt -s suite: simplifycompositelit.Analyzer.Name: { diff --git a/gopls/internal/lsp/source/view.go b/gopls/internal/lsp/source/view.go index 4bdb59714f4..b47e5b800ce 100644 --- a/gopls/internal/lsp/source/view.go +++ b/gopls/internal/lsp/source/view.go @@ -879,6 +879,10 @@ type Analyzer struct { // the analyzer's suggested fixes through a Command, not a TextEdit. Fix string + // fixesDiagnostic reports if a diagnostic from the analyzer can be fixed by Fix. + // If nil then all diagnostics from the analyzer are assumed to be fixable. + fixesDiagnostic func(*Diagnostic) bool + // ActionKind is the kind of code action this analyzer produces. If // unspecified the type defaults to quickfix. ActionKind []protocol.CodeActionKind @@ -908,6 +912,14 @@ func (a Analyzer) IsEnabled(options *Options) bool { return a.Enabled } +// FixesDiagnostic returns true if Analyzer.Fix can fix the Diagnostic. +func (a Analyzer) FixesDiagnostic(d *Diagnostic) bool { + if a.fixesDiagnostic == nil { + return true + } + return a.fixesDiagnostic(d) +} + // Declare explicit types for package paths, names, and IDs to ensure that we // never use an ID where a path belongs, and vice versa. If we confused these, // it would result in confusing errors because package IDs often look like diff --git a/gopls/internal/lsp/testdata/embeddirective/embed.txt b/gopls/internal/lsp/testdata/embeddirective/embed.txt new file mode 100644 index 00000000000..8e27be7d615 --- /dev/null +++ b/gopls/internal/lsp/testdata/embeddirective/embed.txt @@ -0,0 +1 @@ +text diff --git a/gopls/internal/lsp/testdata/embeddirective/fix_import.go b/gopls/internal/lsp/testdata/embeddirective/fix_import.go new file mode 100644 index 00000000000..5eaf3d09868 --- /dev/null +++ b/gopls/internal/lsp/testdata/embeddirective/fix_import.go @@ -0,0 +1,18 @@ +// Copyright 2023 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 embeddirective + +import ( + "io" + "os" +) + +//go:embed embed.txt //@suggestedfix("//go:embed", "quickfix", "") +var t string + +func unused() { + _ = os.Stdin + _ = io.EOF +} diff --git a/gopls/internal/lsp/testdata/embeddirective/fix_import.go.golden b/gopls/internal/lsp/testdata/embeddirective/fix_import.go.golden new file mode 100644 index 00000000000..15a23f4d0a3 --- /dev/null +++ b/gopls/internal/lsp/testdata/embeddirective/fix_import.go.golden @@ -0,0 +1,21 @@ +-- suggestedfix_fix_import_12_1 -- +// Copyright 2023 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 embeddirective + +import ( + _ "embed" + "io" + "os" +) + +//go:embed embed.txt //@suggestedfix("//go:embed", "quickfix", "") +var t string + +func unused() { + _ = os.Stdin + _ = io.EOF +} + diff --git a/gopls/internal/lsp/testdata/summary.txt.golden b/gopls/internal/lsp/testdata/summary.txt.golden index aaa28c337ef..4e6c3a08cdc 100644 --- a/gopls/internal/lsp/testdata/summary.txt.golden +++ b/gopls/internal/lsp/testdata/summary.txt.golden @@ -11,7 +11,7 @@ CaseSensitiveCompletionsCount = 4 DiagnosticsCount = 23 FoldingRangesCount = 2 SemanticTokenCount = 3 -SuggestedFixCount = 73 +SuggestedFixCount = 74 MethodExtractionCount = 8 DefinitionsCount = 46 TypeDefinitionsCount = 18 diff --git a/gopls/internal/lsp/testdata/summary_go1.18.txt.golden b/gopls/internal/lsp/testdata/summary_go1.18.txt.golden index 859a8b3837f..7375b821e69 100644 --- a/gopls/internal/lsp/testdata/summary_go1.18.txt.golden +++ b/gopls/internal/lsp/testdata/summary_go1.18.txt.golden @@ -11,7 +11,7 @@ CaseSensitiveCompletionsCount = 4 DiagnosticsCount = 23 FoldingRangesCount = 2 SemanticTokenCount = 3 -SuggestedFixCount = 79 +SuggestedFixCount = 80 MethodExtractionCount = 8 DefinitionsCount = 46 TypeDefinitionsCount = 18 diff --git a/gopls/internal/lsp/testdata/summary_go1.21.txt.golden b/gopls/internal/lsp/testdata/summary_go1.21.txt.golden index 18b7022e37f..8d6a32bb986 100644 --- a/gopls/internal/lsp/testdata/summary_go1.21.txt.golden +++ b/gopls/internal/lsp/testdata/summary_go1.21.txt.golden @@ -11,7 +11,7 @@ CaseSensitiveCompletionsCount = 4 DiagnosticsCount = 24 FoldingRangesCount = 2 SemanticTokenCount = 3 -SuggestedFixCount = 79 +SuggestedFixCount = 80 MethodExtractionCount = 8 DefinitionsCount = 46 TypeDefinitionsCount = 18 diff --git a/gopls/internal/regtest/diagnostics/analysis_test.go b/gopls/internal/regtest/diagnostics/analysis_test.go index fc11eeb9492..190f5777258 100644 --- a/gopls/internal/regtest/diagnostics/analysis_test.go +++ b/gopls/internal/regtest/diagnostics/analysis_test.go @@ -85,3 +85,43 @@ func main() { }) } } + +// Test the embed directive analyzer. +// +// There is a fix for missing imports, but it should not trigger for other +// kinds of issues reported by the analayzer, here the variable +// declaration following the embed directive is wrong. +func TestNoSuggestedFixesForEmbedDirectiveDeclaration(t *testing.T) { + const generated = ` +-- go.mod -- +module mod.com + +go 1.20 + +-- foo.txt -- +FOO + +-- main.go -- +package main + +import _ "embed" + +//go:embed foo.txt +var foo, bar string + +func main() { + _ = foo +} +` + Run(t, generated, func(t *testing.T, env *Env) { + env.OpenFile("main.go") + var d protocol.PublishDiagnosticsParams + env.AfterChange( + Diagnostics(env.AtRegexp("main.go", "//go:embed")), + ReadDiagnostics("main.go", &d), + ) + if fixes := env.GetQuickFixes("main.go", d.Diagnostics); len(fixes) != 0 { + t.Errorf("got quick fixes %v, wanted none", fixes) + } + }) +}