gopls/internal/test/integration: simplify GoToDefinition

- Definition no longer calls OpenFile on the first location;
  it merely returns the list. Also, rename to Definitions.
- GoToDefinition is renamed FirstDefinition; it asserts
  that the list is non-empty.
- Tests that assumed OpenFile now call it explicitly.

These changes should make it easier to write both positive
and negative marker tests of the Definitions result set.

Change-Id: I1e57bba8c78983f7b266ad876b1862b8af86a797
Reviewed-on: https://go-review.googlesource.com/c/tools/+/675016
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Madeline Kalil <[email protected]>

Author: adonovan

gopls/internal/test/integration/bench/definition_test.go

@@ -31,15 +31,15 @@ func BenchmarkDefinition(b *testing.B) {
 
 			loc := env.RegexpSearch(test.file, test.regexp)
 			env.Await(env.DoneWithOpen())
-			env.GoToDefinition(loc) // pre-warm the query, and open the target file
+			env.FirstDefinition(loc) // pre-warm the query, and open the target file
 			b.ResetTimer()
 
 			if stopAndRecord := startProfileIfSupported(b, env, qualifiedName(test.repo, "definition")); stopAndRecord != nil {
 				defer stopAndRecord()
 			}
 
 			for b.Loop() {
-				env.GoToDefinition(loc) // pre-warm the query
+				env.FirstDefinition(loc) // pre-warm the query
 			}
 		})
 	}

gopls/internal/test/integration/diagnostics/builtin_test.go

@@ -26,7 +26,7 @@ const (
 `
 	Run(t, src, func(t *testing.T, env *Env) {
 		env.OpenFile("a.go")
-		loc := env.GoToDefinition(env.RegexpSearch("a.go", "iota"))
+		loc := env.FirstDefinition(env.RegexpSearch("a.go", "iota"))
 		if !strings.HasSuffix(string(loc.URI), "builtin.go") {
 			t.Fatalf("jumped to %q, want builtin.go", loc.URI)
 		}

gopls/internal/test/integration/diagnostics/diagnostics_test.go

@@ -1790,7 +1790,7 @@ func helloHelper() {}
 		env.AfterChange(
 			NoDiagnostics(ForFile("nested/hello/hello.go")),
 		)
-		loc := env.GoToDefinition(env.RegexpSearch("nested/hello/hello.go", "helloHelper"))
+		loc := env.FirstDefinition(env.RegexpSearch("nested/hello/hello.go", "helloHelper"))
 		want := "nested/hello/hello_helper.go"
 		if got := env.Sandbox.Workdir.URIToPath(loc.URI); got != want {
 			t.Errorf("Definition() returned %q, want %q", got, want)

gopls/internal/test/integration/fake/editor.go

@@ -932,54 +932,30 @@ func (e *Editor) setBufferContentLocked(ctx context.Context, path string, dirty
 	return nil
 }
 
-// GoToDefinition jumps to the definition of the symbol at the given position
-// in an open buffer. It returns the location of the resulting jump.
-func (e *Editor) Definition(ctx context.Context, loc protocol.Location) (protocol.Location, error) {
+// Definitions returns the definitions of the symbol at the given
+// location in an open buffer.
+func (e *Editor) Definitions(ctx context.Context, loc protocol.Location) ([]protocol.Location, error) {
 	if err := e.checkBufferLocation(loc); err != nil {
-		return protocol.Location{}, err
+		return nil, err
 	}
 	params := &protocol.DefinitionParams{}
 	params.TextDocument.URI = loc.URI
 	params.Position = loc.Range.Start
 
-	resp, err := e.Server.Definition(ctx, params)
-	if err != nil {
-		return protocol.Location{}, fmt.Errorf("definition: %w", err)
-	}
-	return e.extractFirstLocation(ctx, resp)
+	return e.Server.Definition(ctx, params)
 }
 
-// TypeDefinition jumps to the type definition of the symbol at the given
-// location in an open buffer.
-func (e *Editor) TypeDefinition(ctx context.Context, loc protocol.Location) (protocol.Location, error) {
+// TypeDefinitions returns the type definitions of the symbol at the
+// given location in an open buffer.
+func (e *Editor) TypeDefinitions(ctx context.Context, loc protocol.Location) ([]protocol.Location, error) {
 	if err := e.checkBufferLocation(loc); err != nil {
-		return protocol.Location{}, err
+		return nil, err
 	}
 	params := &protocol.TypeDefinitionParams{}
 	params.TextDocument.URI = loc.URI
 	params.Position = loc.Range.Start
 
-	resp, err := e.Server.TypeDefinition(ctx, params)
-	if err != nil {
-		return protocol.Location{}, fmt.Errorf("type definition: %w", err)
-	}
-	return e.extractFirstLocation(ctx, resp)
-}
-
-// extractFirstLocation returns the first location.
-// It opens the file if needed.
-func (e *Editor) extractFirstLocation(ctx context.Context, locs []protocol.Location) (protocol.Location, error) {
-	if len(locs) == 0 {
-		return protocol.Location{}, nil
-	}
-
-	newPath := e.sandbox.Workdir.URIToPath(locs[0].URI)
-	if !e.HasBuffer(newPath) {
-		if err := e.OpenFile(ctx, newPath); err != nil {
-			return protocol.Location{}, fmt.Errorf("OpenFile: %w", err)
-		}
-	}
-	return locs[0], nil
+	return e.Server.TypeDefinition(ctx, params)
 }
 
 // Symbol performs a workspace symbol search using query

gopls/internal/test/integration/misc/definition_test.go

@@ -41,13 +41,13 @@ const message = "Hello World."
 func TestGoToInternalDefinition(t *testing.T) {
 	Run(t, internalDefinition, func(t *testing.T, env *Env) {
 		env.OpenFile("main.go")
-		loc := env.GoToDefinition(env.RegexpSearch("main.go", "message"))
+		loc := env.FirstDefinition(env.RegexpSearch("main.go", "message"))
 		name := env.Sandbox.Workdir.URIToPath(loc.URI)
 		if want := "const.go"; name != want {
-			t.Errorf("GoToDefinition: got file %q, want %q", name, want)
+			t.Errorf("Definition: got file %q, want %q", name, want)
 		}
 		if want := env.RegexpSearch("const.go", "message"); loc != want {
-			t.Errorf("GoToDefinition: got location %v, want %v", loc, want)
+			t.Errorf("Definition: got location %v, want %v", loc, want)
 		}
 	})
 }
@@ -90,13 +90,13 @@ func TestGoToLinknameDefinition(t *testing.T) {
 
 		// Jump from directives 2nd arg.
 		start := env.RegexpSearch("upper/upper.go", `lower.bar`)
-		loc := env.GoToDefinition(start)
+		loc := env.FirstDefinition(start)
 		name := env.Sandbox.Workdir.URIToPath(loc.URI)
 		if want := "lower/lower.go"; name != want {
-			t.Errorf("GoToDefinition: got file %q, want %q", name, want)
+			t.Errorf("Definition: got file %q, want %q", name, want)
 		}
 		if want := env.RegexpSearch("lower/lower.go", `bar`); loc != want {
-			t.Errorf("GoToDefinition: got position %v, want %v", loc, want)
+			t.Errorf("Definition: got position %v, want %v", loc, want)
 		}
 	})
 }
@@ -139,13 +139,13 @@ func TestGoToLinknameDefinitionInReverseDep(t *testing.T) {
 
 		// Jump from directives 2nd arg.
 		start := env.RegexpSearch("lower/lower.go", `upper.foo`)
-		loc := env.GoToDefinition(start)
+		loc := env.FirstDefinition(start)
 		name := env.Sandbox.Workdir.URIToPath(loc.URI)
 		if want := "upper/upper.go"; name != want {
-			t.Errorf("GoToDefinition: got file %q, want %q", name, want)
+			t.Errorf("Definition: got file %q, want %q", name, want)
 		}
 		if want := env.RegexpSearch("upper/upper.go", `foo`); loc != want {
-			t.Errorf("GoToDefinition: got position %v, want %v", loc, want)
+			t.Errorf("Definition: got position %v, want %v", loc, want)
 		}
 	})
 }
@@ -178,13 +178,13 @@ func TestGoToLinknameDefinitionDisconnected(t *testing.T) {
 
 		// Jump from directives 2nd arg.
 		start := env.RegexpSearch("a/a.go", `b.bar`)
-		loc := env.GoToDefinition(start)
+		loc := env.FirstDefinition(start)
 		name := env.Sandbox.Workdir.URIToPath(loc.URI)
 		if want := "b/b.go"; name != want {
-			t.Errorf("GoToDefinition: got file %q, want %q", name, want)
+			t.Errorf("Definition: got file %q, want %q", name, want)
 		}
 		if want := env.RegexpSearch("b/b.go", `bar`); loc != want {
-			t.Errorf("GoToDefinition: got position %v, want %v", loc, want)
+			t.Errorf("Definition: got position %v, want %v", loc, want)
 		}
 	})
 }
@@ -206,30 +206,32 @@ func main() {
 func TestGoToStdlibDefinition_Issue37045(t *testing.T) {
 	Run(t, stdlibDefinition, func(t *testing.T, env *Env) {
 		env.OpenFile("main.go")
-		loc := env.GoToDefinition(env.RegexpSearch("main.go", `fmt.(Printf)`))
+		loc := env.FirstDefinition(env.RegexpSearch("main.go", `fmt.(Printf)`))
 		name := env.Sandbox.Workdir.URIToPath(loc.URI)
 		if got, want := path.Base(name), "print.go"; got != want {
-			t.Errorf("GoToDefinition: got file %q, want %q", name, want)
+			t.Errorf("Definition: got file %q, want %q", name, want)
 		}
+		env.OpenFile(name)
 
 		// Test that we can jump to definition from outside our workspace.
 		// See golang.org/issues/37045.
-		newLoc := env.GoToDefinition(loc)
+		newLoc := env.FirstDefinition(loc)
 		newName := env.Sandbox.Workdir.URIToPath(newLoc.URI)
 		if newName != name {
-			t.Errorf("GoToDefinition is not idempotent: got %q, want %q", newName, name)
+			t.Errorf("Definition is not idempotent: got %q, want %q", newName, name)
 		}
 		if newLoc != loc {
-			t.Errorf("GoToDefinition is not idempotent: got %v, want %v", newLoc, loc)
+			t.Errorf("Definition is not idempotent: got %v, want %v", newLoc, loc)
 		}
 	})
 }
 
 func TestUnexportedStdlib_Issue40809(t *testing.T) {
 	Run(t, stdlibDefinition, func(t *testing.T, env *Env) {
 		env.OpenFile("main.go")
-		loc := env.GoToDefinition(env.RegexpSearch("main.go", `fmt.(Printf)`))
+		loc := env.FirstDefinition(env.RegexpSearch("main.go", `fmt.(Printf)`))
 		name := env.Sandbox.Workdir.URIToPath(loc.URI)
+		env.OpenFile(name)
 
 		loc = env.RegexpSearch(name, `:=\s*(newPrinter)\(\)`)
 
@@ -239,7 +241,7 @@ func TestUnexportedStdlib_Issue40809(t *testing.T) {
 			t.Errorf("expected 5+ references to newPrinter, found: %#v", refs)
 		}
 
-		loc = env.GoToDefinition(loc)
+		loc = env.FirstDefinition(loc)
 		content, _ := env.Hover(loc)
 		if !strings.Contains(content.Value, "newPrinter") {
 			t.Fatal("definition of newPrinter went to the incorrect place")
@@ -306,7 +308,7 @@ func main() {}
 				Settings{"importShortcut": tt.importShortcut},
 			).Run(t, mod, func(t *testing.T, env *Env) {
 				env.OpenFile("main.go")
-				loc := env.GoToDefinition(env.RegexpSearch("main.go", `"fmt"`))
+				loc := env.FirstDefinition(env.RegexpSearch("main.go", `"fmt"`))
 				if loc == (protocol.Location{}) {
 					t.Fatalf("expected definition, got none")
 				}
@@ -354,7 +356,7 @@ func main() {}
 			Run(t, mod, func(t *testing.T, env *Env) {
 				env.OpenFile("main.go")
 
-				loc, err := env.Editor.TypeDefinition(env.Ctx, env.RegexpSearch("main.go", tt.re))
+				locs, err := env.Editor.TypeDefinitions(env.Ctx, env.RegexpSearch("main.go", tt.re))
 				if tt.wantError {
 					if err == nil {
 						t.Fatal("expected error, got nil")
@@ -364,10 +366,13 @@ func main() {}
 				if err != nil {
 					t.Fatalf("expected nil error, got %s", err)
 				}
+				if len(locs) == 0 {
+					t.Fatalf("TypeDefinitions: empty result")
+				}
 
 				typeLoc := env.RegexpSearch("main.go", tt.wantTypeRe)
-				if loc != typeLoc {
-					t.Errorf("invalid pos: want %+v, got %+v", typeLoc, loc)
+				if locs[0] != typeLoc {
+					t.Errorf("invalid pos: want %+v, got %+v", typeLoc, locs[0])
 				}
 			})
 		})
@@ -389,7 +394,16 @@ func F[T comparable]() {}
 	Run(t, mod, func(t *testing.T, env *Env) {
 		env.OpenFile("main.go")
 
-		_ = env.TypeDefinition(env.RegexpSearch("main.go", "comparable")) // must not panic
+		// TypeDefinition of comparable should
+		// returns an empty result, not panic.
+		loc := env.RegexpSearch("main.go", "comparable")
+		locs, err := env.Editor.TypeDefinitions(env.Ctx, loc)
+		if err != nil {
+			t.Fatal(err)
+		}
+		if len(locs) > 0 {
+			t.Fatalf("unexpected result: %v", locs)
+		}
 	})
 }
 
@@ -429,7 +443,7 @@ package client
 `
 	Run(t, mod, func(t *testing.T, env *Env) {
 		env.OpenFile("client/client_role_test.go")
-		env.GoToDefinition(env.RegexpSearch("client/client_role_test.go", "RoleSetup"))
+		env.FirstDefinition(env.RegexpSearch("client/client_role_test.go", "RoleSetup"))
 	})
 }
 
@@ -487,11 +501,11 @@ const _ = b.K
 		refLoc := env.RegexpSearch("a.go", "K") // find "b.K" reference
 
 		// Initially, b.K is defined in the module cache.
-		gotLoc := env.GoToDefinition(refLoc)
+		gotLoc := env.FirstDefinition(refLoc)
 		gotFile := env.Sandbox.Workdir.URIToPath(gotLoc.URI)
 		wantCache := filepath.ToSlash(env.Sandbox.GOPATH()) + "/pkg/mod/other.com/[email protected]/b.go"
 		if gotFile != wantCache {
-			t.Errorf("GoToDefinition, before: got file %q, want %q", gotFile, wantCache)
+			t.Errorf("Definition, before: got file %q, want %q", gotFile, wantCache)
 		}
 
 		// Run 'go mod vendor' outside the editor.
@@ -501,10 +515,10 @@ const _ = b.K
 		env.Await(env.DoneWithChangeWatchedFiles())
 
 		// Now, b.K is defined in the vendor tree.
-		gotLoc = env.GoToDefinition(refLoc)
+		gotLoc = env.FirstDefinition(refLoc)
 		wantVendor := "vendor/other.com/b/b.go"
 		if gotFile != wantVendor {
-			t.Errorf("GoToDefinition, after go mod vendor: got file %q, want %q", gotFile, wantVendor)
+			t.Errorf("Definition, after go mod vendor: got file %q, want %q", gotFile, wantVendor)
 		}
 
 		// Delete the vendor tree.
@@ -520,10 +534,10 @@ const _ = b.K
 		env.Await(env.DoneWithChangeWatchedFiles())
 
 		// b.K is once again defined in the module cache.
-		gotLoc = env.GoToDefinition(gotLoc)
+		gotLoc = env.FirstDefinition(gotLoc)
 		gotFile = env.Sandbox.Workdir.URIToPath(gotLoc.URI)
 		if gotFile != wantCache {
-			t.Errorf("GoToDefinition, after rm -rf vendor: got file %q, want %q", gotFile, wantCache)
+			t.Errorf("Definition, after rm -rf vendor: got file %q, want %q", gotFile, wantCache)
 		}
 	})
 }
@@ -554,16 +568,16 @@ FOO
 SKIP
 `
 
-func TestGoToEmbedDefinition(t *testing.T) {
+func TestEmbedDefinition(t *testing.T) {
 	Run(t, embedDefinition, func(t *testing.T, env *Env) {
 		env.OpenFile("main.go")
 
 		start := env.RegexpSearch("main.go", `\*.txt`)
-		loc := env.GoToDefinition(start)
+		loc := env.FirstDefinition(start)
 
 		name := env.Sandbox.Workdir.URIToPath(loc.URI)
 		if want := "foo.txt"; name != want {
-			t.Errorf("GoToDefinition: got file %q, want %q", name, want)
+			t.Errorf("Definition: got file %q, want %q", name, want)
 		}
 	})
 }
@@ -588,10 +602,10 @@ func _(err error) {
 		env.OpenFile("a.go")
 
 		start := env.RegexpSearch("a.go", `Error`)
-		loc := env.GoToDefinition(start)
+		loc := env.FirstDefinition(start)
 
 		if !strings.HasSuffix(string(loc.URI), "builtin.go") {
-			t.Errorf("GoToDefinition(err.Error) = %#v, want builtin.go", loc)
+			t.Errorf("Definition(err.Error) = %#v, want builtin.go", loc)
 		}
 	})
 }
@@ -628,13 +642,13 @@ var _ = foo(123) // call
 
 		// Definition at the call"foo(123)" takes us to the Go declaration.
 		callLoc := env.RegexpSearch("a.go", regexp.QuoteMeta("foo(123)"))
-		declLoc := env.GoToDefinition(callLoc)
+		declLoc := env.FirstDefinition(callLoc)
 		if got, want := locString(declLoc), "a.go:5:5-5:8"; got != want {
 			t.Errorf("Definition(call): got %s, want %s", got, want)
 		}
 
 		// Definition a second time takes us to the assembly implementation.
-		implLoc := env.GoToDefinition(declLoc)
+		implLoc := env.FirstDefinition(declLoc)
 		if got, want := locString(implLoc), "foo_darwin_arm64.s:2:6-2:9"; got != want {
 			t.Errorf("Definition(go decl): got %s, want %s", got, want)
 		}
@@ -670,7 +684,7 @@ func Foo() {
 		env.OpenFile("a.go")
 
 		fooLoc := env.RegexpSearch("a.go", "()Foo")
-		loc0 := env.GoToDefinition(fooLoc)
+		loc0 := env.FirstDefinition(fooLoc)
 
 		// Insert a space that will be removed by formatting.
 		env.EditBuffer("a.go", protocol.TextEdit{
@@ -679,7 +693,7 @@ func Foo() {
 		})
 		env.SaveBuffer("a.go") // reformats the file before save
 		env.AfterChange()
-		loc1 := env.GoToDefinition(env.RegexpSearch("a.go", "Foo"))
+		loc1 := env.FirstDefinition(env.RegexpSearch("a.go", "Foo"))
 		if diff := cmp.Diff(loc0, loc1); diff != "" {
 			t.Errorf("mismatching locations (-want +got):\n%s", diff)
 		}