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
Open

go/ast: optionally save comments #29821

stamblerre opened this issue Jan 18, 2019 · 3 comments
Labels
FeatureRequest NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@stamblerre
Copy link
Contributor

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

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

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

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

@bcmills bcmills added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. FeatureRequest labels Jan 29, 2019
@bcmills bcmills modified the milestones: unps, Unplanned Jan 29, 2019
@stamblerre stamblerre removed their assignment Oct 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FeatureRequest NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

4 participants