gopls/internal/golang: OutgoingCalls: fix crash on unsafe.Slice

Also, audit the golang package for similar places where we
discriminate builtins, and make them all use the new isBuiltin
helper, which is based on lack of a position instead of messing
around with pkg==nil||pkg==types.Unsafe||...
"A symbol without a position" is a fair definition of a built-in.

Fixes golang/go#66923

Change-Id: I7f94b8d0f865f8c079f1164fd61121eefbb40522
Reviewed-on: https://go-review.googlesource.com/c/tools/+/588937
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Robert Findley <[email protected]>
Auto-Submit: Alan Donovan <[email protected]>

gopls/internal/cache/parsego/file.go

@@ -41,10 +41,12 @@ type File struct {
 	ParseErr scanner.ErrorList
 }
 
+func (pgf File) String() string { return string(pgf.URI) }
+
 // Fixed reports whether p was "Fixed", meaning that its source or positions
 // may not correlate with the original file.
-func (p File) Fixed() bool {
-	return p.fixedSrc || p.fixedAST
+func (pgf File) Fixed() bool {
+	return pgf.fixedSrc || pgf.fixedAST
 }
 
 // -- go/token domain convenience helpers --

gopls/internal/golang/call_hierarchy.go

@@ -201,13 +201,8 @@ func OutgoingCalls(ctx context.Context, snapshot *cache.Snapshot, fh file.Handle
 		return nil, nil
 	}
 
-	// Skip builtins.
-	if obj.Pkg() == nil {
-		return nil, nil
-	}
-
-	if !obj.Pos().IsValid() {
-		return nil, bug.Errorf("internal error: object %s.%s missing position", obj.Pkg().Path(), obj.Name())
+	if isBuiltin(obj) {
+		return nil, nil // built-ins have no position
 	}
 
 	declFile := pkg.FileSet().File(obj.Pos())
@@ -271,10 +266,8 @@ func OutgoingCalls(ctx context.Context, snapshot *cache.Snapshot, fh file.Handle
 		if obj == nil {
 			continue
 		}
-
-		// ignore calls to builtin functions
-		if obj.Pkg() == nil {
-			continue
+		if isBuiltin(obj) {
+			continue // built-ins have no position
 		}
 
 		outgoingCall, ok := outgoingCalls[obj.Pos()]

gopls/internal/golang/definition.go

@@ -87,8 +87,8 @@ func Definition(ctx context.Context, snapshot *cache.Snapshot, fh file.Handle, p
 		return nil, nil
 	}
 
-	// Handle objects with no position: builtin, unsafe.
-	if !obj.Pos().IsValid() {
+	// Built-ins have no position.
+	if isBuiltin(obj) {
 		return builtinDefinition(ctx, snapshot, obj)
 	}
 

gopls/internal/golang/hover.go

@@ -169,12 +169,14 @@ func hover(ctx context.Context, snapshot *cache.Snapshot, fh file.Handle, pp pro
 
 	// Handle hovering over a doc link
 	if obj, rng, _ := parseDocLink(pkg, pgf, pos); obj != nil {
-		hoverRange = &rng
-		// Handle builtins, which don't have a package or position.
-		if !obj.Pos().IsValid() {
+		// Built-ins have no position.
+		if isBuiltin(obj) {
 			h, err := hoverBuiltin(ctx, snapshot, obj)
-			return *hoverRange, h, err
+			return rng, h, err
 		}
+
+		// Find position in declaring file.
+		hoverRange = &rng
 		objURI := safetoken.StartPosition(pkg.FileSet(), obj.Pos())
 		pkg, pgf, err = NarrowestPackageForFile(ctx, snapshot, protocol.URIFromPath(objURI.Filename))
 		if err != nil {
@@ -240,8 +242,8 @@ func hover(ctx context.Context, snapshot *cache.Snapshot, fh file.Handle, pp pro
 		}, nil
 	}
 
-	// Handle builtins, which don't have a package or position.
-	if !obj.Pos().IsValid() {
+	if isBuiltin(obj) {
+		// Built-ins have no position.
 		h, err := hoverBuiltin(ctx, snapshot, obj)
 		return *hoverRange, h, err
 	}

gopls/internal/golang/references.go

@@ -252,15 +252,9 @@ func ordinaryReferences(ctx context.Context, snapshot *cache.Snapshot, uri proto
 	}
 
 	// nil, error, error.Error, iota, or other built-in?
-	if obj.Pkg() == nil {
+	if isBuiltin(obj) {
 		return nil, fmt.Errorf("references to builtin %q are not supported", obj.Name())
 	}
-	if !obj.Pos().IsValid() {
-		if obj.Pkg().Path() != "unsafe" {
-			bug.Reportf("references: object %v has no position", obj)
-		}
-		return nil, fmt.Errorf("references to unsafe.%s are not supported", obj.Name())
-	}
 
 	// Find metadata of all packages containing the object's defining file.
 	// This may include the query pkg, and possibly other variants.

gopls/internal/golang/signature_help.go

@@ -99,9 +99,7 @@ FindCall:
 	case *ast.SelectorExpr:
 		obj = info.ObjectOf(t.Sel)
 	}
-
-	// Call to built-in?
-	if obj != nil && !obj.Pos().IsValid() {
+	if obj != nil && isBuiltin(obj) {
 		// function?
 		if obj, ok := obj.(*types.Builtin); ok {
 			return builtinSignature(ctx, snapshot, callExpr, obj.Name(), pos)

gopls/internal/golang/type_definition.go

@@ -12,7 +12,6 @@ import (
 	"golang.org/x/tools/gopls/internal/cache"
 	"golang.org/x/tools/gopls/internal/file"
 	"golang.org/x/tools/gopls/internal/protocol"
-	"golang.org/x/tools/gopls/internal/util/bug"
 	"golang.org/x/tools/internal/event"
 )
 
@@ -42,13 +41,8 @@ func TypeDefinition(ctx context.Context, snapshot *cache.Snapshot, fh file.Handl
 	if tname == nil {
 		return nil, fmt.Errorf("no type definition for %s", obj.Name())
 	}
-
-	if !tname.Pos().IsValid() {
-		// The only defined types with no position are error and comparable.
-		if tname.Name() != "error" && tname.Name() != "comparable" {
-			bug.Reportf("unexpected type name with no position: %s", tname)
-		}
-		return nil, nil
+	if isBuiltin(tname) {
+		return nil, nil // built-ins (error, comparable) have no position
 	}
 
 	loc, err := mapPosition(ctx, pkg.FileSet(), snapshot, tname.Pos(), tname.Pos()+token.Pos(len(tname.Name())))

gopls/internal/golang/types_format.go

@@ -283,7 +283,7 @@ func FormatVarType(ctx context.Context, snapshot *cache.Snapshot, srcpkg *cache.
 		return types.TypeString(obj.Type(), qf), nil
 	}
 
-	if obj.Pkg() == nil || !obj.Pos().IsValid() {
+	if isBuiltin(obj) {
 		// This is defensive, though it is extremely unlikely we'll ever have a
 		// builtin var.
 		return types.TypeString(obj.Type(), qf), nil

gopls/internal/golang/util.go

@@ -350,3 +350,7 @@ func embeddedIdent(x ast.Expr) *ast.Ident {
 type ImporterFunc func(path string) (*types.Package, error)
 
 func (f ImporterFunc) Import(path string) (*types.Package, error) { return f(path) }
+
+// isBuiltin reports whether obj is a built-in symbol (e.g. append, iota, error.Error, unsafe.Slice).
+// All other symbols have a valid position and a valid package.
+func isBuiltin(obj types.Object) bool { return !obj.Pos().IsValid() }

gopls/internal/test/marker/testdata/callhierarchy/issue66923.txt

@@ -0,0 +1,15 @@
+Regression test for a crash (#66923) in outgoing calls
+to a built-in function (unsafe.Slice).
+
+-- go.mod --
+module example.com
+go 1.17
+
+-- a/a.go --
+package a
+
+import "unsafe"
+
+func A() []int {			 //@ loc(A, "A")
+	return unsafe.Slice(new(int), 1) //@ outgoingcalls(A)
+}