gopls/internal/lsp/source: fix crash in definitions of builtins

Definition queries on built-ins should report the pseudo-package
"builtin", and queries on unsafe.Pointer et al should report
the real (if irregular) package "unsafe". This CL fixes a bug
wherein the latter was crashing. The solution required synthesizing
Metadata("unsafe").GoFiles=["unsafe/unsafe.go"] from the filename
in builtins, because unfortunately go/packages discards it.

Also, references queries of builtins and unsafe.Pointer should be
rejected with a clear error.

The symbol tests were tweaked to handle the new matches in
the unsafe package. Better ideas welcome.

Fixes golang/go#43058

Change-Id: I9d7cd7ffcb679d77f9859e39b240a8eb605a267b
Reviewed-on: https://go-review.googlesource.com/c/tools/+/491036
TryBot-Result: Gopher Robot <[email protected]>
Run-TryBot: Alan Donovan <[email protected]>
Reviewed-by: Robert Findley <[email protected]>

Author: adonovan

gopls/internal/lsp/cache/load.go

@@ -144,6 +144,31 @@ func (s *snapshot) load(ctx context.Context, allowNetwork bool, scopes ...loadSc
 		return fmt.Errorf("packages.Load error: %w", err)
 	}
 
+	// Workaround for a bug (?) that has been in go/packages since
+	// the outset: Package("unsafe").GoFiles=[], whereas it should
+	// include unsafe/unsafe.go. Derive it from builtins.go.
+	//
+	// This workaround relies on the fact that we always add both
+	// builtins and unsafe to the set of scopes in the workspace load.
+	//
+	// TODO(adonovan): fix upstream in go/packages.
+	// (Does this need a proposal? Arguably not.)
+	{
+		var builtin, unsafe *packages.Package
+		for _, pkg := range pkgs {
+			if pkg.ID == "unsafe" {
+				unsafe = pkg
+			} else if pkg.ID == "builtin" {
+				builtin = pkg
+			}
+		}
+		if builtin != nil && unsafe != nil && len(builtin.GoFiles) == 1 {
+			unsafe.GoFiles = []string{
+				filepath.Join(filepath.Dir(builtin.GoFiles[0]), "../unsafe/unsafe.go"),
+			}
+		}
+	}
+
 	moduleErrs := make(map[string][]packages.Error) // module path -> errors
 	filterFunc := s.view.filterFunc()
 	newMetadata := make(map[PackageID]*source.Metadata)
@@ -453,6 +478,7 @@ func buildMetadata(ctx context.Context, pkg *packages.Package, cfg *packages.Con
 		//
 		// We should consider doing a more complete guard against import cycles
 		// elsewhere.
+		// [Update: we do! breakImportCycles in metadataGraph.Clone. Delete this.]
 		for _, prev := range path {
 			if prev == id {
 				return fmt.Errorf("import cycle detected: %q", id)

gopls/internal/lsp/cache/view.go

@@ -741,8 +741,10 @@ func (s *snapshot) loadWorkspace(ctx context.Context, firstAttempt bool) (loadEr
 	// If we're loading anything, ensure we also load builtin,
 	// since it provides fake definitions (and documentation)
 	// for types like int that are used everywhere.
+	// ("unsafe" is also needed since its sole GoFiles is
+	// derived from that of "builtin" via a workaround in load.)
 	if len(scopes) > 0 {
-		scopes = append(scopes, packageLoadScope("builtin"))
+		scopes = append(scopes, packageLoadScope("builtin"), packageLoadScope("unsafe"))
 	}
 	loadErr = s.load(ctx, true, scopes...)
 

gopls/internal/lsp/source/definition.go

@@ -64,42 +64,61 @@ func Definition(ctx context.Context, snapshot Snapshot, fh FileHandle, position
 		return nil, nil
 	}
 
-	// Handle built-in identifiers.
-	if obj.Parent() == types.Universe {
-		builtin, err := snapshot.BuiltinFile(ctx)
-		if err != nil {
-			return nil, err
+	// Handle objects with no position: builtin, unsafe.
+	if !obj.Pos().IsValid() {
+		var pgf *ParsedGoFile
+		if obj.Parent() == types.Universe {
+			// pseudo-package "builtin"
+			builtinPGF, err := snapshot.BuiltinFile(ctx)
+			if err != nil {
+				return nil, err
+			}
+			pgf = builtinPGF
+
+		} else if obj.Pkg() == types.Unsafe {
+			// package "unsafe"
+			unsafe := snapshot.Metadata("unsafe")
+			if unsafe == nil {
+				return nil, fmt.Errorf("no metadata for package 'unsafe'")
+			}
+			uri := unsafe.GoFiles[0]
+			fh, err := snapshot.ReadFile(ctx, uri)
+			if err != nil {
+				return nil, err
+			}
+			pgf, err = snapshot.ParseGo(ctx, fh, ParseFull&^SkipObjectResolution)
+			if err != nil {
+				return nil, err
+			}
+
+		} else {
+			return nil, bug.Errorf("internal error: no position for %v", obj.Name())
 		}
-		// Note that builtinObj is an ast.Object, not types.Object :)
-		builtinObj := builtin.File.Scope.Lookup(obj.Name())
-		if builtinObj == nil {
-			// Every builtin should have documentation.
-			return nil, bug.Errorf("internal error: no builtin object for %s", obj.Name())
+		// Inv: pgf ∈ {builtin,unsafe}.go
+
+		// Use legacy (go/ast) object resolution.
+		astObj := pgf.File.Scope.Lookup(obj.Name())
+		if astObj == nil {
+			// Every built-in should have documentation syntax.
+			return nil, bug.Errorf("internal error: no object for %s", obj.Name())
 		}
-		decl, ok := builtinObj.Decl.(ast.Node)
+		decl, ok := astObj.Decl.(ast.Node)
 		if !ok {
 			return nil, bug.Errorf("internal error: no declaration for %s", obj.Name())
 		}
-		// The builtin package isn't in the dependency graph, so the usual
-		// utilities won't work here.
-		loc, err := builtin.PosLocation(decl.Pos(), decl.Pos()+token.Pos(len(obj.Name())))
+		loc, err := pgf.PosLocation(decl.Pos(), decl.Pos()+token.Pos(len(obj.Name())))
 		if err != nil {
 			return nil, err
 		}
 		return []protocol.Location{loc}, nil
 	}
 
 	// Finally, map the object position.
-	var locs []protocol.Location
-	if !obj.Pos().IsValid() {
-		return nil, bug.Errorf("internal error: no position for %v", obj.Name())
-	}
 	loc, err := mapPosition(ctx, pkg.FileSet(), snapshot, obj.Pos(), adjustedObjEnd(obj))
 	if err != nil {
 		return nil, err
 	}
-	locs = append(locs, loc)
-	return locs, nil
+	return []protocol.Location{loc}, nil
 }
 
 // referencedObject returns the identifier and object referenced at the

gopls/internal/lsp/source/hover.go

@@ -364,8 +364,6 @@ func hoverBuiltin(ctx context.Context, snapshot Snapshot, obj types.Object) (*Ho
 		return nil, err
 	}
 
-	// TODO(rfindley): add a test for jump to definition of error.Error (which is
-	// probably failing, considering it lacks special handling).
 	if obj.Name() == "Error" {
 		signature := obj.String()
 		return &HoverJSON{

gopls/internal/lsp/source/references.go

@@ -246,14 +246,14 @@ func ordinaryReferences(ctx context.Context, snapshot Snapshot, uri span.URI, pp
 
 	// nil, error, error.Error, iota, or other built-in?
 	if obj.Pkg() == nil {
-		// For some reason, existing tests require that iota has no references,
-		// nor an error. TODO(adonovan): do something more principled.
-		if obj.Name() == "iota" {
-			return nil, nil
-		}
-
 		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/regtest/marker/testdata/references/iota.txt

@@ -100,6 +100,60 @@ func _() {
 	})
 }
 
+func TestDefsRefsBuiltins(t *testing.T) {
+	testenv.NeedsGo1Point(t, 17) // for unsafe.{Add,Slice}
+	// TODO(adonovan): add unsafe.SliceData,String,StringData} in later go versions.
+	const files = `
+-- go.mod --
+module example.com
+go 1.16
+
+-- a.go --
+package a
+
+import "unsafe"
+
+const _ = iota
+var _ error
+var _ int
+var _ = append()
+var _ = unsafe.Pointer(nil)
+var _ = unsafe.Add(nil, nil)
+var _ = unsafe.Sizeof(0)
+var _ = unsafe.Alignof(0)
+var _ = unsafe.Slice(nil, 0)
+`
+
+	Run(t, files, func(t *testing.T, env *Env) {
+		env.OpenFile("a.go")
+		for _, name := range strings.Fields(
+			"iota error int nil append iota Pointer Sizeof Alignof Add Slice") {
+			loc := env.RegexpSearch("a.go", `\b`+name+`\b`)
+
+			// definition -> {builtin,unsafe}.go
+			def, err := env.Editor.GoToDefinition(env.Ctx, loc)
+			if err != nil {
+				t.Errorf("definition(%q) failed: %v", name, err)
+			} else if (!strings.HasSuffix(string(def.URI), "builtin.go") &&
+				!strings.HasSuffix(string(def.URI), "unsafe.go")) ||
+				def.Range.Start.Line == 0 {
+				t.Errorf("definition(%q) = %v, want {builtin,unsafe}.go",
+					name, def)
+			}
+
+			// "references to (builtin "Foo"|unsafe.Foo) are not supported"
+			_, err = env.Editor.References(env.Ctx, loc)
+			gotErr := fmt.Sprint(err)
+			if !strings.Contains(gotErr, "references to") ||
+				!strings.Contains(gotErr, "not supported") ||
+				!strings.Contains(gotErr, name) {
+				t.Errorf("references(%q) error: got %q, want %q",
+					name, gotErr, "references to ... are not supported")
+			}
+		}
+	})
+}
+
 func TestPackageReferences(t *testing.T) {
 	tests := []struct {
 		packageName  string

gopls/internal/regtest/misc/references_test.go

@@ -7,7 +7,7 @@ package misc
 import (
 	"testing"
 
-	"golang.org/x/tools/gopls/internal/lsp/protocol"
+	"github.com/google/go-cmp/cmp"
 	. "golang.org/x/tools/gopls/internal/lsp/regtest"
 	"golang.org/x/tools/gopls/internal/lsp/source"
 )
@@ -21,31 +21,27 @@ go 1.17
 -- a.go --
 package p
 
-const C1 = "a.go"
+const K1 = "a.go"
 -- exclude.go --
 
 //go:build exclude
 // +build exclude
 
 package exclude
 
-const C2 = "exclude.go"
+const K2 = "exclude.go"
 `
 
+	// NB: the name K was chosen to avoid spurious
+	// matches in the always-present "unsafe" package.
 	Run(t, files, func(t *testing.T, env *Env) {
 		env.OpenFile("a.go")
-		syms := env.Symbol("C")
-		if got, want := len(syms), 1; got != want {
-			t.Errorf("got %d symbols, want %d", got, want)
-		}
+		checkSymbols(env, "K", "K1")
 
 		// Opening up an ignored file will result in an overlay with missing
 		// metadata, but this shouldn't break workspace symbols requests.
 		env.OpenFile("exclude.go")
-		syms = env.Symbol("C")
-		if got, want := len(syms), 1; got != want {
-			t.Errorf("got %d symbols, want %d", got, want)
-		}
+		checkSymbols(env, "K", "K1")
 	})
 }
 
@@ -71,15 +67,14 @@ const (
 	WithOptions(
 		Settings{"symbolMatcher": symbolMatcher},
 	).Run(t, files, func(t *testing.T, env *Env) {
-		want := []string{
+		checkSymbols(env, "Foo",
 			"Foo",    // prefer exact segment matches first
 			"FooBar", // ...followed by exact word matches
 			"Fooex",  // shorter than Fooest, FooBar, lexically before Fooey
 			"Fooey",  // shorter than Fooest, Foobar
 			"Fooest",
-		}
-		got := env.Symbol("Foo")
-		compareSymbols(t, got, want...)
+			"unsafe.Offsetof", // a very fuzzy match
+		)
 	})
 }
 
@@ -102,23 +97,21 @@ const (
 	WithOptions(
 		Settings{"symbolMatcher": symbolMatcher},
 	).Run(t, files, func(t *testing.T, env *Env) {
-		compareSymbols(t, env.Symbol("ABC"), "ABC", "AxxBxxCxx")
-		compareSymbols(t, env.Symbol("'ABC"), "ABC")
-		compareSymbols(t, env.Symbol("^mod.com"), "mod.com/a.ABC", "mod.com/a.AxxBxxCxx")
-		compareSymbols(t, env.Symbol("^mod.com Axx"), "mod.com/a.AxxBxxCxx")
-		compareSymbols(t, env.Symbol("C$"), "ABC")
+		checkSymbols(env, "ABC", "ABC", "AxxBxxCxx")
+		checkSymbols(env, "'ABC", "ABC")
+		checkSymbols(env, "^mod.com", "mod.com/a.ABC", "mod.com/a.AxxBxxCxx")
+		checkSymbols(env, "^mod.com Axx", "mod.com/a.AxxBxxCxx")
+		checkSymbols(env, "C$", "ABC")
 	})
 }
 
-func compareSymbols(t *testing.T, got []protocol.SymbolInformation, want ...string) {
-	t.Helper()
-	if len(got) != len(want) {
-		t.Errorf("got %d symbols, want %d", len(got), len(want))
+func checkSymbols(env *Env, query string, want ...string) {
+	env.T.Helper()
+	var got []string
+	for _, info := range env.Symbol(query) {
+		got = append(got, info.Name)
 	}
-
-	for i := range got {
-		if got[i].Name != want[i] {
-			t.Errorf("got[%d] = %q, want %q", i, got[i].Name, want[i])
-		}
+	if diff := cmp.Diff(got, want); diff != "" {
+		env.T.Errorf("unexpected Symbol(%q) result (+want -got):\n%s", query, diff)
 	}
 }