From 1ffc3a165d1c666f785bfb67033ee87eb79b31c2 Mon Sep 17 00:00:00 2001 From: Rob Findley Date: Thu, 21 Nov 2024 15:55:30 +0000 Subject: [PATCH] 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 LUCI-TryBot-Result: Go LUCI --- gopls/internal/test/marker/doc.go | 58 ++++---- gopls/internal/test/marker/marker_test.go | 133 +++++++++++------- .../marker/testdata/implementation/basic.txt | 6 + .../testdata/workspacesymbol/allscope.txt | 2 +- 4 files changed, 127 insertions(+), 72 deletions(-) diff --git a/gopls/internal/test/marker/doc.go b/gopls/internal/test/marker/doc.go index ff77786e00f..620773c7026 100644 --- a/gopls/internal/test/marker/doc.go +++ b/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 diff --git a/gopls/internal/test/marker/marker_test.go b/gopls/internal/test/marker/marker_test.go index bfea22e934a..f6ccc228a72 100644 --- a/gopls/internal/test/marker/marker_test.go +++ b/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 "" } - 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 "" + 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 "" } - 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 "" } + 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 +// +// ::[-[:]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 = "" } diff --git a/gopls/internal/test/marker/testdata/implementation/basic.txt b/gopls/internal/test/marker/testdata/implementation/basic.txt index 3f63a5d00c1..28522cb5bc4 100644 --- a/gopls/internal/test/marker/testdata/implementation/basic.txt +++ b/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 diff --git a/gopls/internal/test/marker/testdata/workspacesymbol/allscope.txt b/gopls/internal/test/marker/testdata/workspacesymbol/allscope.txt index 18fe4e5446f..645a9c967c9 100644 --- a/gopls/internal/test/marker/testdata/workspacesymbol/allscope.txt +++ b/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 - fmt.Println Function + fmt.Println Function