From 9a89d3a9850fd26cb9aee85127007257ea7ed951 Mon Sep 17 00:00:00 2001 From: Rob Findley Date: Wed, 6 Nov 2024 16:54:09 +0000 Subject: [PATCH] internal/analysisinternal: avoid sub-token spans in TypeErrorEndPos Avoid assigning end positions to type errors that are within the current token (such as could happen in an import path). To test this, introduce our first named argument in the marker tests: 'exact' for the @diag marker. Also, attempt to document the heuristic of TypeErrorEndPos. Fixes golang/go#69505 Change-Id: If3cf82f241dd354d834a7dcbf24b7b3c59246911 Reviewed-on: https://go-review.googlesource.com/c/tools/+/625916 Auto-Submit: Robert Findley LUCI-TryBot-Result: Go LUCI Reviewed-by: Alan Donovan --- gopls/internal/test/marker/doc.go | 10 +-- gopls/internal/test/marker/marker_test.go | 61 ++++++++++++++----- .../testdata/diagnostics/issue69505.txt | 22 +++++++ internal/analysisinternal/analysis.go | 45 ++++++++++++-- 4 files changed, 114 insertions(+), 24 deletions(-) create mode 100644 gopls/internal/test/marker/testdata/diagnostics/issue69505.txt diff --git a/gopls/internal/test/marker/doc.go b/gopls/internal/test/marker/doc.go index 00ee8655b20..509791d509c 100644 --- a/gopls/internal/test/marker/doc.go +++ b/gopls/internal/test/marker/doc.go @@ -127,13 +127,13 @@ The following markers are supported within marker tests: - complete(location, ...items): specifies expected completion results at the given location. Must be used in conjunction with @item. - - diag(location, regexp): specifies an expected diagnostic matching the - given regexp at the given location. The test runner requires - a 1:1 correspondence between observed diagnostics and diag annotations. - The diagnostics source and kind fields are ignored, to reduce fuss. + - diag(location, regexp, exact=bool): specifies an expected diagnostic + matching the given regexp at the given location. The test runner requires a + 1:1 correspondence between observed diagnostics and diag annotations. The + diagnostics source and kind fields are ignored, to reduce fuss. The specified location must match the start position of the diagnostic, - but end positions are ignored. + but end positions are ignored unless exact=true. TODO(adonovan): in the older marker framework, the annotation asserted two additional fields (source="compiler", kind="error"). Restore them? diff --git a/gopls/internal/test/marker/marker_test.go b/gopls/internal/test/marker/marker_test.go index cb0e9303897..272809c3384 100644 --- a/gopls/internal/test/marker/marker_test.go +++ b/gopls/internal/test/marker/marker_test.go @@ -205,8 +205,6 @@ func Test(t *testing.T) { } } - // Wait for the didOpen notifications to be processed, then collect - // diagnostics. for path, diags := range allDiags { uri := run.env.Sandbox.Workdir.URI(path) for _, diag := range diags { @@ -246,7 +244,13 @@ func Test(t *testing.T) { if !test.ignoreExtraDiags { for loc, diags := range run.diags { for _, diag := range diags { - t.Errorf("%s: unexpected diagnostic: %q", run.fmtLoc(loc), diag.Message) + // Note that loc is collapsed (start==end). + // For formatting, show the exact span. + exactLoc := protocol.Location{ + URI: loc.URI, + Range: diag.Range, + } + t.Errorf("%s: unexpected diagnostic: %q", run.fmtLoc(exactLoc), diag.Message) } } } @@ -404,10 +408,11 @@ func valueMarkerFunc(fn any) func(marker) { // called during the processing of action markers (e.g. @action("abc", 123)) // with marker arguments converted to function parameters. The provided // function's first parameter must be of type 'marker', and it must not return -// any values. +// any values. Any named arguments that may be used by the marker func must be +// listed in allowedNames. // // The provided fn should not mutate the test environment. -func actionMarkerFunc(fn any) func(marker) { +func actionMarkerFunc(fn any, allowedNames ...string) func(marker) { ftype := reflect.TypeOf(fn) if ftype.NumIn() == 0 || ftype.In(0) != markerType { panic(fmt.Sprintf("action marker function %#v must accept marker as its first argument", ftype)) @@ -416,7 +421,21 @@ func actionMarkerFunc(fn any) func(marker) { panic(fmt.Sprintf("action marker function %#v cannot have results", ftype)) } + var allowed map[string]bool + if len(allowedNames) > 0 { + allowed = make(map[string]bool) + for _, name := range allowedNames { + allowed[name] = true + } + } + return func(mark marker) { + for name := range mark.note.NamedArgs { + if !allowed[name] { + mark.errorf("unexpected named argument %q", name) + } + } + args := append([]any{mark}, mark.note.Args...) argValues, err := convertArgs(mark, ftype, args) if err != nil { @@ -470,6 +489,18 @@ func convertArgs(mark marker, ftype reflect.Type, args []any) ([]reflect.Value, return argValues, nil } +// namedArg returns the named argument for name, or the default value. +func namedArg[T any](mark marker, name string, dflt T) T { + if v, ok := mark.note.NamedArgs[name]; ok { + if e, ok := v.(T); ok { + return e + } else { + mark.errorf("invalid value for %q: %v", name, v) + } + } + return dflt +} + // is reports whether arg is a T. func is[T any](arg any) bool { _, ok := arg.(T) @@ -492,7 +523,7 @@ var actionMarkerFuncs = map[string]func(marker){ "codelenses": actionMarkerFunc(codeLensesMarker), "complete": actionMarkerFunc(completeMarker), "def": actionMarkerFunc(defMarker), - "diag": actionMarkerFunc(diagMarker), + "diag": actionMarkerFunc(diagMarker, "exact"), "documentlink": actionMarkerFunc(documentLinkMarker), "foldingrange": actionMarkerFunc(foldingRangeMarker), "format": actionMarkerFunc(formatMarker), @@ -1659,7 +1690,8 @@ func locMarker(mark marker, loc protocol.Location) protocol.Location { return lo // 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) { - if _, ok := removeDiagnostic(mark, loc, re); !ok { + exact := namedArg(mark, "exact", false) + if _, ok := removeDiagnostic(mark, loc, exact, re); !ok { mark.errorf("no diagnostic at %v matches %q", loc, re) } } @@ -1670,12 +1702,13 @@ func diagMarker(mark marker, loc protocol.Location, re *regexp.Regexp) { // from the unmatched set. // // If not found, it returns (protocol.Diagnostic{}, false). -func removeDiagnostic(mark marker, loc protocol.Location, re *regexp.Regexp) (protocol.Diagnostic, bool) { - loc.Range.End = loc.Range.Start // diagnostics ignore end position. - diags := mark.run.diags[loc] +func removeDiagnostic(mark marker, loc protocol.Location, matchEnd bool, re *regexp.Regexp) (protocol.Diagnostic, bool) { + key := loc + key.Range.End = key.Range.Start // diagnostics ignore end position. + diags := mark.run.diags[key] for i, diag := range diags { - if re.MatchString(diag.Message) { - mark.run.diags[loc] = append(diags[:i], diags[i+1:]...) + if re.MatchString(diag.Message) && (!matchEnd || diag.Range.End == loc.Range.End) { + mark.run.diags[key] = append(diags[:i], diags[i+1:]...) return diag, true } } @@ -2007,7 +2040,7 @@ func (mark marker) consumeExtraNotes(name string, f func(marker)) { func quickfixMarker(mark marker, loc protocol.Location, re *regexp.Regexp, golden *Golden) { loc.Range.End = loc.Range.Start // diagnostics ignore end position. // Find and remove the matching diagnostic. - diag, ok := removeDiagnostic(mark, loc, re) + diag, ok := removeDiagnostic(mark, loc, false, re) if !ok { mark.errorf("no diagnostic at %v matches %q", loc, re) return @@ -2027,7 +2060,7 @@ func quickfixMarker(mark marker, loc protocol.Location, re *regexp.Regexp, golde func quickfixErrMarker(mark marker, loc protocol.Location, re *regexp.Regexp, wantErr stringMatcher) { loc.Range.End = loc.Range.Start // diagnostics ignore end position. // Find and remove the matching diagnostic. - diag, ok := removeDiagnostic(mark, loc, re) + diag, ok := removeDiagnostic(mark, loc, false, re) if !ok { mark.errorf("no diagnostic at %v matches %q", loc, re) return diff --git a/gopls/internal/test/marker/testdata/diagnostics/issue69505.txt b/gopls/internal/test/marker/testdata/diagnostics/issue69505.txt new file mode 100644 index 00000000000..6b2751d840b --- /dev/null +++ b/gopls/internal/test/marker/testdata/diagnostics/issue69505.txt @@ -0,0 +1,22 @@ +This test checks that diagnostics ranges computed with the TypeErrorEndPos +heuristic span at least a full token. + +-- go.mod -- +module example.com + +go 1.21 + +-- main.go -- +package main + +import "example.com/foo-bar" //@ diag(re`"[^"]*"`, re`not used`, exact=true) + +func f(int) {} + +func main() { + var x int + _ = x + 1.e+0i //@ diag("1.e+0i", re`truncated`, exact=true) +} + +-- foo-bar/baz.go -- +package foo diff --git a/internal/analysisinternal/analysis.go b/internal/analysisinternal/analysis.go index 24755b41265..4ccaa210af1 100644 --- a/internal/analysisinternal/analysis.go +++ b/internal/analysisinternal/analysis.go @@ -10,6 +10,7 @@ import ( "bytes" "fmt" "go/ast" + "go/scanner" "go/token" "go/types" "os" @@ -21,12 +22,46 @@ import ( func TypeErrorEndPos(fset *token.FileSet, src []byte, start token.Pos) token.Pos { // Get the end position for the type error. - offset, end := fset.PositionFor(start, false).Offset, start - if offset >= len(src) { - return end + file := fset.File(start) + if file == nil { + return start } - if width := bytes.IndexAny(src[offset:], " \n,():;[]+-*"); width > 0 { - end = start + token.Pos(width) + if offset := file.PositionFor(start, false).Offset; offset > len(src) { + return start + } else { + src = src[offset:] + } + + // Attempt to find a reasonable end position for the type error. + // + // TODO(rfindley): the heuristic implemented here is unclear. It looks like + // it seeks the end of the primary operand starting at start, but that is not + // quite implemented (for example, given a func literal this heuristic will + // return the range of the func keyword). + // + // We should formalize this heuristic, or deprecate it by finally proposing + // to add end position to all type checker errors. + // + // Nevertheless, ensure that the end position at least spans the current + // token at the cursor (this was golang/go#69505). + end := start + { + var s scanner.Scanner + fset := token.NewFileSet() + f := fset.AddFile("", fset.Base(), len(src)) + s.Init(f, src, nil /* no error handler */, scanner.ScanComments) + pos, tok, lit := s.Scan() + if tok != token.SEMICOLON && token.Pos(f.Base()) <= pos && pos <= token.Pos(f.Base()+f.Size()) { + off := file.Offset(pos) + len(lit) + src = src[off:] + end += token.Pos(off) + } + } + + // Look for bytes that might terminate the current operand. See note above: + // this is imprecise. + if width := bytes.IndexAny(src, " \n,():;[]+-*/"); width > 0 { + end += token.Pos(width) } return end }