From c0b9fb59f72f14cd18bb029942e14b7c2cf4a5d7 Mon Sep 17 00:00:00 2001 From: Heschi Kreinick Date: Tue, 8 Feb 2022 15:24:08 -0500 Subject: [PATCH] internal/lsp/analysis/undeclaredname: suppress impossible quick fixes Reading the code and running the tests, it appears we only support quick fixes for undeclared name errors inside function bodies. Don't offer them in other places. The specific situation raised was type constraints, so that's what I tested even though the problem was much broader. Also add actual error messages where we had empty strings. Fixes golang/go#50935. Change-Id: I8dadcb6e4301bf22cbe7fe5d66064aafeef02690 Reviewed-on: https://go-review.googlesource.com/c/tools/+/384117 Trust: Heschi Kreinick Run-TryBot: Heschi Kreinick Reviewed-by: Robert Findley gopls-CI: kokoro TryBot-Result: Gopher Robot --- .../regtest/diagnostics/diagnostics_test.go | 28 +++++ .../lsp/analysis/undeclaredname/undeclared.go | 111 ++++++++++-------- 2 files changed, 92 insertions(+), 47 deletions(-) diff --git a/gopls/internal/regtest/diagnostics/diagnostics_test.go b/gopls/internal/regtest/diagnostics/diagnostics_test.go index 82da4cd392..e5b346cb3d 100644 --- a/gopls/internal/regtest/diagnostics/diagnostics_test.go +++ b/gopls/internal/regtest/diagnostics/diagnostics_test.go @@ -2157,3 +2157,31 @@ const C = 0b10 env.Await(EmptyDiagnostics("main.go")) }) } + +func TestNoQuickFixForUndeclaredConstraint(t *testing.T) { + testenv.NeedsGo1Point(t, 18) + const files = ` +-- go.mod -- +module mod.com + +go 1.18 +-- main.go -- +package main + +func F[T C](_ T) { +} +` + + Run(t, files, func(t *testing.T, env *Env) { + var d protocol.PublishDiagnosticsParams + env.Await( + OnceMet( + env.DiagnosticAtRegexpWithMessage("main.go", `C`, "undeclared name"), + ReadDiagnostics("main.go", &d), + ), + ) + if fixes := env.GetQuickFixes("main.go", d.Diagnostics); len(fixes) != 0 { + t.Errorf("got quick fixes %v, wanted none", fixes) + } + }) +} diff --git a/internal/lsp/analysis/undeclaredname/undeclared.go b/internal/lsp/analysis/undeclaredname/undeclared.go index 1e4770631e..22b552c374 100644 --- a/internal/lsp/analysis/undeclaredname/undeclared.go +++ b/internal/lsp/analysis/undeclaredname/undeclared.go @@ -49,59 +49,80 @@ const undeclaredNamePrefix = "undeclared name: " func run(pass *analysis.Pass) (interface{}, error) { for _, err := range analysisinternal.GetTypeErrors(pass) { - if !FixesError(err.Msg) { - continue - } - name := strings.TrimPrefix(err.Msg, undeclaredNamePrefix) - var file *ast.File - for _, f := range pass.Files { - if f.Pos() <= err.Pos && err.Pos < f.End() { - file = f - break - } - } - if file == nil { - continue - } - - // Get the path for the relevant range. - path, _ := astutil.PathEnclosingInterval(file, err.Pos, err.Pos) - if len(path) < 2 { - continue - } - ident, ok := path[0].(*ast.Ident) - if !ok || ident.Name != name { - continue - } - // Skip selector expressions because it might be too complex - // to try and provide a suggested fix for fields and methods. - if _, ok := path[1].(*ast.SelectorExpr); ok { - continue - } - tok := pass.Fset.File(file.Pos()) - if tok == nil { - continue - } - offset := pass.Fset.Position(err.Pos).Offset - end := tok.Pos(offset + len(name)) - pass.Report(analysis.Diagnostic{ - Pos: err.Pos, - End: end, - Message: err.Msg, - }) + runForError(pass, err) } return nil, nil } +func runForError(pass *analysis.Pass, err types.Error) { + if !strings.HasPrefix(err.Msg, undeclaredNamePrefix) { + return + } + name := strings.TrimPrefix(err.Msg, undeclaredNamePrefix) + var file *ast.File + for _, f := range pass.Files { + if f.Pos() <= err.Pos && err.Pos < f.End() { + file = f + break + } + } + if file == nil { + return + } + + // Get the path for the relevant range. + path, _ := astutil.PathEnclosingInterval(file, err.Pos, err.Pos) + if len(path) < 2 { + return + } + ident, ok := path[0].(*ast.Ident) + if !ok || ident.Name != name { + return + } + + // Undeclared quick fixes only work in function bodies. + inFunc := false + for i := range path { + if _, inFunc = path[i].(*ast.FuncDecl); inFunc { + if i == 0 { + return + } + if _, isBody := path[i-1].(*ast.BlockStmt); !isBody { + return + } + break + } + } + if !inFunc { + return + } + // Skip selector expressions because it might be too complex + // to try and provide a suggested fix for fields and methods. + if _, ok := path[1].(*ast.SelectorExpr); ok { + return + } + tok := pass.Fset.File(file.Pos()) + if tok == nil { + return + } + offset := pass.Fset.Position(err.Pos).Offset + end := tok.Pos(offset + len(name)) + pass.Report(analysis.Diagnostic{ + Pos: err.Pos, + End: end, + Message: err.Msg, + }) +} + func SuggestedFix(fset *token.FileSet, rng span.Range, content []byte, file *ast.File, pkg *types.Package, info *types.Info) (*analysis.SuggestedFix, error) { pos := rng.Start // don't use the end path, _ := astutil.PathEnclosingInterval(file, pos, pos) if len(path) < 2 { - return nil, fmt.Errorf("") + return nil, fmt.Errorf("no expression found") } ident, ok := path[0].(*ast.Ident) if !ok { - return nil, fmt.Errorf("") + return nil, fmt.Errorf("no identifier found") } // Check for a possible call expression, in which case we should add a @@ -115,7 +136,7 @@ func SuggestedFix(fset *token.FileSet, rng span.Range, content []byte, file *ast // Get the place to insert the new statement. insertBeforeStmt := analysisinternal.StmtToInsertVarBefore(path) if insertBeforeStmt == nil { - return nil, fmt.Errorf("") + return nil, fmt.Errorf("could not locate insertion point") } insertBefore := fset.Position(insertBeforeStmt.Pos()).Offset @@ -139,10 +160,6 @@ func SuggestedFix(fset *token.FileSet, rng span.Range, content []byte, file *ast }, nil } -func FixesError(msg string) bool { - return strings.HasPrefix(msg, undeclaredNamePrefix) -} - func newFunctionDeclaration(path []ast.Node, file *ast.File, pkg *types.Package, info *types.Info, fset *token.FileSet) (*analysis.SuggestedFix, error) { if len(path) < 3 { return nil, fmt.Errorf("unexpected set of enclosing nodes: %v", path)