Skip to content

Commit 509ed1c

Browse files
adonovangopherbot
authored andcommitted
gopls/internal/golang: work around bug in go/doc
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 <[email protected]> Reviewed-by: Robert Findley <[email protected]> Auto-Submit: Alan Donovan <[email protected]>
1 parent 9551398 commit 509ed1c

File tree

5 files changed

+217
-85
lines changed

5 files changed

+217
-85
lines changed

gopls/internal/golang/pkgdoc.go

+58-7
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,9 @@ import (
3838
"golang.org/x/tools/gopls/internal/cache"
3939
"golang.org/x/tools/gopls/internal/protocol"
4040
"golang.org/x/tools/gopls/internal/util/astutil"
41+
"golang.org/x/tools/gopls/internal/util/bug"
4142
"golang.org/x/tools/gopls/internal/util/safetoken"
43+
"golang.org/x/tools/gopls/internal/util/slices"
4244
"golang.org/x/tools/internal/typesinternal"
4345
)
4446

@@ -68,7 +70,43 @@ func RenderPackageDoc(pkg *cache.Package, posURL func(filename string, line, col
6870
Name: pkg.Types().Name(),
6971
Files: fileMap,
7072
}
71-
docpkg := doc.New(astpkg, pkg.Types().Path(), doc.PreserveAST)
73+
// PreserveAST mode only half works (golang/go#66449): it still
74+
// mutates ASTs when filtering out non-exported symbols.
75+
// As a workaround, enable AllDecls to suppress filtering,
76+
// and do it ourselves.
77+
mode := doc.PreserveAST | doc.AllDecls
78+
docpkg := doc.New(astpkg, pkg.Types().Path(), mode)
79+
80+
// Discard non-exported symbols.
81+
// TODO(adonovan): do this conditionally, and expose option in UI.
82+
const showUnexported = false
83+
if !showUnexported {
84+
var (
85+
unexported = func(name string) bool { return !token.IsExported(name) }
86+
filterValues = func(slice *[]*doc.Value) {
87+
delValue := func(v *doc.Value) bool {
88+
v.Names = slices.DeleteFunc(v.Names, unexported)
89+
return len(v.Names) == 0
90+
}
91+
*slice = slices.DeleteFunc(*slice, delValue)
92+
}
93+
filterFuncs = func(funcs *[]*doc.Func) {
94+
*funcs = slices.DeleteFunc(*funcs, func(v *doc.Func) bool {
95+
return unexported(v.Name)
96+
})
97+
}
98+
)
99+
filterValues(&docpkg.Consts)
100+
filterValues(&docpkg.Vars)
101+
filterFuncs(&docpkg.Funcs)
102+
docpkg.Types = slices.DeleteFunc(docpkg.Types, func(t *doc.Type) bool {
103+
filterValues(&t.Consts)
104+
filterValues(&t.Vars)
105+
filterFuncs(&t.Funcs)
106+
filterFuncs(&t.Methods)
107+
return unexported(t.Name)
108+
})
109+
}
72110

73111
// Ensure doc links (e.g. "[fmt.Println]") become valid links.
74112
docpkg.Printer().DocLinkURL = func(link *comment.DocLink) string {
@@ -181,9 +219,22 @@ function httpGET(url) {
181219
for _, file := range pkg.CompiledGoFiles() {
182220
if astutil.NodeContains(file.File, n.Pos()) {
183221
pos := n.Pos()
222+
223+
// emit emits source in the interval [pos:to] and updates pos.
184224
emit := func(to token.Pos) {
185-
start, _ := safetoken.Offset(file.Tok, pos)
186-
end, _ := safetoken.Offset(file.Tok, to)
225+
// Ident and BasicLit always have a valid pos.
226+
// (Failure means the AST has been corrupted.)
227+
if !to.IsValid() {
228+
bug.Reportf("invalid Pos")
229+
}
230+
start, err := safetoken.Offset(file.Tok, pos)
231+
if err != nil {
232+
bug.Reportf("invalid start Pos: %v", err)
233+
}
234+
end, err := safetoken.Offset(file.Tok, to)
235+
if err != nil {
236+
bug.Reportf("invalid end Pos: %v", err)
237+
}
187238
buf.WriteString(escape(string(file.Src[start:end])))
188239
pos = to
189240
}
@@ -295,9 +346,9 @@ function httpGET(url) {
295346
values := func(vals []*doc.Value) {
296347
for _, v := range vals {
297348
// declaration
298-
decl2 := v.Decl
349+
decl2 := *v.Decl // shallow copy
299350
decl2.Doc = nil
300-
fmt.Fprintf(&buf, "<pre class='code'>%s</pre>\n", nodeHTML(decl2))
351+
fmt.Fprintf(&buf, "<pre class='code'>%s</pre>\n", nodeHTML(&decl2))
301352

302353
// comment (if any)
303354
fmt.Fprintf(&buf, "<div class='comment'>%s</div>\n", docpkg.HTML(v.Doc))
@@ -346,9 +397,9 @@ function httpGET(url) {
346397

347398
// declaration
348399
// TODO(adonovan): excise non-exported struct fields somehow.
349-
decl2 := doctype.Decl
400+
decl2 := *doctype.Decl // shallow copy
350401
decl2.Doc = nil
351-
fmt.Fprintf(&buf, "<pre class='code'>%s</pre>\n", nodeHTML(decl2))
402+
fmt.Fprintf(&buf, "<pre class='code'>%s</pre>\n", nodeHTML(&decl2))
352403

353404
// comment (if any)
354405
fmt.Fprintf(&buf, "<div class='comment'>%s</div>\n", docpkg.HTML(doctype.Doc))

gopls/internal/test/integration/misc/webserver_test.go

+131-76
Original file line numberDiff line numberDiff line change
@@ -32,87 +32,18 @@ const A = 1
3232
// EOF
3333
`
3434
Run(t, files, func(t *testing.T, env *Env) {
35-
env.OpenFile("a/a.go")
36-
37-
// Invoke the "View package documentation" code
38-
// action to start the server.
39-
var docAction *protocol.CodeAction
40-
actions := env.CodeAction("a/a.go", nil)
41-
for _, act := range actions {
42-
if act.Title == "View package documentation" {
43-
docAction = &act
44-
break
45-
}
46-
}
47-
if docAction == nil {
48-
t.Fatalf("can't find action with Title 'View package documentation', only %#v",
49-
actions)
50-
}
51-
52-
// Execute the command.
53-
// Its side effect should be a single showDocument request.
54-
params := &protocol.ExecuteCommandParams{
55-
Command: docAction.Command.Command,
56-
Arguments: docAction.Command.Arguments,
57-
}
58-
var result command.DebuggingResult
59-
env.ExecuteCommand(params, &result)
60-
61-
// shownDocument returns the first shown document matching the URI prefix.
62-
//
63-
// TODO(adonovan): the integration test framework
64-
// needs a way to reset ShownDocuments so they don't
65-
// accumulate, necessitating the fragile prefix hack.
66-
shownDocument := func(prefix string) *protocol.ShowDocumentParams {
67-
var shown []*protocol.ShowDocumentParams
68-
env.Await(ShownDocuments(&shown))
69-
var first *protocol.ShowDocumentParams
70-
for _, sd := range shown {
71-
if strings.HasPrefix(sd.URI, prefix) {
72-
if first != nil {
73-
t.Errorf("got multiple showDocument requests: %#v", shown)
74-
break
75-
}
76-
first = sd
77-
}
78-
}
79-
return first
80-
}
81-
82-
// get fetches the content of a document over HTTP.
83-
get := func(url string) []byte {
84-
resp, err := http.Get(url)
85-
if err != nil {
86-
t.Fatal(err)
87-
}
88-
defer resp.Body.Close()
89-
got, err := io.ReadAll(resp.Body)
90-
if err != nil {
91-
t.Fatal(err)
92-
}
93-
return got
94-
}
95-
96-
checkMatch := func(got []byte, pattern string) {
97-
if !regexp.MustCompile(pattern).Match(got) {
98-
t.Errorf("input did not match pattern %q; got:\n%s",
99-
pattern, got)
100-
}
101-
}
102-
10335
// Assert that the HTML page contains the expected const declaration.
10436
// (We may need to make allowances for HTML markup.)
105-
shownDoc := shownDocument("http:")
106-
t.Log("showDocument(package doc) URL:", shownDoc.URI)
107-
doc1 := get(shownDoc.URI)
108-
checkMatch(doc1, "const A =.*1")
37+
uri1 := viewPkgDoc(t, env, "a/a.go")
38+
doc1 := get(t, uri1)
39+
checkMatch(t, true, doc1, "const A =.*1")
10940

11041
// Check that edits to the buffer (even unsaved) are
11142
// reflected in the HTML document.
11243
env.RegexpReplace("a/a.go", "// EOF", "func NewFunc() {}")
11344
env.Await(env.DoneDiagnosingChanges())
114-
doc2 := get(shownDoc.URI)
115-
checkMatch(doc2, "func NewFunc")
45+
doc2 := get(t, uri1)
46+
checkMatch(t, true, doc2, "func NewFunc")
11647

11748
// TODO(adonovan): assert some basic properties of the
11849
// HTML document using something like
@@ -129,10 +60,10 @@ const A = 1
12960
// downcall, this time for a "file:" URL, causing the
13061
// client editor to navigate to the source file.
13162
t.Log("extracted /open URL", openURL)
132-
get(openURL)
63+
get(t, openURL)
13364

13465
// Check that that shown location is that of NewFunc.
135-
shownSource := shownDocument("file:")
66+
shownSource := shownDocument(t, env, "file:")
13667
gotLoc := protocol.Location{
13768
URI: protocol.DocumentURI(shownSource.URI), // fishy conversion
13869
Range: *shownSource.Selection,
@@ -144,3 +75,127 @@ const A = 1
14475
}
14576
})
14677
}
78+
79+
func TestRenderNoPanic66449(t *testing.T) {
80+
// This particular input triggered a latent bug in doc.New
81+
// that would corrupt the AST while filtering out unexported
82+
// symbols such as b, causing nodeHTML to panic.
83+
// Now it doesn't crash.
84+
const files = `
85+
-- go.mod --
86+
module example.com
87+
88+
-- a/a.go --
89+
package a
90+
91+
var A, b = 0, 0
92+
93+
func ExportedFunc()
94+
func unexportedFunc()
95+
type ExportedType int
96+
type unexportedType int
97+
`
98+
Run(t, files, func(t *testing.T, env *Env) {
99+
uri1 := viewPkgDoc(t, env, "a/a.go")
100+
doc1 := get(t, uri1)
101+
// (Ideally our code rendering would also
102+
// eliminate unexported symbols...)
103+
// TODO(adonovan): when we emit var anchors,
104+
// check that #A exists and #b does not.
105+
checkMatch(t, true, doc1, "var A, b = .*0.*0")
106+
107+
// Unexported funcs/types/... must still be discarded.
108+
checkMatch(t, true, doc1, "ExportedFunc")
109+
checkMatch(t, false, doc1, "unexportedFunc")
110+
checkMatch(t, true, doc1, "ExportedType")
111+
checkMatch(t, false, doc1, "unexportedType")
112+
})
113+
}
114+
115+
// viewPkgDoc invokes the "View package documention" code action in
116+
// the specified file. It returns the URI of the document, or fails
117+
// the test.
118+
func viewPkgDoc(t *testing.T, env *Env, filename string) protocol.URI {
119+
env.OpenFile(filename)
120+
121+
// Invoke the "View package documentation" code
122+
// action to start the server.
123+
var docAction *protocol.CodeAction
124+
actions := env.CodeAction(filename, nil)
125+
for _, act := range actions {
126+
if act.Title == "View package documentation" {
127+
docAction = &act
128+
break
129+
}
130+
}
131+
if docAction == nil {
132+
t.Fatalf("can't find action with Title 'View package documentation', only %#v",
133+
actions)
134+
}
135+
136+
// Execute the command.
137+
// Its side effect should be a single showDocument request.
138+
params := &protocol.ExecuteCommandParams{
139+
Command: docAction.Command.Command,
140+
Arguments: docAction.Command.Arguments,
141+
}
142+
var result command.DebuggingResult
143+
env.ExecuteCommand(params, &result)
144+
145+
doc := shownDocument(t, env, "http:")
146+
if doc == nil {
147+
t.Fatalf("no showDocument call had 'http:' prefix")
148+
}
149+
t.Log("showDocument(package doc) URL:", doc.URI)
150+
return doc.URI
151+
}
152+
153+
// shownDocument returns the first shown document matching the URI prefix.
154+
// It may be nil.
155+
//
156+
// TODO(adonovan): the integration test framework
157+
// needs a way to reset ShownDocuments so they don't
158+
// accumulate, necessitating the fragile prefix hack.
159+
func shownDocument(t *testing.T, env *Env, prefix string) *protocol.ShowDocumentParams {
160+
t.Helper()
161+
var shown []*protocol.ShowDocumentParams
162+
env.Await(ShownDocuments(&shown))
163+
var first *protocol.ShowDocumentParams
164+
for _, sd := range shown {
165+
if strings.HasPrefix(sd.URI, prefix) {
166+
if first != nil {
167+
t.Errorf("got multiple showDocument requests: %#v", shown)
168+
break
169+
}
170+
first = sd
171+
}
172+
}
173+
return first
174+
}
175+
176+
// get fetches the content of a document over HTTP.
177+
func get(t *testing.T, url string) []byte {
178+
t.Helper()
179+
resp, err := http.Get(url)
180+
if err != nil {
181+
t.Fatal(err)
182+
}
183+
defer resp.Body.Close()
184+
got, err := io.ReadAll(resp.Body)
185+
if err != nil {
186+
t.Fatal(err)
187+
}
188+
return got
189+
}
190+
191+
// checkMatch asserts that got matches (or doesn't match, if !want) the pattern.
192+
func checkMatch(t *testing.T, want bool, got []byte, pattern string) {
193+
t.Helper()
194+
if regexp.MustCompile(pattern).Match(got) != want {
195+
if want {
196+
t.Errorf("input did not match wanted pattern %q; got:\n%s", pattern, got)
197+
} else {
198+
t.Errorf("input matched unwanted pattern %q; got:\n%s", pattern, got)
199+
}
200+
}
201+
}

gopls/internal/util/slices/slices.go

+26
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,32 @@ func Grow[S ~[]E, E any](s S, n int) S {
7474
return s
7575
}
7676

77+
// DeleteFunc removes any elements from s for which del returns true,
78+
// returning the modified slice.
79+
// DeleteFunc zeroes the elements between the new length and the original length.
80+
// TODO(adonovan): use go1.21 slices.DeleteFunc.
81+
func DeleteFunc[S ~[]E, E any](s S, del func(E) bool) S {
82+
i := IndexFunc(s, del)
83+
if i == -1 {
84+
return s
85+
}
86+
// Don't start copying elements until we find one to delete.
87+
for j := i + 1; j < len(s); j++ {
88+
if v := s[j]; !del(v) {
89+
s[i] = v
90+
i++
91+
}
92+
}
93+
clear(s[i:]) // zero/nil out the obsolete elements, for GC
94+
return s[:i]
95+
}
96+
97+
func clear[T any](slice []T) {
98+
for i := range slice {
99+
slice[i] = *new(T)
100+
}
101+
}
102+
77103
// Remove removes all values equal to elem from slice.
78104
//
79105
// The closest equivalent in the standard slices package is:

internal/imports/imports.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -107,7 +107,7 @@ func ApplyFixes(fixes []*ImportFix, filename string, src []byte, opt *Options, e
107107
}
108108

109109
// formatFile formats the file syntax tree.
110-
// It may mutate the token.FileSet.
110+
// It may mutate the token.FileSet and the ast.File.
111111
//
112112
// If an adjust function is provided, it is called after formatting
113113
// with the original source (formatFile's src parameter) and the

internal/imports/sortimports.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ import (
1818
// sortImports sorts runs of consecutive import lines in import blocks in f.
1919
// It also removes duplicate imports when it is possible to do so without data loss.
2020
//
21-
// It may mutate the token.File.
21+
// It may mutate the token.File and the ast.File.
2222
func sortImports(localPrefix string, tokFile *token.File, f *ast.File) {
2323
for i, d := range f.Decls {
2424
d, ok := d.(*ast.GenDecl)

0 commit comments

Comments
 (0)