go/ast: record start and end of file in File.FileStart, File.FileEnd

[adonovan]

The Pos/End methods on ast.Node are supposed to report the extent of the syntax node, but ast.File has always been an exception: its Pos reports the position of the start of the Package token, which is typically not the start of the file, since the package declaration is usually preceded by one or more of a copyright header comment, a build tag comment, and a package doc comment, all of which are subjects of interest to developer tools.

I propose that ast.File be augmented with a new field, Start token.Pos, and that the parser set this field. Furthermore I propose that (*File).Pos() return f.Start instead of f.Package. This change is slightly higher risk since it may break workarounds for the bug, but it is a bug nonetheless.

See https://cs.opensource.google/go/x/tools/+/master:go/ast/astutil/enclosing.go;l=57-61 for an example of the nuisance.

[dmitshur]

@adonovan Thanks for reporting. Do you think this API addition needs to go through the proposal process or it small enough to be a normal issue with a FeatureRequest label?

CC @griesemer via https://dev.golang.org/owners.

[adonovan]

No, I don't think it needs the proposal process as the field addition is harmless. The question for @griesemer is whether any behavior change to File.Pos/End is acceptable. The patch below caused some gopls tests to fail, for example.

src$ git diff
diff --git a/src/go/ast/ast.go b/src/go/ast/ast.go
index 1e089b9e70..2dc0f43dcd 100644
--- a/src/go/ast/ast.go
+++ b/src/go/ast/ast.go
@@ -1035,23 +1035,19 @@ func (*FuncDecl) declNode() {}
 // and Comment comments directly associated with nodes, the remaining comments
 // are "free-floating" (see also issues #18593, #20744).
 type File struct {
-       Doc        *CommentGroup   // associated documentation; or nil
-       Package    token.Pos       // position of "package" keyword
-       Name       *Ident          // package name
-       Decls      []Decl          // top-level declarations; or nil
-       Scope      *Scope          // package scope (this file only)
-       Imports    []*ImportSpec   // imports in this file
-       Unresolved []*Ident        // unresolved identifiers in this file
-       Comments   []*CommentGroup // list of all comments in the source file
+       StartPos, EndPos token.Pos       // start and end of file
+       Doc              *CommentGroup   // associated documentation; or nil
+       Package          token.Pos       // position of "package" keyword
+       Name             *Ident          // package name
+       Decls            []Decl          // top-level declarations; or nil
+       Scope            *Scope          // package scope (this file only)
+       Imports          []*ImportSpec   // imports in this file
+       Unresolved       []*Ident        // unresolved identifiers in this file
+       Comments         []*CommentGroup // list of all comments in the source file
 }
 
-func (f *File) Pos() token.Pos { return f.Package }
-func (f *File) End() token.Pos {
-       if n := len(f.Decls); n > 0 {
-               return f.Decls[n-1].End()
-       }
-       return f.Name.End()
-}
+func (f *File) Pos() token.Pos { return f.StartPos }
+func (f *File) End() token.Pos { return f.EndPos }
 
 // A Package node represents a set of source files
 // collectively building a Go package.
diff --git a/src/go/ast/filter.go b/src/go/ast/filter.go
index 2fc73c4b99..cfdaa074f8 100644
--- a/src/go/ast/filter.go
+++ b/src/go/ast/filter.go
@@ -484,5 +484,6 @@ func MergePackageFiles(pkg *Package, mode MergeMode) *File {
        }
 
        // TODO(gri) need to compute unresolved identifiers!
-       return &File{doc, pos, NewIdent(pkg.Name), decls, pkg.Scope, imports, nil, comments}
+       // TODO: use least start and max end pos?
+       return &File{token.NoPos, token.NoPos, doc, pos, NewIdent(pkg.Name), decls, pkg.Scope, imports, nil, comments}
 }
diff --git a/src/go/parser/parser.go b/src/go/parser/parser.go
index 18041ff808..354d429623 100644
--- a/src/go/parser/parser.go
+++ b/src/go/parser/parser.go
@@ -2887,6 +2887,8 @@ func (p *parser) parseFile() *ast.File {
        }
 
        f := &ast.File{
+               StartPos: token.Pos(p.file.Base()),
+               EndPos:   token.Pos(p.file.Base() + p.file.Size()),
                Doc:      doc,
                Package:  pos,
                Name:     ident,

[ianlancetaylor]

I agree that this is harmless, but new public API should always go through the proposal process.

[rsc]

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

[griesemer]

I don't think we can change the behavior of (values returned by) File.Pos() and File.End() - this may affect all kinds of programs in the wild. Also, changing their meaning would make them inconsistent with how the Pos/End pairs are defined for the other nodes. That is, an ast.File describes a "logical" Go file not the actual source file.

I'm ok with adding two new fields (FileStart, FileEnd ?) that describe the file extent, as a pragmatic solution to wanting this information. (For comparison, the syntax package has a field File.EOF that provides the source file eof position for similar reasons).

[rsc]

It sounds like the answer is to add two fields to ast.File - Start and EOF, or else FileStart and FileEnd - but not change the results of Pos and End.

Both @adonovan and @griesemer will be away off and on for a while so let's put this on hold.

[rsc]

Placed on hold.
— rsc for the proposal review group

[adonovan]

[gri] I don't think we can change the behavior of (values returned by) File.Pos() and File.End()

[rsc] It sounds like the answer is to add two fields to ast.File - Start and EOF, or else FileStart and FileEnd - but not change the results of Pos and End.

Ok, let's leave Pos/End as they are.

[adonovan]

CL implementing proposed changes: https://go-review.googlesource.com/c/go/+/427955

[gopherbot]

Change https://go.dev/cl/427955 mentions this issue: go/ast: record start and end of file in File.{Start,End}Pos

[rsc]

Taking off hold. It's a little surprising that End() and EndPos are different values: the names don't make clear which is which (both are Pos). Maybe File.FileStart and File.FileEnd? It's a bit repetitive but it at least tells a bit more.

[rsc]

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

[rsc]

Retitled for the fields being FileStart and FileEnd. People seem happy with that.

[rsc]

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

[rsc]

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group