x/tools: various cleanups related to planned parser changes

This CL extracts the good bits out of a large exploratory patch
(CL 668677) to update x/tools in anticipation of parser and AST
changes that I was hoping to land in go1.25; however that isn't
going to happen.

Updates golang/go#73438
Updates golang/go#66790
Updates golang/go#66683
Updates golang/go#67704

Change-Id: Iba4a0a7c4a93d04fc6d46466c9fb9980d52067a3
Reviewed-on: https://go-review.googlesource.com/c/tools/+/672055
LUCI-TryBot-Result: Go LUCI <[email protected]>
Auto-Submit: Alan Donovan <[email protected]>
Reviewed-by: Robert Findley <[email protected]>

Author: adonovan

go/ast/astutil/enclosing.go

@@ -207,6 +207,9 @@ func childrenOf(n ast.Node) []ast.Node {
 		return false // no recursion
 	})
 
+	// TODO(adonovan): be more careful about missing (!Pos.Valid)
+	// tokens in trees produced from invalid input.
+
 	// Then add fake Nodes for bare tokens.
 	switch n := n.(type) {
 	case *ast.ArrayType:
@@ -226,9 +229,12 @@ func childrenOf(n ast.Node) []ast.Node {
 		children = append(children, tok(n.OpPos, len(n.Op.String())))
 
 	case *ast.BlockStmt:
-		children = append(children,
-			tok(n.Lbrace, len("{")),
-			tok(n.Rbrace, len("}")))
+		if n.Lbrace.IsValid() {
+			children = append(children, tok(n.Lbrace, len("{")))
+		}
+		if n.Rbrace.IsValid() {
+			children = append(children, tok(n.Rbrace, len("}")))
+		}
 
 	case *ast.BranchStmt:
 		children = append(children,
@@ -304,9 +310,12 @@ func childrenOf(n ast.Node) []ast.Node {
 		// TODO(adonovan): Field.{Doc,Comment,Tag}?
 
 	case *ast.FieldList:
-		children = append(children,
-			tok(n.Opening, len("(")), // or len("[")
-			tok(n.Closing, len(")"))) // or len("]")
+		if n.Opening.IsValid() {
+			children = append(children, tok(n.Opening, len("(")))
+		}
+		if n.Closing.IsValid() {
+			children = append(children, tok(n.Closing, len(")")))
+		}
 
 	case *ast.File:
 		// TODO test: Doc

gopls/internal/cache/check.go

@@ -2070,12 +2070,14 @@ func typeErrorsToDiagnostics(pkg *syntaxPackage, inputs *typeCheckInputs, errs [
 					}
 				}
 			} else {
+				// TODO(adonovan): check File(start)==File(end).
+
 				// debugging golang/go#65960
 				if _, err := safetoken.Offset(pgf.Tok, end); err != nil {
 					if pkg.hasFixedFiles() {
-						bug.Reportf("ReadGo116ErrorData returned invalid end: %v (fixed files)", err)
+						bug.Reportf("ErrorCodeStartEnd returned invalid end: %v (fixed files)", err)
 					} else {
-						bug.Reportf("ReadGo116ErrorData returned invalid end: %v", err)
+						bug.Reportf("ErrorCodeStartEnd returned invalid end: %v", err)
 					}
 				}
 			}

gopls/internal/cache/parsego/parse.go

@@ -210,6 +210,7 @@ func fixAST(n ast.Node, tok *token.File, src []byte) (fixes []FixType) {
 
 // walkASTWithParent walks the AST rooted at n. The semantics are
 // similar to ast.Inspect except it does not call f(nil).
+// TODO(adonovan): replace with PreorderStack.
 func walkASTWithParent(n ast.Node, f func(n ast.Node, parent ast.Node) bool) {
 	var ancestors []ast.Node
 	ast.Inspect(n, func(n ast.Node) (recurse bool) {
@@ -422,8 +423,10 @@ func fixEmptySwitch(body *ast.BlockStmt, tok *token.File, src []byte) bool {
 	return true
 }
 
-// fixDanglingSelector inserts real "_" selector expressions in place
-// of phantom "_" selectors. For example:
+// fixDanglingSelector inserts a real "_" selector expression in place
+// of a phantom parser-inserted "_" selector so that the parser will
+// not consume the following non-identifier token.
+// For example:
 //
 //	func _() {
 //		x.<>
@@ -453,17 +456,13 @@ func fixDanglingSelector(s *ast.SelectorExpr, tf *token.File, src []byte) []byte
 		return nil
 	}
 
-	var buf bytes.Buffer
-	buf.Grow(len(src) + 1)
-	buf.Write(src[:insertOffset])
-	buf.WriteByte('_')
-	buf.Write(src[insertOffset:])
-	return buf.Bytes()
+	return slices.Concat(src[:insertOffset], []byte("_"), src[insertOffset:])
 }
 
-// fixPhantomSelector tries to fix selector expressions with phantom
-// "_" selectors. In particular, we check if the selector is a
-// keyword, and if so we swap in an *ast.Ident with the keyword text. For example:
+// fixPhantomSelector tries to fix selector expressions whose Sel is a
+// phantom (parser-invented) "_". If the text after the '.' is a
+// keyword, it updates Sel to a fake ast.Ident of that name. For
+// example:
 //
 // foo.var
 //
@@ -498,21 +497,18 @@ func fixPhantomSelector(sel *ast.SelectorExpr, tf *token.File, src []byte) bool
 	})
 }
 
-// isPhantomUnderscore reports whether the given ident is a phantom
-// underscore. The parser sometimes inserts phantom underscores when
-// it encounters otherwise unparseable situations.
+// isPhantomUnderscore reports whether the given ident from a
+// SelectorExpr.Sel was invented by the parser and is not present in
+// source text. The parser creates a blank "_" identifier when the
+// syntax (e.g. a selector) demands one but none is present. The fixer
+// also inserts them.
 func isPhantomUnderscore(id *ast.Ident, tok *token.File, src []byte) bool {
-	if id == nil || id.Name != "_" {
-		return false
+	switch id.Name {
+	case "_": // go1.24 parser
+		offset, err := safetoken.Offset(tok, id.Pos())
+		return err == nil && offset < len(src) && src[offset] != '_'
 	}
-
-	// Phantom underscore means the underscore is not actually in the
-	// program text.
-	offset, err := safetoken.Offset(tok, id.Pos())
-	if err != nil {
-		return false
-	}
-	return len(src) <= offset || src[offset] != '_'
+	return false // real
 }
 
 // fixInitStmt fixes cases where the parser misinterprets an
@@ -821,11 +817,7 @@ FindTo:
 // positions are valid.
 func parseStmt(tok *token.File, pos token.Pos, src []byte) (ast.Stmt, error) {
 	// Wrap our expression to make it a valid Go file we can pass to ParseFile.
-	fileSrc := bytes.Join([][]byte{
-		[]byte("package fake;func _(){"),
-		src,
-		[]byte("}"),
-	}, nil)
+	fileSrc := slices.Concat([]byte("package fake;func _(){"), src, []byte("}"))
 
 	// Use ParseFile instead of ParseExpr because ParseFile has
 	// best-effort behavior, whereas ParseExpr fails hard on any error.
@@ -873,8 +865,8 @@ var tokenPosType = reflect.TypeOf(token.NoPos)
 
 // offsetPositions applies an offset to the positions in an ast.Node.
 func offsetPositions(tok *token.File, n ast.Node, offset token.Pos) {
-	fileBase := int64(tok.Base())
-	fileEnd := fileBase + int64(tok.Size())
+	fileBase := token.Pos(tok.Base())
+	fileEnd := fileBase + token.Pos(tok.Size())
 	ast.Inspect(n, func(n ast.Node) bool {
 		if n == nil {
 			return false
@@ -894,20 +886,21 @@ func offsetPositions(tok *token.File, n ast.Node, offset token.Pos) {
 					continue
 				}
 
+				pos := token.Pos(f.Int())
+
 				// Don't offset invalid positions: they should stay invalid.
-				if !token.Pos(f.Int()).IsValid() {
+				if !pos.IsValid() {
 					continue
 				}
 
 				// Clamp value to valid range; see #64335.
 				//
 				// TODO(golang/go#64335): this is a hack, because our fixes should not
-				// produce positions that overflow (but they do: golang/go#64488).
-				pos := max(f.Int()+int64(offset), fileBase)
-				if pos > fileEnd {
-					pos = fileEnd
-				}
-				f.SetInt(pos)
+				// produce positions that overflow (but they do; see golang/go#64488,
+				// #73438, #66790, #66683, #67704).
+				pos = min(max(pos+offset, fileBase), fileEnd)
+
+				f.SetInt(int64(pos))
 			}
 		}
 
@@ -950,7 +943,7 @@ func replaceNode(parent, oldChild, newChild ast.Node) bool {
 
 		switch f.Kind() {
 		// Check interface and pointer fields.
-		case reflect.Interface, reflect.Ptr:
+		case reflect.Interface, reflect.Pointer:
 			if tryReplace(f) {
 				return true
 			}

gopls/internal/cache/parsego/parse_test.go

@@ -300,14 +300,14 @@ func TestFixPhantomSelector(t *testing.T) {
 			// ensure the selector has been converted to underscore by parser.
 			ensureSource(t, src, func(sel *ast.SelectorExpr) {
 				if sel.Sel.Name != "_" {
-					t.Errorf("%s: the input doesn't cause a blank selector after parser", tc.source)
+					t.Errorf("%s: selector name is %q, want _", tc.source, sel.Sel.Name)
 				}
 			})
 
 			fset := tokeninternal.FileSetFor(pgf.Tok)
 			inspect(t, pgf, func(sel *ast.SelectorExpr) {
 				// the fix should restore the selector as is.
-				if got, want := fmt.Sprintf("%s", analysisinternal.Format(fset, sel)), tc.source; got != want {
+				if got, want := analysisinternal.Format(fset, sel), tc.source; got != want {
 					t.Fatalf("got %v want %v", got, want)
 				}
 			})

gopls/internal/cache/snapshot.go

@@ -1463,10 +1463,11 @@ func orphanedFileDiagnosticRange(ctx context.Context, cache *parseCache, fh file
 		return nil, protocol.Range{}, false
 	}
 	pgf := pgfs[0]
-	if !pgf.File.Name.Pos().IsValid() {
+	name := pgf.File.Name
+	if !name.Pos().IsValid() {
 		return nil, protocol.Range{}, false
 	}
-	rng, err := pgf.PosRange(pgf.File.Name.Pos(), pgf.File.Name.End())
+	rng, err := pgf.PosRange(name.Pos(), name.End())
 	if err != nil {
 		return nil, protocol.Range{}, false
 	}

gopls/internal/fuzzy/matcher.go

@@ -61,6 +61,8 @@ type Matcher struct {
 	rolesBuf [MaxInputSize]RuneRole
 }
 
+func (m *Matcher) String() string { return m.pattern }
+
 func (m *Matcher) bestK(i, j int) int {
 	if m.scores[i][j][0].val() < m.scores[i][j][1].val() {
 		return 1

gopls/internal/golang/addtest.go

@@ -13,7 +13,6 @@ import (
 	"fmt"
 	"go/ast"
 	"go/format"
-	"go/token"
 	"go/types"
 	"os"
 	"path/filepath"
@@ -395,25 +394,26 @@ func AddTestForFunc(ctx context.Context, snapshot *cache.Snapshot, loc protocol.
 			NewText: header.String(),
 		})
 	} else { // existing _test.go file.
-		if testPGF.File.Name == nil || testPGF.File.Name.NamePos == token.NoPos {
+		file := testPGF.File
+		if !file.Name.NamePos.IsValid() {
 			return nil, fmt.Errorf("missing package declaration")
 		}
-		switch testPGF.File.Name.Name {
+		switch file.Name.Name {
 		case pgf.File.Name.Name:
 			xtest = false
 		case pgf.File.Name.Name + "_test":
 			xtest = true
 		default:
-			return nil, fmt.Errorf("invalid package declaration %q in test file %q", testPGF.File.Name, testPGF)
+			return nil, fmt.Errorf("invalid package declaration %q in test file %q", file.Name, testPGF)
 		}
 
-		eofRange, err = testPGF.PosRange(testPGF.File.FileEnd, testPGF.File.FileEnd)
+		eofRange, err = testPGF.PosRange(file.FileEnd, file.FileEnd)
 		if err != nil {
 			return nil, err
 		}
 
 		// Collect all the imports from the foo_test.go.
-		if testImports, err = collectImports(testPGF.File); err != nil {
+		if testImports, err = collectImports(file); err != nil {
 			return nil, err
 		}
 	}

gopls/internal/golang/completion/completion.go

@@ -10,7 +10,6 @@ import (
 	"context"
 	"fmt"
 	"go/ast"
-	"go/build"
 	"go/constant"
 	"go/parser"
 	"go/printer"
@@ -505,7 +504,9 @@ func Completion(ctx context.Context, snapshot *cache.Snapshot, fh file.Handle, p
 	startTime := time.Now()
 
 	pkg, pgf, err := golang.NarrowestPackageForFile(ctx, snapshot, fh.URI())
-	if err != nil || pgf.File.Package == token.NoPos {
+	if err != nil || !pgf.File.Package.IsValid() {
+		// Invalid package declaration
+		//
 		// If we can't parse this file or find position for the package
 		// keyword, it may be missing a package declaration. Try offering
 		// suggestions for the package declaration.
@@ -586,12 +587,6 @@ func Completion(ctx context.Context, snapshot *cache.Snapshot, fh file.Handle, p
 	}
 	scopes = append(scopes, pkg.Types().Scope(), types.Universe)
 
-	var goversion string // "" => no version check
-	// Prior go1.22, the behavior of FileVersion is not useful to us.
-	if slices.Contains(build.Default.ReleaseTags, "go1.22") {
-		goversion = versions.FileVersion(info, pgf.File) // may be ""
-	}
-
 	opts := snapshot.Options()
 	c := &completer{
 		pkg:      pkg,
@@ -605,7 +600,7 @@ func Completion(ctx context.Context, snapshot *cache.Snapshot, fh file.Handle, p
 		fh:                        fh,
 		filename:                  fh.URI().Path(),
 		pgf:                       pgf,
-		goversion:                 goversion,
+		goversion:                 versions.FileVersion(info, pgf.File), // may be "" => no version check
 		path:                      path,
 		pos:                       pos,
 		seen:                      make(map[types.Object]bool),
@@ -746,24 +741,30 @@ func (c *completer) collectCompletions(ctx context.Context) error {
 		if c.pgf.File.Name == n {
 			return c.packageNameCompletions(ctx, c.fh.URI(), n)
 		} else if sel, ok := c.path[1].(*ast.SelectorExpr); ok && sel.Sel == n {
-			// Is this the Sel part of a selector?
+			// We are in the Sel part of a selector (e.g. x.‸sel or x.sel‸).
 			return c.selector(ctx, sel)
 		}
 		return c.lexical(ctx)
-	// The function name hasn't been typed yet, but the parens are there:
-	//   recv.‸(arg)
+
 	case *ast.TypeAssertExpr:
+		// The function name hasn't been typed yet, but the parens are there:
+		//   recv.‸(arg)
 		// Create a fake selector expression.
-		//
+
 		// The name "_" is the convention used by go/parser to represent phantom
 		// selectors.
 		sel := &ast.Ident{NamePos: n.X.End() + token.Pos(len(".")), Name: "_"}
 		return c.selector(ctx, &ast.SelectorExpr{X: n.X, Sel: sel})
+
 	case *ast.SelectorExpr:
+		// We are in the X part of a selector (x‸.sel),
+		// or after the dot with a fixed/phantom Sel (x.‸_).
 		return c.selector(ctx, n)
-	// At the file scope, only keywords are allowed.
+
 	case *ast.BadDecl, *ast.File:
+		// At the file scope, only keywords are allowed.
 		c.addKeywordCompletions()
+
 	default:
 		// fallback to lexical completions
 		return c.lexical(ctx)
@@ -823,6 +824,8 @@ func (c *completer) scanToken(contents []byte) (token.Pos, token.Token, string)
 	tok := c.pkg.FileSet().File(c.pos)
 
 	var s scanner.Scanner
+	// TODO(adonovan): fix! this mutates the token.File borrowed from c.pkg,
+	// calling AddLine and AddLineColumnInfo. Not sound!
 	s.Init(tok, contents, nil, 0)
 	for {
 		tknPos, tkn, lit := s.Scan()
@@ -1232,6 +1235,9 @@ const (
 )
 
 // selector finds completions for the specified selector expression.
+//
+// The caller should ensure that sel.X has type information,
+// even if sel is synthetic.
 func (c *completer) selector(ctx context.Context, sel *ast.SelectorExpr) error {
 	c.inference.objChain = objChain(c.pkg.TypesInfo(), sel.X)
 
@@ -1283,7 +1289,7 @@ func (c *completer) selector(ctx context.Context, sel *ast.SelectorExpr) error {
 	// -- completion of symbols in unimported packages --
 
 	// use new code for unimported completions, if flag allows it
-	if id, ok := sel.X.(*ast.Ident); ok && c.snapshot.Options().ImportsSource == settings.ImportsSourceGopls {
+	if c.snapshot.Options().ImportsSource == settings.ImportsSourceGopls {
 		// The user might have typed strings.TLower, so id.Name==strings, sel.Sel.Name == TLower,
 		// but the cursor might be inside TLower, so adjust the prefix
 		prefix := sel.Sel.Name
@@ -2916,9 +2922,7 @@ func objChain(info *types.Info, e ast.Expr) []types.Object {
 	}
 
 	// Reverse order so the layout matches the syntactic order.
-	for i := range len(objs) / 2 {
-		objs[i], objs[len(objs)-1-i] = objs[len(objs)-1-i], objs[i]
-	}
+	slices.Reverse(objs)
 
 	return objs
 }