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 <adonovan@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Run-TryBot: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>

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
 }

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()
 		}
 	}

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: ""}

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)

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

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

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