Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
adonovan opened this issue Jun 2, 2022 · 15 comments
Closed

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

adonovan opened this issue Jun 2, 2022 · 15 comments

Comments

@adonovan
Copy link
Member

adonovan commented Jun 2, 2022

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
Copy link
Contributor

dmitshur commented Jun 3, 2022

@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.

@dmitshur dmitshur added this to the Backlog milestone Jun 3, 2022
@dmitshur dmitshur added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. FeatureRequest labels Jun 3, 2022
@adonovan
Copy link
Member Author

adonovan commented Jun 4, 2022

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
Copy link
Contributor

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

@ianlancetaylor ianlancetaylor changed the title go/ast: File.Pos() does not report the start of the file proposal: go/ast: add File.Start field, change File.Pos result Jun 22, 2022
@ianlancetaylor ianlancetaylor modified the milestones: Backlog, Proposal Jun 22, 2022
@ianlancetaylor ianlancetaylor removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. FeatureRequest labels Jun 22, 2022
@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) Jun 22, 2022
@rsc
Copy link
Contributor

rsc commented Jun 22, 2022

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 rsc moved this from Incoming to Active in Proposals (old) Jun 22, 2022
@griesemer
Copy link
Contributor

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
Copy link
Contributor

rsc commented Jul 13, 2022

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
Copy link
Contributor

rsc commented Jul 13, 2022

Placed on hold.
— rsc for the proposal review group

@rsc rsc moved this from Active to Hold in Proposals (old) Jul 13, 2022
@adonovan
Copy link
Member Author

[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
Copy link
Member Author

adonovan commented Sep 2, 2022

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

@gopherbot
Copy link

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

@griesemer griesemer changed the title proposal: go/ast: add File.Start field, change File.Pos result proposal: go/ast: record start and end of file in File.{Start,End}Pos Sep 6, 2022
@rsc
Copy link
Contributor

rsc commented Sep 7, 2022

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
Copy link
Contributor

rsc commented Sep 7, 2022

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 rsc changed the title proposal: go/ast: record start and end of file in File.{Start,End}Pos proposal: go/ast: record start and end of file in File.FileStart, File.FileEnd Sep 21, 2022
@rsc
Copy link
Contributor

rsc commented Sep 21, 2022

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

@rsc
Copy link
Contributor

rsc commented Sep 21, 2022

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

@rsc
Copy link
Contributor

rsc commented Sep 28, 2022

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

@rsc rsc changed the title proposal: go/ast: record start and end of file in File.FileStart, File.FileEnd go/ast: record start and end of file in File.FileStart, File.FileEnd Sep 28, 2022
@rsc rsc modified the milestones: Proposal, Backlog Sep 28, 2022
@golang golang locked and limited conversation to collaborators Sep 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Development

No branches or pull requests

6 participants