From 933c7ccb15451459ca4fe53c041a4108f4859d91 Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Thu, 25 May 2023 16:26:39 -0400 Subject: [PATCH] internal/lsp/source: use exact match in import highlighting Previously, hovering on a package name such as http.XYZ would highlight its import path ("net/http"), but also any other one that contained it as a substring, such as "net/http/httptest". (This behavior was there from the outset in CL 215258, but wasn't remarked upon during the review.) This change uses exact matching based on type-checker objects, not strings,, adds a test of same, and clarifies the logic. Fixes golang/go#60435 Change-Id: I9cc07dbcdaf54707d17be2a162bfcb0a22aa440a Reviewed-on: https://go-review.googlesource.com/c/tools/+/498268 Auto-Submit: Alan Donovan Reviewed-by: Robert Findley Run-TryBot: Alan Donovan TryBot-Result: Gopher Robot --- gopls/internal/lsp/source/highlight.go | 114 +++++++++--------- gopls/internal/lsp/source/util.go | 8 +- gopls/internal/lsp/source/xrefs/xrefs.go | 11 +- .../lsp/testdata/highlights/issue60435.go | 14 +++ .../internal/lsp/testdata/summary.txt.golden | 2 +- .../lsp/testdata/summary_go1.18.txt.golden | 2 +- .../lsp/testdata/summary_go1.21.txt.golden | 2 +- 7 files changed, 76 insertions(+), 77 deletions(-) create mode 100644 gopls/internal/lsp/testdata/highlights/issue60435.go diff --git a/gopls/internal/lsp/source/highlight.go b/gopls/internal/lsp/source/highlight.go index ad13d253df9..adfc659e20c 100644 --- a/gopls/internal/lsp/source/highlight.go +++ b/gopls/internal/lsp/source/highlight.go @@ -10,7 +10,6 @@ import ( "go/ast" "go/token" "go/types" - "strings" "golang.org/x/tools/go/ast/astutil" "golang.org/x/tools/gopls/internal/lsp/protocol" @@ -67,10 +66,27 @@ func highlightPath(path []ast.Node, file *ast.File, info *types.Info) (map[posRa result := make(map[posRange]struct{}) switch node := path[0].(type) { case *ast.BasicLit: + // Import path string literal? if len(path) > 1 { - if _, ok := path[1].(*ast.ImportSpec); ok { - err := highlightImportUses(path, info, result) - return result, err + if imp, ok := path[1].(*ast.ImportSpec); ok { + highlight := func(n ast.Node) { + result[posRange{start: n.Pos(), end: n.End()}] = struct{}{} + } + + // Highlight the import itself... + highlight(imp) + + // ...and all references to it in the file. + if pkgname, ok := ImportedPkgName(info, imp); ok { + ast.Inspect(file, func(n ast.Node) bool { + if id, ok := n.(*ast.Ident); ok && + info.Uses[id] == pkgname { + highlight(id) + } + return true + }) + } + return result, nil } } highlightFuncControlFlow(path, result) @@ -419,66 +435,46 @@ Outer: }) } -func highlightImportUses(path []ast.Node, info *types.Info, result map[posRange]struct{}) error { - basicLit, ok := path[0].(*ast.BasicLit) - if !ok { - return fmt.Errorf("highlightImportUses called with an ast.Node of type %T", basicLit) - } - ast.Inspect(path[len(path)-1], func(node ast.Node) bool { - if imp, ok := node.(*ast.ImportSpec); ok && imp.Path == basicLit { - result[posRange{start: node.Pos(), end: node.End()}] = struct{}{} - return false - } - n, ok := node.(*ast.Ident) - if !ok { - return true - } - obj, ok := info.ObjectOf(n).(*types.PkgName) - if !ok { - return true - } - if !strings.Contains(basicLit.Value, obj.Name()) { - return true - } +func highlightIdentifier(id *ast.Ident, file *ast.File, info *types.Info, result map[posRange]struct{}) { + highlight := func(n ast.Node) { result[posRange{start: n.Pos(), end: n.End()}] = struct{}{} - return false - }) - return nil -} + } -func highlightIdentifier(id *ast.Ident, file *ast.File, info *types.Info, result map[posRange]struct{}) { - // TODO(rfindley): idObj may be nil. Note that returning early in this case - // causes tests to fail (because the nObj == idObj check below was succeeded - // for nil == nil!) - // - // Revisit this. If ObjectOf is nil, there are type errors, and it seems - // reasonable for identifier highlighting not to work. - idObj := info.ObjectOf(id) - pkgObj, isImported := idObj.(*types.PkgName) - ast.Inspect(file, func(node ast.Node) bool { - if imp, ok := node.(*ast.ImportSpec); ok && isImported { - highlightImport(pkgObj, imp, result) - } - n, ok := node.(*ast.Ident) - if !ok { - return true - } - if n.Name != id.Name { - return false - } - if nObj := info.ObjectOf(n); nObj == idObj { - result[posRange{start: n.Pos(), end: n.End()}] = struct{}{} + // obj may be nil if the Ident is undefined. + // In this case, the behavior expected by tests is + // to match other undefined Idents of the same name. + obj := info.ObjectOf(id) + + ast.Inspect(file, func(n ast.Node) bool { + switch n := n.(type) { + case *ast.Ident: + if n.Name == id.Name && info.ObjectOf(n) == obj { + highlight(n) + } + + case *ast.ImportSpec: + pkgname, ok := ImportedPkgName(info, n) + if ok && pkgname == obj { + if n.Name != nil { + highlight(n.Name) + } else { + highlight(n) + } + } } - return false + return true }) } -func highlightImport(obj *types.PkgName, imp *ast.ImportSpec, result map[posRange]struct{}) { - if imp.Name != nil || imp.Path == nil { - return - } - if !strings.Contains(imp.Path.Value, obj.Name()) { - return +// ImportedPkgName returns the PkgName object declared by an ImportSpec. +// TODO(adonovan): make this a method of types.Info. +func ImportedPkgName(info *types.Info, imp *ast.ImportSpec) (*types.PkgName, bool) { + var obj types.Object + if imp.Name != nil { + obj = info.Defs[imp.Name] + } else { + obj = info.Implicits[imp] } - result[posRange{start: imp.Path.Pos(), end: imp.Path.End()}] = struct{}{} + pkgname, ok := obj.(*types.PkgName) + return pkgname, ok } diff --git a/gopls/internal/lsp/source/util.go b/gopls/internal/lsp/source/util.go index cbb17809496..d0ecd50ac6b 100644 --- a/gopls/internal/lsp/source/util.go +++ b/gopls/internal/lsp/source/util.go @@ -255,13 +255,7 @@ func Qualifier(f *ast.File, pkg *types.Package, info *types.Info) types.Qualifie // Construct mapping of import paths to their defined or implicit names. imports := make(map[*types.Package]string) for _, imp := range f.Imports { - var obj types.Object - if imp.Name != nil { - obj = info.Defs[imp.Name] - } else { - obj = info.Implicits[imp] - } - if pkgname, ok := obj.(*types.PkgName); ok { + if pkgname, ok := ImportedPkgName(info, imp); ok { imports[pkgname.Imported()] = pkgname.Name() } } diff --git a/gopls/internal/lsp/source/xrefs/xrefs.go b/gopls/internal/lsp/source/xrefs/xrefs.go index 6231f888430..36463c26972 100644 --- a/gopls/internal/lsp/source/xrefs/xrefs.go +++ b/gopls/internal/lsp/source/xrefs/xrefs.go @@ -87,16 +87,11 @@ func Index(files []*source.ParsedGoFile, pkg *types.Package, info *types.Info) [ case *ast.ImportSpec: // Report a reference from each import path // string to the imported package. - var obj types.Object - if n.Name != nil { - obj = info.Defs[n.Name] - } else { - obj = info.Implicits[n] - } - if obj == nil { + pkgname, ok := source.ImportedPkgName(info, n) + if !ok { return true // missing import } - objects := getObjects(obj.(*types.PkgName).Imported()) + objects := getObjects(pkgname.Imported()) gobObj, ok := objects[nil] if !ok { gobObj = &gobObject{Path: ""} diff --git a/gopls/internal/lsp/testdata/highlights/issue60435.go b/gopls/internal/lsp/testdata/highlights/issue60435.go new file mode 100644 index 00000000000..de0070e5832 --- /dev/null +++ b/gopls/internal/lsp/testdata/highlights/issue60435.go @@ -0,0 +1,14 @@ +package highlights + +import ( + "net/http" //@mark(httpImp, `"net/http"`) + "net/http/httptest" //@mark(httptestImp, `"net/http/httptest"`) +) + +// This is a regression test for issue 60435: +// Highlighting "net/http" shouldn't have any effect +// on an import path that contains it as a substring, +// such as httptest. + +var _ = httptest.NewRequest +var _ = http.NewRequest //@mark(here, "http"), highlight(here, here, httpImp) diff --git a/gopls/internal/lsp/testdata/summary.txt.golden b/gopls/internal/lsp/testdata/summary.txt.golden index c572e268f7f..e6cee0c9b3c 100644 --- a/gopls/internal/lsp/testdata/summary.txt.golden +++ b/gopls/internal/lsp/testdata/summary.txt.golden @@ -15,7 +15,7 @@ SuggestedFixCount = 73 MethodExtractionCount = 6 DefinitionsCount = 46 TypeDefinitionsCount = 18 -HighlightsCount = 69 +HighlightsCount = 70 InlayHintsCount = 4 RenamesCount = 41 PrepareRenamesCount = 7 diff --git a/gopls/internal/lsp/testdata/summary_go1.18.txt.golden b/gopls/internal/lsp/testdata/summary_go1.18.txt.golden index da3b553834c..4d847b42511 100644 --- a/gopls/internal/lsp/testdata/summary_go1.18.txt.golden +++ b/gopls/internal/lsp/testdata/summary_go1.18.txt.golden @@ -15,7 +15,7 @@ SuggestedFixCount = 79 MethodExtractionCount = 6 DefinitionsCount = 46 TypeDefinitionsCount = 18 -HighlightsCount = 69 +HighlightsCount = 70 InlayHintsCount = 5 RenamesCount = 48 PrepareRenamesCount = 7 diff --git a/gopls/internal/lsp/testdata/summary_go1.21.txt.golden b/gopls/internal/lsp/testdata/summary_go1.21.txt.golden index 52fba365236..9c1b504ab7c 100644 --- a/gopls/internal/lsp/testdata/summary_go1.21.txt.golden +++ b/gopls/internal/lsp/testdata/summary_go1.21.txt.golden @@ -15,7 +15,7 @@ SuggestedFixCount = 79 MethodExtractionCount = 6 DefinitionsCount = 46 TypeDefinitionsCount = 18 -HighlightsCount = 69 +HighlightsCount = 70 InlayHintsCount = 5 RenamesCount = 48 PrepareRenamesCount = 7