-
Notifications
You must be signed in to change notification settings - Fork 18k
go/doc: New trashes the AST, even in PreserveAST mode #66453
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
Comments
Change https://go.dev/cl/573575 mentions this issue: |
I had a quick crack at a fix, but it's more fiddly than I first thought. It's easy to shallow-copy the ast.File before calling In any case gopls now has a workaround. I've posted my test case below in case it helps: func TestIssue66453(t *testing.T) {
fset := token.NewFileSet()
f, _ := parser.ParseFile(fset, "a.go", "package p; var v int", 0)
pkg := &ast.Package{Name: "foo", Files: map[string]*ast.File{"a.go": f}}
dump := func() string {
var buf strings.Builder
ast.Fprint(&buf, fset, f, nil)
return buf.String()
}
before := dump()
defer func() {
if t.Failed() {
t.Logf("before: %s", before)
}
}()
if len(f.Decls) != 1 {
t.Errorf("len(f.Decls) = %d, want 1 (v)", len(f.Decls))
}
// New(PreserveAST) must not mutate the tree.
_ = New(pkg, "foo", PreserveAST)
after := dump()
defer func() {
if t.Failed() {
t.Logf("after: %s", after)
}
}()
if after != before {
t.Error("AST was modified")
}
} There is a lesson here: don't create APIs that may or may not mutate their arguments; pick a lane, ideally the second one. |
The associated bug was a crash in package doc rendering due to invalid ASTs resulting from mutation caused by doc.New, even in PreserveAST mode. (This bug has been latent since 2018.) The mutation occurs when filtering out unexported symbols. This change is a workaround: we enable AllDecls mode to suppress filtering, and perform the filtering ourselves. Also, add a regression test for the panic, and check the new filtering behavior. This required some test refactoring. Fixes golang/go#66449 Updates golang/go#66453 Change-Id: I434c97e08930728d4894b90c1c7e010e7a922209 Reviewed-on: https://go-review.googlesource.com/c/tools/+/573575 LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Robert Findley <rfindley@google.com> Auto-Submit: Alan Donovan <adonovan@google.com>
The doc.New function mutates the AST, even in PreserveAST mode. See https://go.dev/play/p/YSn-iNSBXsv for a minimal repro. This leads to crashes in gopls (e.g. #66449).
Here's one culprit; there may be others:
The text was updated successfully, but these errors were encountered: