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

Jump to bottom

go/printer: CommentedNode behavior is partially undocumented #39219

Open
dmitshur opened this issue May 22, 2020 · 4 comments
Open

go/printer: CommentedNode behavior is partially undocumented #39219

dmitshur opened this issue May 22, 2020 · 4 comments
Labels
Documentation NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Backlog

Comments

@dmitshur
Copy link
Contributor

dmitshur commented May 22, 2020 •
edited

What did you do?

I read the documentation of package go/printer in order to understand how to use printer.CommentedNode.

CommentedNode is currently defined as:

// A CommentedNode bundles an AST node and corresponding comments.
// It may be provided as argument to any of the Fprint functions.
//
type CommentedNode struct {
	Node     interface{} // *ast.File, or ast.Expr, ast.Decl, ast.Spec, or ast.Stmt
	Comments []*ast.CommentGroup
}

It is also mentioned in the documentation of the Config.Fprint method:

// Fprint "pretty-prints" an AST node to output for a given configuration cfg.
// Position information is interpreted relative to the file set fset.
// The node type must be *ast.File, *CommentedNode, []ast.Decl, []ast.Stmt,
// or assignment-compatible to ast.Expr, ast.Decl, ast.Spec, or ast.Stmt.
//
func (cfg *Config) Fprint(output io.Writer, fset *token.FileSet, node interface{}) error

As well as the format.Node function, but there isn't additional information there.

What did you expect to see?

Documentation that would allow me to learn an answer to the question of "what happens to comments that are already a part of the Node being wrapped in a printer.CommentedNode?" without having to also read the source code.

What did you see instead?

Insufficient documentation to know what the intended behavior is.

/cc @griesemer @josharian
@dmitshur dmitshur added Documentation NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels May 22, 2020
@dmitshur dmitshur added this to the Backlog milestone May 22, 2020
@gopherbot
Copy link

Change https://golang.org/cl/240217 mentions this issue: internal/fetch/dochtml/internal/render: preserve existing doc when trimming

@gopherbot
Copy link

Change https://golang.org/cl/276632 mentions this issue: internal/godoc/dochtml/internal/render: fix long-literal comment rendering

@jba
Copy link
Contributor

jba commented Dec 9, 2020

Using a CommentedNode also turns off the comments added by Fprint, like "contains filtered or unexported fields". It's difficult to add those back manually, and I couldn't get rid of some extra blank lines. See https://golang.org/cl/276632. It's unclear if this is WAI or a bug.

All this is in service of modifying the doc to shorten long literals, replacing them with comments. If there were a way to do that by adding comment nodes directly to the AST, then that might be simpler.

gopherbot pushed a commit to golang/pkgsite that referenced this issue Dec 14, 2020
@jba
internal/godoc/dochtml/internal/render: fix long-literal comment rend…
c069bf1 
…ering

The presence of a long literal caused all struct fields comments to be
removed.

This imperfectly fixes the problem by collecting all comments when
walking the declaration AST. Previously, it was thought that the
comments in an ast.CommentedNode would augment the existing comments
instead of replacing them (see golang/go#39219). The fix, as in
https://golang.org/cl/240217 and copied here, is to collect all
comments during the walk.

This isn't sufficient, because using a CommentedNode also turns off
the "contains filtered or unexported" comments that are normally added
during printing. So we have to add them back manually, which is tricky,
and can't be done perfectly (there are extra blank lines).

For golang/go#42425

Change-Id: I0bd8e5ddfc764bc3c7610575e1e3eedf9c0bfd84
Reviewed-on: https://go-review.googlesource.com/c/pkgsite/+/276632
Trust: Jonathan Amsterdam <jba@google.com>
Run-TryBot: Jonathan Amsterdam <jba@google.com>
Reviewed-by: Julie Qiu <julie@golang.org>
@gopherbot
Copy link

Change https://golang.org/cl/284235 mentions this issue: internal/godoc/dochtml/internal/render: improve long-literal handling

gopherbot pushed a commit to golang/pkgsite that referenced this issue Jan 20, 2021
@jba
internal/godoc/dochtml/internal/render: improve long-literal handling
92df1cd 
Using an ast.CommentedNode add comments for long literals does not
work well (golang/go#42425, golang/go#39219). Instead, modify
the AST in place, adding comments directly to the existing nodes.

Protect this change under an experiment until we have more
confidence that it doesn't make things worse.

Fixes golang/go#42425

Change-Id: I7e92356beb7fb873271bb9dd5ee37773d750aeb6
Reviewed-on: https://go-review.googlesource.com/c/pkgsite/+/284235
Trust: Jonathan Amsterdam <jba@google.com>
Run-TryBot: Jonathan Amsterdam <jba@google.com>
TryBot-Result: kokoro <noreply+kokoro@google.com>
Reviewed-by: Julie Qiu <julie@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Documentation NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
3 participants
@dmitshur @gopherbot @jba