internal/godoc/dochtml/internal/render: improve long-literal handling
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>
diff --git a/internal/experiment.go b/internal/experiment.go
index e45d0a2..9c7b39e 100644
--- a/internal/experiment.go
+++ b/internal/experiment.go
@@ -9,6 +9,7 @@
ExperimentDirectoryTree = "directory-tree"
ExperimentNotAtLatest = "not-at-latest"
ExperimentRedirectedFromBanner = "redirected-from-banner"
+ ExperimentRewriteAST = "rewrite-ast"
)
// Experiments represents all of the active experiments in the codebase and
@@ -17,6 +18,7 @@
ExperimentDirectoryTree: "Enable the directory tree layout on the unit page.",
ExperimentNotAtLatest: "Enable the display of a 'not at latest' badge.",
ExperimentRedirectedFromBanner: "Display banner with path that request was redirected from.",
+ ExperimentRewriteAST: "Rewrite AST for long literals when rendering doc instead of adding comments.",
}
// Experiment holds data associated with an experimental feature for frontend
diff --git a/internal/godoc/dochtml/internal/render/linkify.go b/internal/godoc/dochtml/internal/render/linkify.go
index 64afdcb..0f7a1f0 100644
--- a/internal/godoc/dochtml/internal/render/linkify.go
+++ b/internal/godoc/dochtml/internal/render/linkify.go
@@ -377,14 +377,20 @@
})
// Trim large string literals and composite literals.
- //ast.Fprint(os.Stdout, nil, decl, nil)
- v := &declVisitor{}
- ast.Walk(v, decl)
-
+ var (
+ b bytes.Buffer
+ n interface{}
+ )
+ if r.rewriteAST {
+ n = rewriteDecl(decl, 125, 100)
+ } else {
+ v := &declVisitor{}
+ ast.Walk(v, decl)
+ n = &printer.CommentedNode{Node: decl, Comments: v.Comments}
+ }
// Format decl as Go source code file.
- var b bytes.Buffer
p := printer.Config{Mode: printer.UseSpaces | printer.TabIndent, Tabwidth: 4}
- p.Fprint(&b, r.fset, &printer.CommentedNode{Node: decl, Comments: v.Comments})
+ p.Fprint(&b, r.fset, n)
src := b.Bytes()
fset := token.NewFileSet()
file := fset.AddFile("", fset.Base(), b.Len())
@@ -706,3 +712,64 @@
})
return m
}
+
+// rewriteDecl rewrites n by removing strings longer than maxStringSize and
+// composite literals longer than maxElements.
+func rewriteDecl(n ast.Decl, maxStringSize, maxElements int) ast.Decl {
+ v := &rewriteVisitor{maxStringSize, maxElements}
+ ast.Walk(v, n)
+ return n
+}
+
+type rewriteVisitor struct {
+ maxStringSize, maxElements int
+}
+
+func (v *rewriteVisitor) Visit(n ast.Node) ast.Visitor {
+ switch n := n.(type) {
+ case *ast.ValueSpec:
+ for _, val := range n.Values {
+ v.rewriteLongValue(val, &n.Comment)
+ }
+ case *ast.Field:
+ if n.Tag != nil {
+ v.rewriteLongValue(n.Tag, &n.Comment)
+ }
+ }
+ return v
+}
+
+func (v *rewriteVisitor) rewriteLongValue(n ast.Node, pcg **ast.CommentGroup) {
+ switch n := n.(type) {
+ case *ast.BasicLit:
+ if n.Kind != token.STRING {
+ return
+ }
+ size := len(n.Value) - 2 // subtract quotation marks
+ if size <= v.maxStringSize {
+ return
+ }
+ addComment(pcg, n.ValuePos, fmt.Sprintf("/* %d-byte string literal not displayed */", size))
+ if len(n.Value) == 0 {
+ // Impossible, but avoid the panic just in case.
+ return
+ }
+ if quote := n.Value[0]; quote == '`' {
+ n.Value = "``"
+ } else {
+ n.Value = `""`
+ }
+ case *ast.CompositeLit:
+ if len(n.Elts) > v.maxElements {
+ addComment(pcg, n.Lbrace, fmt.Sprintf("/* %d elements not displayed */", len(n.Elts)))
+ n.Elts = n.Elts[:0]
+ }
+ }
+}
+
+func addComment(cg **ast.CommentGroup, pos token.Pos, text string) {
+ if *cg == nil {
+ *cg = &ast.CommentGroup{}
+ }
+ (*cg).List = append((*cg).List, &ast.Comment{Slash: pos, Text: text})
+}
diff --git a/internal/godoc/dochtml/internal/render/linkify_test.go b/internal/godoc/dochtml/internal/render/linkify_test.go
index 18cdd41..9e7e581 100644
--- a/internal/godoc/dochtml/internal/render/linkify_test.go
+++ b/internal/godoc/dochtml/internal/render/linkify_test.go
@@ -16,6 +16,8 @@
"github.com/google/go-cmp/cmp"
"github.com/google/safehtml"
"github.com/google/safehtml/testconversions"
+ "golang.org/x/pkgsite/internal"
+ "golang.org/x/pkgsite/internal/experiment"
"golang.org/x/pkgsite/internal/godoc/internal/doc"
)
@@ -176,10 +178,12 @@
}
func TestDeclHTML(t *testing.T) {
+ pkgTime2, fsetTime2 := mustLoadPackage("time") // Needed because DeclHTML changes the AST.
for _, test := range []struct {
- name string
- symbol string
- want string
+ name string
+ symbol string
+ want string
+ wantRewrite string
}{
{
name: "const",
@@ -208,6 +212,10 @@
</span> <span class="comment">// contains filtered or unexported fields</span>
}`,
+ wantRewrite: `type Ticker struct {
+<span id="Ticker.C" data-kind="field"> C <-chan <a href="#Time">Time</a> <span class="comment">// The channel on which the ticks are delivered.</span>
+</span> <span class="comment">// contains filtered or unexported fields</span>
+}`,
},
{
name: "func",
@@ -229,6 +237,11 @@
<span class="comment">// contains filtered or unexported methods</span>
}`,
+ wantRewrite: `type Iface interface {
+<span id="Iface.M" data-kind="method"> <span class="comment">// Method comment.</span>
+</span> M()
+ <span class="comment">// contains filtered or unexported methods</span>
+}`,
},
{
name: "long literal",
@@ -243,6 +256,26 @@
<span class="comment">// contains filtered or unexported fields</span>
}`,
+ wantRewrite: `type TooLongLiteral struct {
+<span id="TooLongLiteral.Name" data-kind="field"> <span class="comment">// The name.</span>
+</span> Name <a href="/builtin#string">string</a>
+
+<span id="TooLongLiteral.Labels" data-kind="field"> <span class="comment">// The labels.</span>
+</span> Labels <a href="/builtin#int">int</a> ` + "``" + ` <span class="comment">/* 137-byte string literal not displayed */</span>
+ <span class="comment">// contains filtered or unexported fields</span>
+}`,
+ },
+ {
+ name: "filtered comment",
+ symbol: "FieldTagFiltered",
+ want: `type FieldTagFiltered struct {
+<span id="FieldTagFiltered.Name" data-kind="field"> Name <a href="/builtin#string">string</a> ` + "`tag`" + ` <span class="comment">// contains filtered or unexported fields</span>
+</span>
+}`,
+ wantRewrite: `type FieldTagFiltered struct {
+<span id="FieldTagFiltered.Name" data-kind="field"> Name <a href="/builtin#string">string</a> ` + "`tag`" + `
+</span> <span class="comment">// contains filtered or unexported fields</span>
+}`,
},
} {
t.Run(test.name, func(t *testing.T) {
@@ -250,9 +283,17 @@
r := New(context.Background(), fsetTime, pkgTime, nil)
got := r.DeclHTML("", decl).Decl.String()
if diff := cmp.Diff(test.want, got); diff != "" {
- fmt.Println(got)
t.Errorf("mismatch (-want +got)\n%s", diff)
}
+
+ if test.wantRewrite != "" {
+ r = New(experiment.NewContext(context.Background(), internal.ExperimentRewriteAST), fsetTime2, pkgTime2, nil)
+ decl = declForName(t, pkgTime2, test.symbol)
+ got := r.DeclHTML("", decl).Decl.String()
+ if diff := cmp.Diff(test.wantRewrite, got); diff != "" {
+ t.Errorf("mismatch (-want +got)\n%s", diff)
+ }
+ }
})
}
}
diff --git a/internal/godoc/dochtml/internal/render/render.go b/internal/godoc/dochtml/internal/render/render.go
index 21c58f5..591859f 100644
--- a/internal/godoc/dochtml/internal/render/render.go
+++ b/internal/godoc/dochtml/internal/render/render.go
@@ -15,6 +15,8 @@
"github.com/google/safehtml"
"github.com/google/safehtml/template"
+ "golang.org/x/pkgsite/internal"
+ "golang.org/x/pkgsite/internal/experiment"
"golang.org/x/pkgsite/internal/godoc/internal/doc"
)
@@ -40,6 +42,7 @@
docTmpl *template.Template
exampleTmpl *template.Template
links []Link // Links removed from package overview to be displayed elsewhere.
+ rewriteAST bool
}
type Options struct {
@@ -119,6 +122,7 @@
disablePermalinks: disablePermalinks,
docTmpl: docDataTmpl,
exampleTmpl: exampleTmpl,
+ rewriteAST: experiment.IsActive(ctx, internal.ExperimentRewriteAST),
}
}
diff --git a/internal/godoc/dochtml/internal/render/testdata/rewrites.go b/internal/godoc/dochtml/internal/render/testdata/rewrites.go
new file mode 100644
index 0000000..d6a80fe
--- /dev/null
+++ b/internal/godoc/dochtml/internal/render/testdata/rewrites.go
@@ -0,0 +1,41 @@
+// Copyright 2021 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+// This package has symbols for testing changes
+// to the AST to improve the generated documentation.
+// See TestRewriteDecl in ../linkify_test.go.
+package rewrites
+
+const OK = "ok"
+
+const OKWithComment = "ok" // comment
+
+const Long = "long"
+
+const LongWithComment = "long" // comment
+
+const (
+ GroupOK = "ok"
+ GroupWithComment = "ok" // comment
+ GroupLong = "long"
+ GroupLongWithComment = "long" // comment
+)
+
+type FieldTag struct {
+ F1 int `ok`
+ F2 int `long`
+ F3 int `long` // comment
+}
+
+type FieldTagFiltered struct {
+ u int
+
+ Name string `long` // comment
+}
+
+var CompositeOK = []int{1, 2}
+
+var CompositeLong = []int{1, 2, 3}
+
+var CompositeLongComment = []int{1, 2, 3} // comment
diff --git a/internal/godoc/dochtml/internal/render/testdata/time.go b/internal/godoc/dochtml/internal/render/testdata/time.go
index 50e71eb..7f50890 100644
--- a/internal/godoc/dochtml/internal/render/testdata/time.go
+++ b/internal/godoc/dochtml/internal/render/testdata/time.go
@@ -869,3 +869,9 @@
u()
}
+
+type FieldTagFiltered struct {
+ u int
+
+ Name string `tag`
+}
