gopls/internal/test/marker: add defloc, to bind positions by definition

Address a long-standing TODO in the marker tests by adding a new value
marker @defloc, which binds a name to the result of a definition
request.

Also
- more documentation improvements
- add support for formatting positions outside of the test archive
- refactor location formatting, to better handle external locations

Change-Id: I33e0d0e1a5d6d58cd83c62b8ad9b50e147077e2e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/630555
Reviewed-by: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>

gopls/internal/test/marker/doc.go

@@ -102,13 +102,41 @@ treatment by the test runner:
 
 # Marker types
 
-Markers are of two kinds. A few are "value markers" (e.g. @item), which are
-processed in a first pass and each computes a value that may be referred to
-by name later. Most are "action markers", which are processed in a second
-pass and take some action such as testing an LSP operation; they may refer
-to values computed by value markers.
+Markers are of two kinds: "value markers" and "action markers". Value markers
+are processed in a first pass, and define named values that may be referred to
+as arguments to action markers. For example, the @loc marker defines a named
+location that may be used wherever a location is expected. Value markers cannot
+refer to names defined by other value markers. Action markers are processed in
+a second pass and perform some action such as testing an LSP operation.
 
-The following markers are supported within marker tests:
+Below, we list supported markers using function signatures, augmented with the
+named argument support name=value, as described above. The types referred to in
+the signatures below are described in the Argument conversion section.
+
+Here is the list of supported value markers:
+
+  - loc(name, location): specifies the name for a location in the source. These
+    locations may be referenced by other markers. Naturally, the location
+    argument may be specified only as a string or regular expression in the
+    first pass.
+
+  - defloc(name, location): performs a textDocument/defintiion request at the
+    src location, and binds the result to the given name. This may be used to
+    refer to positions in the standard library.
+
+  - hiloc(name, location, kind): defines a documentHighlight value of the
+    given location and kind. Use its label in a @highlightall marker to
+    indicate the expected result of a highlight query.
+
+  - item(name, details, kind): defines a completionItem with the provided
+    fields. This information is not positional, and therefore @item markers
+    may occur anywhere in the source. Use in conjunction with @complete,
+    @snippet, or @rank.
+
+    TODO(rfindley): rethink whether floating @item annotations are the best
+    way to specify completion results.
+
+Here is the list of supported action markers:
 
   - acceptcompletion(location, label, golden): specifies that accepting the
     completion candidate produced at the given location with provided label
@@ -177,10 +205,6 @@ The following markers are supported within marker tests:
     textDocument/highlight request at the given src location, which should
     highlight the provided dst locations and kinds.
 
-  - hiloc(label, location, kind): defines a documentHighlight value of the
-    given location and kind. Use its label in a @highlightall marker to
-    indicate the expected result of a highlight query.
-
   - hover(src, dst location, sm stringMatcher): performs a textDocument/hover
     at the src location, and checks that the result is the dst location, with
     matching hover content.
@@ -198,17 +222,6 @@ The following markers are supported within marker tests:
     (These locations are the declarations of the functions enclosing
     the calls, not the calls themselves.)
 
-  - item(label, details, kind): defines a completionItem with the provided
-    fields. This information is not positional, and therefore @item markers
-    may occur anywhere in the source. Used in conjunction with @complete,
-    @snippet, or @rank.
-
-    TODO(rfindley): rethink whether floating @item annotations are the best
-    way to specify completion results.
-
-  - loc(name, location): specifies the name for a location in the source. These
-    locations may be referenced by other markers.
-
   - outgoingcalls(src location, want ...location): makes a
     callHierarchy/outgoingCalls query at the src location, and checks that
     the set of call.To locations matches want.
@@ -382,9 +395,6 @@ Note that -update does not cause missing @diag or @loc markers to be added.
 # TODO
 
   - Rename the files .txtar.
-  - Provide some means by which locations in the standard library
-    (or builtin.go) can be named, so that, for example, we can we
-    can assert that MyError implements the built-in error type.
   - Eliminate all *err markers, preferring named arguments.
 */
 package marker

gopls/internal/test/marker/marker_test.go

@@ -112,6 +112,7 @@ func Test(t *testing.T) {
 		test := test
 		t.Run(test.name, func(t *testing.T) {
 			t.Parallel()
+
 			if test.skipReason != "" {
 				t.Skip(test.skipReason)
 			}
@@ -154,6 +155,7 @@ func Test(t *testing.T) {
 				}
 				testenv.NeedsTool(t, "cgo")
 			}
+
 			config := fake.EditorConfig{
 				Settings:         test.settings,
 				CapabilitiesJSON: test.capabilities,
@@ -177,6 +179,7 @@ func Test(t *testing.T) {
 				diags:      make(map[protocol.Location][]protocol.Diagnostic),
 				extraNotes: make(map[protocol.DocumentURI]map[string][]*expect.Note),
 			}
+
 			// TODO(rfindley): make it easier to clean up the integration test environment.
 			defer run.env.Editor.Shutdown(context.Background()) // ignore error
 			defer run.env.Sandbox.Close()                       // ignore error
@@ -346,7 +349,16 @@ func (mark marker) mapper() *protocol.Mapper {
 	return mapper
 }
 
-// errorf reports an error with a prefix indicating the position of the marker note.
+// error reports an error with a prefix indicating the position of the marker
+// note.
+func (mark marker) error(args ...any) {
+	mark.T().Helper()
+	msg := fmt.Sprint(args...)
+	mark.T().Errorf("%s: %s", mark.run.fmtPos(mark.note.Pos), msg)
+}
+
+// errorf reports a formatted error with a prefix indicating the position of
+// the marker note.
 //
 // It formats the error message using mark.sprintf.
 func (mark marker) errorf(format string, args ...any) {
@@ -402,7 +414,7 @@ func valueMarkerFunc(fn any) func(marker) {
 		args := append([]any{mark}, mark.note.Args[1:]...)
 		argValues, err := convertArgs(mark, ftype, args)
 		if err != nil {
-			mark.errorf("converting args: %v", err)
+			mark.error(err)
 			return
 		}
 		results := reflect.ValueOf(fn).Call(argValues)
@@ -445,7 +457,7 @@ func actionMarkerFunc(fn any, allowedNames ...string) func(marker) {
 		args := append([]any{mark}, mark.note.Args...)
 		argValues, err := convertArgs(mark, ftype, args)
 		if err != nil {
-			mark.errorf("converting args: %v", err)
+			mark.error(err)
 			return
 		}
 		reflect.ValueOf(fn).Call(argValues)
@@ -540,9 +552,10 @@ func is[T any](arg any) bool {
 
 // Supported value marker functions. See [valueMarkerFunc] for more details.
 var valueMarkerFuncs = map[string]func(marker){
-	"loc":   valueMarkerFunc(locMarker),
-	"item":  valueMarkerFunc(completionItemMarker),
-	"hiloc": valueMarkerFunc(highlightLocationMarker),
+	"loc":    valueMarkerFunc(locMarker),
+	"item":   valueMarkerFunc(completionItemMarker),
+	"hiloc":  valueMarkerFunc(highlightLocationMarker),
+	"defloc": valueMarkerFunc(defLocMarker),
 }
 
 // Supported action marker functions. See [actionMarkerFunc] for more details.
@@ -1029,22 +1042,10 @@ func (run *markerTestRun) fmtPos(pos token.Pos) string {
 // archive-relative paths for files and including the line number in the full
 // archive file.
 func (run *markerTestRun) fmtLoc(loc protocol.Location) string {
-	formatted := run.fmtLocDetails(loc, true)
-	if formatted == "" {
+	if loc == (protocol.Location{}) {
 		run.env.T.Errorf("unable to find %s in test archive", loc)
 		return "<invalid location>"
 	}
-	return formatted
-}
-
-// See fmtLoc. If includeTxtPos is not set, the position in the full archive
-// file is omitted.
-//
-// If the location cannot be found within the archive, fmtLocDetails returns "".
-func (run *markerTestRun) fmtLocDetails(loc protocol.Location, includeTxtPos bool) string {
-	if loc == (protocol.Location{}) {
-		return ""
-	}
 	lines := bytes.Count(run.test.archive.Comment, []byte("\n"))
 	var name string
 	for _, f := range run.test.archive.Files {
@@ -1057,39 +1058,74 @@ func (run *markerTestRun) fmtLocDetails(loc protocol.Location, includeTxtPos boo
 		lines += bytes.Count(f.Data, []byte("\n"))
 	}
 	if name == "" {
-		return ""
-	}
+		// Fall back to formatting the "lsp" location.
+		// These will be in UTF-16, but we probably don't need to clarify that,
+		// since it will be implied by the file:// URI format.
+		return summarizeLoc(string(loc.URI),
+			int(loc.Range.Start.Line), int(loc.Range.Start.Character),
+			int(loc.Range.End.Line), int(loc.Range.End.Character))
+	}
+	name, startLine, startCol, endLine, endCol := run.mapLocation(loc)
+	innerSpan := summarizeLoc(name, startLine, startCol, endLine, endCol)
+	outerSpan := summarizeLoc(run.test.name, lines+startLine, startCol, lines+endLine, endCol)
+	return fmt.Sprintf("%s (%s)", innerSpan, outerSpan)
+}
+
+// mapLocation returns the relative path and utf8 span of the corresponding
+// location, which must be a valid location in an archive file.
+func (run *markerTestRun) mapLocation(loc protocol.Location) (name string, startLine, startCol, endLine, endCol int) {
+	// Note: Editor.Mapper fails if loc.URI is not open, but we always open all
+	// archive files, so this is probably OK.
+	//
+	// In the future, we may want to have the editor read contents from disk if
+	// the URI is not open.
+	name = run.env.Sandbox.Workdir.URIToPath(loc.URI)
 	m, err := run.env.Editor.Mapper(name)
 	if err != nil {
 		run.env.T.Errorf("internal error: %v", err)
-		return "<invalid location>"
+		return
 	}
 	start, end, err := m.RangeOffsets(loc.Range)
 	if err != nil {
 		run.env.T.Errorf("error formatting location %s: %v", loc, err)
+		return
+	}
+	startLine, startCol = m.OffsetLineCol8(start)
+	endLine, endCol = m.OffsetLineCol8(end)
+	return name, startLine, startCol, endLine, endCol
+}
+
+// fmtLocForGolden is like fmtLoc, but chooses more succinct and stable
+// formatting, such as would be used for formatting locations in Golden
+// content.
+func (run *markerTestRun) fmtLocForGolden(loc protocol.Location) string {
+	if loc == (protocol.Location{}) {
 		return "<invalid location>"
 	}
-	var (
-		startLine, startCol8 = m.OffsetLineCol8(start)
-		endLine, endCol8     = m.OffsetLineCol8(end)
-	)
-	innerSpan := fmt.Sprintf("%d:%d", startLine, startCol8)       // relative to the embedded file
-	outerSpan := fmt.Sprintf("%d:%d", lines+startLine, startCol8) // relative to the archive file
-	if start != end {
-		if endLine == startLine {
-			innerSpan += fmt.Sprintf("-%d", endCol8)
-			outerSpan += fmt.Sprintf("-%d", endCol8)
-		} else {
-			innerSpan += fmt.Sprintf("-%d:%d", endLine, endCol8)
-			outerSpan += fmt.Sprintf("-%d:%d", lines+endLine, endCol8)
-		}
+	name := run.env.Sandbox.Workdir.URIToPath(loc.URI)
+	// Note: we check IsAbs on filepaths rather than the slash-ified name for
+	// accurate handling of windows drive letters.
+	if filepath.IsAbs(filepath.FromSlash(name)) {
+		// Don't format any position information in this case, since it will be
+		// volatile.
+		return "<external>"
 	}
+	return summarizeLoc(run.mapLocation(loc))
+}
 
-	if includeTxtPos {
-		return fmt.Sprintf("%s:%s (%s:%s)", name, innerSpan, run.test.name, outerSpan)
-	} else {
-		return fmt.Sprintf("%s:%s", name, innerSpan)
+// summarizeLoc formats a summary of the given location, in the form
+//
+//	<name>:<startLine>:<startCol>[-[<endLine>:]endCol]
+func summarizeLoc(name string, startLine, startCol, endLine, endCol int) string {
+	span := fmt.Sprintf("%s:%d:%d", name, startLine, startCol)
+	if startLine != endLine || startCol != endCol {
+		span += "-"
+		if endLine != startLine {
+			span += fmt.Sprintf("%d:", endLine)
+		}
+		span += fmt.Sprintf("%d", endCol)
 	}
+	return span
 }
 
 // ---- converters ----
@@ -1144,7 +1180,7 @@ func convert(mark marker, arg any, paramType reflect.Type) (any, error) {
 	if converter, ok := customConverters[paramType]; ok {
 		arg2, err := converter(mark, arg)
 		if err != nil {
-			return nil, fmt.Errorf("converting for input type %T to %v: %v", arg, paramType, err)
+			return nil, err
 		}
 		arg = arg2
 	}
@@ -1763,10 +1799,15 @@ func hoverErrMarker(mark marker, src protocol.Location, em stringMatcher) {
 	em.checkErr(mark, err)
 }
 
-// locMarker implements the @loc marker. It is executed before other
-// markers, so that locations are available.
+// locMarker implements the @loc marker.
 func locMarker(mark marker, loc protocol.Location) protocol.Location { return loc }
 
+// defLocMarker implements the @defloc marker, which binds a location to the
+// (first) result of a jump-to-definition request.
+func defLocMarker(mark marker, loc protocol.Location) protocol.Location {
+	return mark.run.env.GoToDefinition(loc)
+}
+
 // diagMarker implements the @diag marker. It eliminates diagnostics from
 // the observed set in mark.test.
 func diagMarker(mark marker, loc protocol.Location, re *regexp.Regexp) {
@@ -2101,7 +2142,7 @@ func documentLinkMarker(mark marker, g *Golden) {
 			continue
 		}
 		loc := protocol.Location{URI: mark.uri(), Range: l.Range}
-		fmt.Fprintln(&b, mark.run.fmtLocDetails(loc, false), *l.Target)
+		fmt.Fprintln(&b, mark.run.fmtLocForGolden(loc), *l.Target)
 	}
 
 	compareGolden(mark, b.Bytes(), g)
@@ -2554,9 +2595,7 @@ func workspaceSymbolMarker(mark marker, query string, golden *Golden) {
 	for _, s := range gotSymbols {
 		// Omit the txtar position of the symbol location; otherwise edits to the
 		// txtar archive lead to unexpected failures.
-		loc := mark.run.fmtLocDetails(s.Location, false)
-		// TODO(rfindley): can we do better here, by detecting if the location is
-		// relative to GOROOT?
+		loc := mark.run.fmtLocForGolden(s.Location)
 		if loc == "" {
 			loc = "<unknown>"
 		}

gopls/internal/test/marker/testdata/implementation/basic.txt

@@ -43,6 +43,12 @@ type embedsImpP struct { //@loc(embedsImpP, "embedsImpP")
 	ImpP //@implementation("ImpP", Laugher, OtherLaugher)
 }
 
+var _ error //@defloc(StdError, "error")
+
+type MyError struct {} //@implementation("MyError", StdError)
+
+func (MyError) Error() string { return "bah" }
+
 -- other/other.go --
 package other
 

gopls/internal/test/marker/testdata/workspacesymbol/allscope.txt

@@ -27,4 +27,4 @@ func Println(s string) {
 }
 -- @println --
 fmt/fmt.go:5:6-13 mod.test/symbols/fmt.Println Function
-<unknown> fmt.Println Function
+<external> fmt.Println Function