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: Walk does not walk every comment #11880

Closed
zimmski opened this issue Jul 27, 2015 · 7 comments
Closed

go/ast: Walk does not walk every comment #11880

zimmski opened this issue Jul 27, 2015 · 7 comments
Milestone

Comments

@zimmski
Copy link
Contributor

@zimmski zimmski commented Jul 27, 2015

Here is an example https://play.golang.org/p/JFn3IZnr6E

According to http://golang.org/src/go/ast/walk.go?#L348 comments of ast.File are intentionally not walked because "... visited already through the individual nodes ..." but as you can see from the example, this is not true.

EDIT: I would be happy to fix that if you can give me some pointers.

@ianlancetaylor ianlancetaylor changed the title ast.Walk does not walk every comment go/ast: Walk does not walk every comment Jul 27, 2015
@ianlancetaylor ianlancetaylor added this to the Unplanned milestone Jul 27, 2015
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jul 27, 2015

@griesemer
Copy link
Contributor

@griesemer griesemer commented Jul 27, 2015

@zimmski The AST walker only visits comments that were associated with nodes (excluding the list of all comments that's in ast.File). At this point, only comments above declarations, and line comments in declarations (e.g. structs) are associated with the respective nodes; i.e., the parser assumes that those comments "belong" to those nodes.

In general, the parser doesn't make any further assumptions about other comments and does not associate them with any nodes (this is indeed a major shortcoming of the existing AST and we're investigating how to remedy this situation, possibly via a new AST representation).

Walk could be made more sophisticated and walk comments as they appear by position, relative to the other nodes. But this requires that position information is 100% correct (which is not always the case when the AST is manipulated after creation). It also is not clear with which node a comment should appear (this is something a future heuristic would have to address).

If you want to walk all comments, it's trivial to a) ignore comments encountered by Walk, and b) simply traverse the list of all comments attached to ast.File (i.e., the list of comments in ast.File contains all comments, even the ones associated with other nodes).

Closing this for now since it's working as originally intended; and with the understanding that a future implementation needs to do a better job.

@griesemer griesemer closed this Jul 27, 2015
@zimmski
Copy link
Contributor Author

@zimmski zimmski commented Jul 28, 2015

Thanks for the detailed explanation! We have already a workaround which traverses all comments through the AST, remembers them, and then traverses all comments which have not been visited. This is needed because we analyse comments differently if they belong to a declaration.

I am wondering if these changes to the AST and ast.Walk can be implemented at all because of the backwards-compatible promise? Also, do comments really have to appear "with" a node? Can't they just reside inside the AST as an ordinary node? Additionally the position information must be somehow somewhere correct, because using go/printer will output the correct source even after a modification, or is this assumption wrong?

@griesemer
Copy link
Contributor

@griesemer griesemer commented Jul 28, 2015

The suggested changes require a new AST - not entirely different, but capable of associating comments with nodes. I have a prototype but it needs a bit more work. Once I am confident enough that it can work well, I'll send out a proposal.

Comments can appear anywhere between two tokens (actually, from the scanner's point of view, a comment is just a token). Because they can appear anywhere, there's no obvious place to put them in the AST. For instance, an if statement "if expr /* comment */ { foo() }" consists of an IfStmt node and several child nodes (expr, then branch). The comment could be anywhere. In this case it could (and probably does) make sense to say that it "belongs" to the expr. In other words, the comment is said to be "associated" with the expr. If the comment appeared before the "if" it might instead be associated with the IfStmt node, etc. This association is not obvious from the grammar or language, and requires a heuristic (which may fail). However, if comments were associated with nodes, moving nodes would ensure the associated comments also move with them. This is currently not the case.

For an unmodified AST, all positions (including comment positions) are correct. If the AST is modified, but comment and AST node positions are not updated accordingly, the go/printer will print the comments in the wrong places. This is a common issue with AST manipulation and the major reason why comments should probably be attached to nodes. This was in some sense a design mistake (mea culpa) - the AST package is one of the very first Go packages and I didn't appreciate the need for easy manipulation (nor did I foresee all the AST manipulation tools).

@zimmski
Copy link
Contributor Author

@zimmski zimmski commented Oct 18, 2015

Did you put the proposal you are working on somewhere public?

@griesemer
Copy link
Contributor

@griesemer griesemer commented Oct 19, 2015

@zimmski I'm sorry, no, I have not made any progress on this front due to other more pressing 1.6 items. But it's on the forefront of my things to do.

@zimmski
Copy link
Contributor Author

@zimmski zimmski commented Oct 19, 2015

No need to be sorry. I was just wondering how that is going since I stumbled on my "workaround" mark. Is anything easy on your 1.6? Maybe I can help out since I have some open source hours left in the coming months.

@golang golang locked and limited conversation to collaborators Oct 24, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.