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: optionally save comments #29821

Open
stamblerre opened this issue Jan 18, 2019 · 3 comments

Comments

@stamblerre
Copy link
Contributor

commented Jan 18, 2019

go/ast doesn't save comments in the tree when a file is parsed, (they are kept in the field *ast.File.Comments). This causes problems for functions like astutil.PathEnclosingInterval and forces comments to be special cased in the LSP completions package (context: #29780).

@stamblerre stamblerre self-assigned this Jan 18, 2019

@mvdan

This comment has been minimized.

Copy link
Member

commented Jan 18, 2019

I thought @griesemer's plan was to address this in the new syntax tree package. cmd/compile/internal/syntax does attach all comments to nodes.

Also, have you seen https://golang.org/pkg/go/ast/#CommentMap? It should solve most issues related to the relation of comments with nodes.

I'd also imagine that the go/ast package's API is somewhat frozen at this point, since it's one of the oldest packages in the standard library. But don't quote me on that :)

@griesemer

This comment has been minimized.

Copy link
Contributor

commented Jan 18, 2019

cmd/compile/internal/syntax has the tentative data structure for attached comments but doesn't actually do it at the moment (or whatever is there is not fully functional). The long-term plan is to make that happen, but it's not a high priority.

Also given the fact that all existing tools working on ASTs would have to be adjusted, in the short term it may be better to have an extra parser mode that does attach comments to go/ast nodes. Those nodes could be extended w/o imperiling existing code.

Please keep me in the loop if this is something you plan to work on. There's pre-existing work and we don't need to re-invent the wheel. Also, there are a lot of considerations regarding memory use.

@stamblerre

This comment has been minimized.

Copy link
Contributor Author

commented Jan 22, 2019

@griesemer: thanks for the information - this is pretty low priority, so I probably won't be working on it any time soon. I just wanted to make sure I opened an issue to preserve the discussion on #29780. I will definitely check with you before working on this. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.