x/tools: fset.File(file.Pos()) -> fset.File(file.FileStart)

This CL fixes a bug (#70149) in gopls/internal/golang/pkgdoc.go
in which a call to fset.File(file.Pos()) would return nil
because when file points to an empty ast.File, Pos() returns NoPos.

Instead, we should use file.FileStart, which is (in principle)
always valid even for an empty file. However, there is a separate
bug in go1.23 (#70162) that means FileStart is invalid whenever
Pos() is. So, this fix only works with go1.24, and there's no
real workaround short of the additional logic this CL adds to
parsego.Parse, which at least covers all of gopls.

Also, we audit all of x/tools for similar faulty uses of Pos()
and replace them with FileStart. In future, we should use File.Pos
only for its specific meaning related to the package decl.

Fixes golang/go#70149
Updates golang/go#70162

Change-Id: Ic8cedfe912e44a0b4eb6e5e6874a6266d4be9076
Reviewed-on: https://go-review.googlesource.com/c/tools/+/624437
Reviewed-by: Robert Findley <rfindley@google.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>

go/analysis/passes/cgocall/cgocall.go

@@ -179,7 +179,7 @@ func typeCheckCgoSourceFiles(fset *token.FileSet, pkg *types.Package, files []*a
 	for _, raw := range files {
 		// If f is a cgo-generated file, Position reports
 		// the original file, honoring //line directives.
-		filename := fset.Position(raw.Pos()).Filename
+		filename := fset.Position(raw.Pos()).Filename // sic: Pos, not FileStart
 		f, err := parser.ParseFile(fset, filename, nil, parser.SkipObjectResolution)
 		if err != nil {
 			return nil, nil, fmt.Errorf("can't parse raw cgo file: %v", err)

go/analysis/passes/tests/tests.go

@@ -48,7 +48,7 @@ var acceptedFuzzTypes = []types.Type{
 
 func run(pass *analysis.Pass) (interface{}, error) {
 	for _, f := range pass.Files {
-		if !strings.HasSuffix(pass.Fset.File(f.Pos()).Name(), "_test.go") {
+		if !strings.HasSuffix(pass.Fset.File(f.FileStart).Name(), "_test.go") {
 			continue
 		}
 		for _, decl := range f.Decls {

go/ast/inspector/inspector.go

@@ -180,7 +180,9 @@ func (in *Inspector) WithStack(types []ast.Node, f func(n ast.Node, push bool, s
 // traverse builds the table of events representing a traversal.
 func traverse(files []*ast.File) []event {
 	// Preallocate approximate number of events
-	// based on source file extent.
+	// based on source file extent of the declarations.
+	// (We use End-Pos not FileStart-FileEnd to neglect
+	// the effect of long doc comments.)
 	// This makes traverse faster by 4x (!).
 	var extent int
 	for _, f := range files {

go/callgraph/rta/rta_test.go

@@ -100,7 +100,7 @@ func TestRTA(t *testing.T) {
 //
 // Functions are notated as if by ssa.Function.String.
 func check(t *testing.T, f *ast.File, pkg *ssa.Package, res *rta.Result) {
-	tokFile := pkg.Prog.Fset.File(f.Pos())
+	tokFile := pkg.Prog.Fset.File(f.FileStart)
 
 	// Find the WANT comment.
 	expectation := func(f *ast.File) (string, int) {

go/loader/loader.go

@@ -340,13 +340,12 @@ func (conf *Config) addImport(path string, tests bool) {
 func (prog *Program) PathEnclosingInterval(start, end token.Pos) (pkg *PackageInfo, path []ast.Node, exact bool) {
 	for _, info := range prog.AllPackages {
 		for _, f := range info.Files {
-			if f.Pos() == token.NoPos {
-				// This can happen if the parser saw
-				// too many errors and bailed out.
-				// (Use parser.AllErrors to prevent that.)
+			if f.FileStart == token.NoPos {
+				// Workaround for #70162 (undefined FileStart).
+				// TODO(adonovan): delete once go1.24 is assured.
 				continue
 			}
-			if !tokenFileContainsPos(prog.Fset.File(f.Pos()), start) {
+			if !tokenFileContainsPos(prog.Fset.File(f.FileStart), start) {
 				continue
 			}
 			if path, exact := astutil.PathEnclosingInterval(f, start, end); path != nil {

go/packages/packages_test.go

@@ -2684,7 +2684,7 @@ func testIssue48226(t *testing.T, exporter packagestest.Exporter) {
 		t.Fatalf("package has errors: %v", pkg.Errors)
 	}
 
-	fname := pkg.Fset.File(pkg.Syntax[0].Pos()).Name()
+	fname := pkg.Fset.File(pkg.Syntax[0].FileStart).Name()
 	if filepath.Base(fname) != "syntax.go" {
 		t.Errorf("expected the package declaration position "+
 			"to resolve to \"syntax.go\", got %q instead", fname)

go/types/internal/play/play.go

@@ -113,7 +113,7 @@ func handleSelectJSON(w http.ResponseWriter, req *http.Request) {
 
 	fset := pkg.Fset
 	file := pkg.Syntax[0]
-	tokFile := fset.File(file.Pos())
+	tokFile := fset.File(file.FileStart)
 	startPos := tokFile.Pos(startOffset)
 	endPos := tokFile.Pos(endOffset)
 

godoc/index.go

@@ -649,7 +649,7 @@ func (x *Indexer) addFile(f vfs.ReadSeekCloser, filename string, goFile bool) (f
 		if goFile {
 			// parse the file and in the process add it to the file set
 			if ast, err = parser.ParseFile(x.fset, filename, src, parser.ParseComments); err == nil {
-				file = x.fset.File(ast.Pos()) // ast.Pos() is inside the file
+				file = x.fset.File(ast.FileStart) // ast.FileStart is inside the file
 				return
 			}
 			// file has parse errors, and the AST may be incorrect -

gopls/doc/generate/generate.go

@@ -533,7 +533,7 @@ func lowerFirst(x string) string {
 func fileForPos(pkg *packages.Package, pos token.Pos) (*ast.File, error) {
 	fset := pkg.Fset
 	for _, f := range pkg.Syntax {
-		if safetoken.StartPosition(fset, f.Pos()).Filename == safetoken.StartPosition(fset, pos).Filename {
+		if safetoken.StartPosition(fset, f.FileStart).Filename == safetoken.StartPosition(fset, pos).Filename {
 			return f, nil
 		}
 	}

gopls/internal/analysis/fillreturns/fillreturns.go

@@ -45,7 +45,7 @@ outer:
 		}
 		var file *ast.File
 		for _, f := range pass.Files {
-			if f.Pos() <= typeErr.Pos && typeErr.Pos <= f.End() {
+			if f.FileStart <= typeErr.Pos && typeErr.Pos <= f.FileEnd {
 				file = f
 				break
 			}

gopls/internal/analysis/nonewvars/nonewvars.go

@@ -47,7 +47,7 @@ func run(pass *analysis.Pass) (interface{}, error) {
 
 		var file *ast.File
 		for _, f := range pass.Files {
-			if f.Pos() <= assignStmt.Pos() && assignStmt.Pos() < f.End() {
+			if f.FileStart <= assignStmt.Pos() && assignStmt.Pos() < f.FileEnd {
 				file = f
 				break
 			}

gopls/internal/analysis/noresultvalues/noresultvalues.go

@@ -42,7 +42,7 @@ func run(pass *analysis.Pass) (interface{}, error) {
 
 		var file *ast.File
 		for _, f := range pass.Files {
-			if f.Pos() <= retStmt.Pos() && retStmt.Pos() < f.End() {
+			if f.FileStart <= retStmt.Pos() && retStmt.Pos() < f.FileEnd {
 				file = f
 				break
 			}

gopls/internal/analysis/simplifycompositelit/simplifycompositelit.go

@@ -38,7 +38,7 @@ func run(pass *analysis.Pass) (interface{}, error) {
 	generated := make(map[*token.File]bool)
 	for _, file := range pass.Files {
 		if ast.IsGenerated(file) {
-			generated[pass.Fset.File(file.Pos())] = true
+			generated[pass.Fset.File(file.FileStart)] = true
 		}
 	}
 

gopls/internal/analysis/simplifyrange/simplifyrange.go

@@ -33,7 +33,7 @@ func run(pass *analysis.Pass) (interface{}, error) {
 	generated := make(map[*token.File]bool)
 	for _, file := range pass.Files {
 		if ast.IsGenerated(file) {
-			generated[pass.Fset.File(file.Pos())] = true
+			generated[pass.Fset.File(file.FileStart)] = true
 		}
 	}
 

gopls/internal/analysis/simplifyslice/simplifyslice.go

@@ -42,7 +42,7 @@ func run(pass *analysis.Pass) (interface{}, error) {
 	generated := make(map[*token.File]bool)
 	for _, file := range pass.Files {
 		if ast.IsGenerated(file) {
-			generated[pass.Fset.File(file.Pos())] = true
+			generated[pass.Fset.File(file.FileStart)] = true
 		}
 	}
 

gopls/internal/analysis/undeclaredname/undeclared.go

@@ -59,7 +59,7 @@ func runForError(pass *analysis.Pass, err types.Error) {
 	// Find file enclosing error.
 	var file *ast.File
 	for _, f := range pass.Files {
-		if f.Pos() <= err.Pos && err.Pos < f.End() {
+		if f.FileStart <= err.Pos && err.Pos < f.FileEnd {
 			file = f
 			break
 		}

gopls/internal/analysis/unusedvariable/unusedvariable.go

@@ -60,7 +60,7 @@ func run(pass *analysis.Pass) (interface{}, error) {
 func runForError(pass *analysis.Pass, err types.Error, name string) error {
 	var file *ast.File
 	for _, f := range pass.Files {
-		if f.Pos() <= err.Pos && err.Pos < f.End() {
+		if f.FileStart <= err.Pos && err.Pos < f.FileEnd {
 			file = f
 			break
 		}

gopls/internal/cache/parse.go

@@ -40,6 +40,6 @@ func parseGoImpl(ctx context.Context, fset *token.FileSet, fh file.Handle, mode
 	if ctx.Err() != nil {
 		return nil, ctx.Err()
 	}
-	pgf, _ := parsego.Parse(ctx, fset, fh.URI(), content, mode, purgeFuncBodies)
+	pgf, _ := parsego.Parse(ctx, fset, fh.URI(), content, mode, purgeFuncBodies) // ignore 'fixes'
 	return pgf, nil
 }

gopls/internal/cache/parsego/file.go

@@ -20,9 +20,11 @@ type File struct {
 	URI  protocol.DocumentURI
 	Mode parser.Mode
 
-	// File is the file resulting from parsing. Clients must not access the AST's
-	// legacy ast.Object-related fields without first ensuring that
-	// [File.Resolve] was already called.
+	// File is the file resulting from parsing. It is always non-nil.
+	//
+	// Clients must not access the AST's legacy ast.Object-related
+	// fields without first ensuring that [File.Resolve] was
+	// already called.
 	File *ast.File
 	Tok  *token.File
 	// Source code used to build the AST. It may be different from the

gopls/internal/cache/parsego/parse.go

@@ -59,16 +59,36 @@ func Parse(ctx context.Context, fset *token.FileSet, uri protocol.DocumentURI, s
 		// We passed a byte slice, so the only possible error is a parse error.
 		parseErr = err.(scanner.ErrorList)
 	}
+	// Inv: file != nil.
 
-	tok := fset.File(file.Pos())
-	if tok == nil {
-		// file.Pos is the location of the package declaration (issue #53202). If there was
-		// none, we can't find the token.File that ParseFile created, and we
-		// have no choice but to recreate it.
-		tok = fset.AddFile(uri.Path(), -1, len(src))
-		tok.SetLinesForContent(src)
+	// Workaround for #70162 (missing File{Start,End} when
+	// parsing empty file) with go1.23.
+	//
+	// When parsing an empty file, or one without a valid
+	// package declaration, the go1.23 parser bails out before
+	// setting FileStart and End.
+	//
+	// This leaves us no way to find the original
+	// token.File that ParseFile created, so as a
+	// workaround, we recreate the token.File, and
+	// populate the FileStart and FileEnd fields.
+	//
+	// See also #53202.
+	tokenFile := func(file *ast.File) *token.File {
+		tok := fset.File(file.FileStart)
+		if tok == nil {
+			tok = fset.AddFile(uri.Path(), -1, len(src))
+			tok.SetLinesForContent(src)
+			if file.FileStart.IsValid() {
+				file.FileStart = token.Pos(tok.Base())
+				file.FileEnd = token.Pos(tok.Base() + tok.Size())
+			}
+		}
+		return tok
 	}
 
+	tok := tokenFile(file)
+
 	fixedSrc := false
 	fixedAST := false
 	// If there were parse errors, attempt to fix them up.
@@ -96,15 +116,13 @@ func Parse(ctx context.Context, fset *token.FileSet, uri protocol.DocumentURI, s
 			}
 
 			newFile, newErr := parser.ParseFile(fset, uri.Path(), newSrc, mode)
-			if newFile == nil {
-				break // no progress
-			}
+			assert(newFile != nil, "ParseFile returned nil") // I/O error can't happen
 
 			// Maintain the original parseError so we don't try formatting the
 			// doctored file.
 			file = newFile
 			src = newSrc
-			tok = fset.File(file.Pos())
+			tok = tokenFile(file)
 
 			// Only now that we accept the fix do we record the src fix from above.
 			fixes = append(fixes, srcFix)
@@ -122,6 +140,7 @@ func Parse(ctx context.Context, fset *token.FileSet, uri protocol.DocumentURI, s
 			}
 		}
 	}
+	assert(file != nil, "nil *ast.File")
 
 	return &File{
 		URI:      uri,

gopls/internal/golang/extract.go

@@ -24,7 +24,7 @@ import (
 )
 
 func extractVariable(fset *token.FileSet, start, end token.Pos, src []byte, file *ast.File, pkg *types.Package, info *types.Info) (*token.FileSet, *analysis.SuggestedFix, error) {
-	tokFile := fset.File(file.Pos())
+	tokFile := fset.File(file.FileStart)
 	expr, path, ok, err := canExtractVariable(start, end, file)
 	if !ok {
 		return nil, nil, fmt.Errorf("extractVariable: cannot extract %s: %v", safetoken.StartPosition(fset, start), err)
@@ -214,7 +214,7 @@ func extractFunctionMethod(fset *token.FileSet, start, end token.Pos, src []byte
 		errorPrefix = "extractMethod"
 	}
 
-	tok := fset.File(file.Pos())
+	tok := fset.File(file.FileStart)
 	if tok == nil {
 		return nil, nil, bug.Errorf("no file for position")
 	}
@@ -821,7 +821,7 @@ func collectFreeVars(info *types.Info, file *ast.File, fileScope, pkgScope *type
 		if _, ok := obj.(*types.PkgName); ok {
 			return nil, false // imported package
 		}
-		if !(file.Pos() <= obj.Pos() && obj.Pos() <= file.End()) {
+		if !(file.FileStart <= obj.Pos() && obj.Pos() <= file.FileEnd) {
 			return nil, false // not defined in this file
 		}
 		scope := obj.Parent()

gopls/internal/golang/format.go

@@ -215,7 +215,7 @@ func importPrefix(src []byte) (string, error) {
 	if err != nil { // This can happen if 'package' is misspelled
 		return "", fmt.Errorf("importPrefix: failed to parse: %s", err)
 	}
-	tok := fset.File(f.Pos())
+	tok := fset.File(f.FileStart)
 	var importEnd int
 	for _, d := range f.Decls {
 		if x, ok := d.(*ast.GenDecl); ok && x.Tok == token.IMPORT {

gopls/internal/golang/inlay_hint.go

@@ -51,7 +51,9 @@ func InlayHint(ctx context.Context, snapshot *cache.Snapshot, fh file.Handle, pR
 	q := typesutil.FileQualifier(pgf.File, pkg.Types(), info)
 
 	// Set the range to the full file if the range is not valid.
-	start, end := pgf.File.Pos(), pgf.File.End()
+	start, end := pgf.File.FileStart, pgf.File.FileEnd
+
+	// TODO(adonovan): this condition looks completely wrong!
 	if pRng.Start.Line < pRng.End.Line || pRng.Start.Character < pRng.End.Character {
 		// Adjust start and end for the specified range.
 		var err error

gopls/internal/golang/pkgdoc.go

@@ -282,7 +282,7 @@ func PackageDocHTML(viewID string, pkg *cache.Package, web Web) ([]byte, error)
 	// TODO(adonovan): simulate that too.
 	fileMap := make(map[string]*ast.File)
 	for _, f := range pkg.Syntax() {
-		fileMap[pkg.FileSet().File(f.Pos()).Name()] = f
+		fileMap[pkg.FileSet().File(f.FileStart).Name()] = f
 	}
 	astpkg := &ast.Package{
 		Name:  pkg.Types().Name(),

gopls/internal/golang/rename.go

@@ -1420,7 +1420,7 @@ func parsePackageNameDecl(ctx context.Context, snapshot *cache.Snapshot, fh file
 // enclosingFile returns the CompiledGoFile of pkg that contains the specified position.
 func enclosingFile(pkg *cache.Package, pos token.Pos) (*parsego.File, bool) {
 	for _, pgf := range pkg.CompiledGoFiles() {
-		if pgf.File.Pos() <= pos && pos <= pgf.File.End() {
+		if pgf.File.FileStart <= pos && pos <= pgf.File.FileEnd {
 			return pgf, true
 		}
 	}

gopls/internal/protocol/command/commandmeta/meta.go

@@ -248,7 +248,7 @@ func findField(pkg *packages.Package, pos token.Pos) (*ast.Field, error) {
 	fset := pkg.Fset
 	var file *ast.File
 	for _, f := range pkg.Syntax {
-		if fset.File(f.Pos()).Name() == fset.File(pos).Name() {
+		if fset.File(f.FileStart).Name() == fset.File(pos).Name() {
 			file = f
 			break
 		}

gopls/internal/util/safetoken/safetoken_test.go

@@ -24,7 +24,7 @@ func TestWorkaroundIssue57490(t *testing.T) {
 	src := `package p; func f() { var x struct`
 	fset := token.NewFileSet()
 	file, _ := parser.ParseFile(fset, "a.go", src, parser.SkipObjectResolution)
-	tf := fset.File(file.Pos())
+	tf := fset.File(file.FileStart)
 
 	// Add another file to the FileSet.
 	file2, _ := parser.ParseFile(fset, "b.go", "package q", parser.SkipObjectResolution)

internal/analysisinternal/addimport_test.go

@@ -202,7 +202,7 @@ func _() {
 			if err != nil {
 				t.Log(err)
 			}
-			pos := fset.File(f.Pos()).Pos(len(before))
+			pos := fset.File(f.FileStart).Pos(len(before))
 
 			// type-check
 			info := &types.Info{

internal/imports/imports.go

@@ -114,7 +114,7 @@ func ApplyFixes(fixes []*ImportFix, filename string, src []byte, opt *Options, e
 // formatted file, and returns the postpocessed result.
 func formatFile(fset *token.FileSet, file *ast.File, src []byte, adjust func(orig []byte, src []byte) []byte, opt *Options) ([]byte, error) {
 	mergeImports(file)
-	sortImports(opt.LocalPrefix, fset.File(file.Pos()), file)
+	sortImports(opt.LocalPrefix, fset.File(file.FileStart), file)
 	var spacesBefore []string // import paths we need spaces before
 	for _, impSection := range astutil.Imports(fset, file) {
 		// Within each block of contiguous imports, see if any

internal/refactor/inline/inline_test.go

@@ -86,7 +86,7 @@ func TestData(t *testing.T) {
 			for _, pkg := range pkgs {
 				for _, file := range pkg.Syntax {
 					// Read file content (for @inline regexp, and inliner).
-					content, err := os.ReadFile(pkg.Fset.File(file.Pos()).Name())
+					content, err := os.ReadFile(pkg.Fset.File(file.FileStart).Name())
 					if err != nil {
 						t.Error(err)
 						continue
@@ -95,7 +95,7 @@ func TestData(t *testing.T) {
 					// Read and process @inline notes.
 					notes, err := expect.ExtractGo(pkg.Fset, file)
 					if err != nil {
-						t.Errorf("parsing notes in %q: %v", pkg.Fset.File(file.Pos()).Name(), err)
+						t.Errorf("parsing notes in %q: %v", pkg.Fset.File(file.FileStart).Name(), err)
 						continue
 					}
 					for _, note := range notes {
@@ -157,7 +157,7 @@ func doInlineNote(logf func(string, ...any), pkg *packages.Package, file *ast.Fi
 	// Find extent of pattern match within commented line.
 	var startPos, endPos token.Pos
 	{
-		tokFile := pkg.Fset.File(file.Pos())
+		tokFile := pkg.Fset.File(file.FileStart)
 		lineStartOffset := int(tokFile.LineStart(posn.Line)) - tokFile.Base()
 		line := content[lineStartOffset:]
 		if i := bytes.IndexByte(line, '\n'); i >= 0 {

refactor/rename/mvpkg.go

@@ -321,7 +321,7 @@ func (m *mover) move() error {
 			log.Printf("failed to pretty-print syntax tree: %v", err)
 			continue
 		}
-		tokenFile := m.iprog.Fset.File(f.Pos())
+		tokenFile := m.iprog.Fset.File(f.FileStart)
 		writeFile(tokenFile.Name(), buf.Bytes())
 	}
 

refactor/rename/rename.go

@@ -490,7 +490,7 @@ func (r *renamer) update() error {
 	var generatedFileNames []string
 	for _, info := range r.packages {
 		for _, f := range info.Files {
-			tokenFile := r.iprog.Fset.File(f.Pos())
+			tokenFile := r.iprog.Fset.File(f.FileStart)
 			if filesToUpdate[tokenFile] && generated(f, tokenFile) {
 				generatedFileNames = append(generatedFileNames, tokenFile.Name())
 			}
@@ -505,7 +505,7 @@ func (r *renamer) update() error {
 	for _, info := range r.packages {
 		first := true
 		for _, f := range info.Files {
-			tokenFile := r.iprog.Fset.File(f.Pos())
+			tokenFile := r.iprog.Fset.File(f.FileStart)
 			if filesToUpdate[tokenFile] {
 				if first {
 					npkgs++

refactor/rename/spec.go

@@ -313,7 +313,7 @@ func findFromObjectsInFile(iprog *loader.Program, spec *spec) ([]types.Object, e
 		// NB: under certain proprietary build systems, a given
 		// filename may appear in multiple packages.
 		for _, f := range info.Files {
-			thisFile := iprog.Fset.File(f.Pos())
+			thisFile := iprog.Fset.File(f.FileStart)
 			if !sameFile(thisFile.Name(), spec.filename) {
 				continue
 			}