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 <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Alan Donovan <[email protected]>

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?

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

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

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
 }