go/doc: New trashes the AST, even in PreserveAST mode

[adonovan]

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:

// fileExports removes unexported declarations from src in place.
func (r *reader) fileExports(src *ast.File) {
	j := 0
	for _, d := range src.Decls {
		if r.filterDecl(d) {
			src.Decls[j] = d
			j++
		}
	}
	src.Decls = src.Decls[0:j]
}

[gopherbot]

Change https://go.dev/cl/573575 mentions this issue: gopls/internal/golang: work around bug in go/doc

[adonovan]

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 (*reader).fileExports, and to change fileExports not to share the slice's backing array. The challenge is filterDecl: it mutates the underlying array of GenDecl.Specs; we can't shallow-copy the GenDecl too, because then its identity would have changed in Value.Decl, which has observable consequences (e.g. its type info would appear to be missing). Perhaps we need to implement the filtering later, when building up the index, rather than by AST mutation; but this too has observable consequences, and may affect syntax printing. I'm not sure there is a satisfactory solution.

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.