Skip to content

Commit 83f34c1

Browse files
committed
Address review angles A, D, Sweep: chain boundary, var-assign through render, docs
Multiple findings from the angles: A1 — ChainNode boundary case (`(.Params).summary`): the `Params`/`summary` adjacency straddles the receiver/Field boundary. Neither half alone has the pair. Added tailIdents to flatten a chain receiver into one Ident slice and check for the pair across the boundary. Caught: bare ChainNode, nested ChainNode `((.A).Params).summary`, and function-call receiver via fallback recurse. A2 — `{{ $s := .RenderString .Params.summary }}` was being flagged as variable assignment of the raw summary, but the RHS routes summary through RenderString so the bound name holds template.HTML (rendered Markdown). Hugo emits template.HTML without re-escaping, so a later `{{ $s }}` ships rendered output. pipeAssignsSummary now skips the flag when the RHS already outputs via RenderString. Sweep #1 — Forbidden-forms list in docs was missing the TemplateNode case (`{{ template "name" .Params.summary }}` and the `{{ block }}` shorthand). Added. Sweep #2 — Vacuous-pass guard was `scanned >= 5` but the tree has 24 .html files. Raised to 20. Sweep #3 — `assert.Empty(t, formatted)` had no diagnostic message; sibling `ioErrors` line did. Added. Sweep #4 / D1 — Commented the deliberate case-sensitivity asymmetry: identsReferenceSummary uses EqualFold (Hugo Params map is case-insensitive); cmdIsRenderString uses `==` (Go method names are case-sensitive). Without the comment a maintainer "normalising" one would silently break the rule. Sweep #7 / D6 — Commented the inner `if n == nil` guard inside the *parse.ListNode case. The typed-nil ElseList of an if-without-else bypasses the outer guard; removing the inner check would re-introduce a panic. tailIdents — pruned the unreachable VariableNode case (no template syntax produces a ChainNode whose receiver is a bare VariableNode; `$s.Field` parses as VariableNode with the field appended to its own Ident). Per CLAUDE.md, no defensive branch without a red/green driver. Coverage: templatecheck package now at 100.0% of statements. https://claude.ai/code/session_01Ly1xEwP5pfsLrtTCTKBQE3
1 parent 267bee3 commit 83f34c1

4 files changed

Lines changed: 141 additions & 11 deletions

File tree

docs/development/website-config.md

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -158,6 +158,12 @@ Forbidden forms:
158158
- `{{ range .Params.summary }}` — ranging over a
159159
string iterates rune-by-rune and emits each code
160160
point as an integer.
161+
- `{{ template "name" .Params.summary }}` and
162+
`{{ block "name" .Params.summary }}` — these pass
163+
the summary as the sub-template's dot. The
164+
sub-template lives in a separate parse tree; the
165+
scanner cannot follow the rebinding across the
166+
boundary.
161167
- The bare `{{ .Params.summary }}` action.
162168
- Variable assignment in any context — `{{ $s := .Params.summary }}`,
163169
`{{ if $s := .Params.summary }}`,

internal/release/template_summary_test.go

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -61,11 +61,13 @@ func TestSummaryFrontMatterRenderedThroughRenderString(t *testing.T) {
6161
for _, v := range violations {
6262
formatted = append(formatted, fmt.Sprintf("%s:%d: %s", v.Path, v.Line, v.Why))
6363
}
64-
assert.Empty(t, formatted)
64+
assert.Empty(t, formatted, "summary front-matter rendering violations")
6565
assert.Empty(t, ioErrors, "filesystem errors during scan")
6666
// Guard against the test passing vacuously if website/layouts/
67-
// ever disappears or the walker is misconfigured: at least the
68-
// `_default/baseof.html` + four page-rendering layouts must
69-
// have been scanned.
70-
assert.GreaterOrEqual(t, scanned, 5, "expected to scan at least 5 .html files; got %d", scanned)
67+
// ever disappears or the walker is misconfigured. The tree
68+
// currently holds 24 .html files (_default/, partials/,
69+
// shortcodes/, _markup/, rule/, index.html); set the floor at
70+
// 20 to catch a catastrophic regression while leaving headroom
71+
// for legitimate template cleanup.
72+
assert.GreaterOrEqual(t, scanned, 20, "expected to scan at least 20 .html files; got %d", scanned)
7173
}

internal/templatecheck/templatecheck.go

Lines changed: 58 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -113,6 +113,13 @@ func (w *walker) walk(n parse.Node) {
113113
}
114114
switch n := n.(type) {
115115
case *parse.ListNode:
116+
// IfNode.ElseList / WithNode.ElseList / RangeNode.ElseList
117+
// are *parse.ListNode pointers that are nil when no `else`
118+
// clause is present. A nil typed pointer wrapped in a
119+
// non-nil interface bypasses the outer `if n == nil` guard,
120+
// so this inner check is what saves us from dereferencing
121+
// it. Removing it would re-introduce a panic in every
122+
// `{{ if ... }}...{{ end }}` template without an else.
116123
if n == nil {
117124
return
118125
}
@@ -206,9 +213,40 @@ func identsReferenceSummary(idents []string) bool {
206213
}
207214

208215
func fieldIsSummary(f *parse.FieldNode) bool { return identsReferenceSummary(f.Ident) }
209-
func chainIsSummary(c *parse.ChainNode) bool { return identsReferenceSummary(c.Field) }
210216
func variableIsSummary(v *parse.VariableNode) bool { return identsReferenceSummary(v.Ident) }
211217

218+
// chainIsSummary checks whether a ChainNode flattens to a chain
219+
// containing `Params.summary` — including the boundary case
220+
// `(.Params).summary` where the adjacency straddles the receiver
221+
// and the trailing Field list (receiver ends with `Params`, Field
222+
// starts with `summary`). Without the flatten step the trailing
223+
// Field `[summary]` alone has no `Params`-`summary` pair.
224+
func chainIsSummary(c *parse.ChainNode) bool {
225+
return identsReferenceSummary(append(tailIdents(c.Node), c.Field...))
226+
}
227+
228+
// tailIdents extracts the terminal identifier chain of a chain
229+
// receiver. For FieldNode it's the Ident slice. For a ChainNode
230+
// receiver it recurses and concatenates. For a PipeNode wrapping
231+
// a single expression in parens — the common `(...)` receiver
232+
// shape — it unwraps to the wrapped node. Returns nil for shapes
233+
// the flattener cannot trace (multi-cmd pipes, function calls);
234+
// callers that need to scan those fall back to
235+
// pipeReferencesSummary, which handles arbitrary arg nesting.
236+
func tailIdents(n parse.Node) []string {
237+
switch x := n.(type) {
238+
case *parse.FieldNode:
239+
return x.Ident
240+
case *parse.ChainNode:
241+
return append(tailIdents(x.Node), x.Field...)
242+
case *parse.PipeNode:
243+
if len(x.Cmds) == 1 && len(x.Cmds[0].Args) == 1 {
244+
return tailIdents(x.Cmds[0].Args[0])
245+
}
246+
}
247+
return nil
248+
}
249+
212250
func pipeReferencesSummary(p *parse.PipeNode) bool {
213251
if p == nil {
214252
return false
@@ -253,15 +291,22 @@ func argReferencesSummary(arg parse.Node) bool {
253291

254292
// pipeAssignsSummary returns true if the pipe — or any sub-pipe
255293
// nested inside one of its command args — declares variables
256-
// whose right-hand value references the summary. The recursion
257-
// catches forms like `{{ .RenderString (dict) ($s := .Params.summary) }}`
258-
// where the binding hides in a sub-pipeline arg and the outer
259-
// pipe's Decl is empty.
294+
// whose right-hand value is the raw summary. An assignment whose
295+
// right-hand pipe routes summary through `.RenderString` first
296+
// is safe: the bound name holds rendered HTML (a `template.HTML`
297+
// value Hugo emits without re-escaping), so a later `{{ $s }}`
298+
// ships the rendered output, not the raw Markdown.
299+
//
300+
// The recursion catches forms like
301+
// `{{ .RenderString (dict) ($s := .Params.summary) }}` where the
302+
// binding hides in a sub-pipeline arg and the outer pipe's Decl
303+
// is empty.
260304
func pipeAssignsSummary(p *parse.PipeNode) bool {
261305
if p == nil {
262306
return false
263307
}
264-
if len(p.Decl) > 0 && pipeReferencesSummary(p) {
308+
if len(p.Decl) > 0 && pipeReferencesSummary(p) &&
309+
!pipeOutputsSummaryViaRenderString(p) {
265310
return true
266311
}
267312
for _, c := range p.Cmds {
@@ -327,6 +372,13 @@ func pipeOutputsSummaryViaRenderString(p *parse.PipeNode) bool {
327372
// (multi-Ident FieldNode), the dollar-context `$.RenderString`
328373
// (VariableNode with Ident `["$", "RenderString"]`), and chain
329374
// receivers (ChainNode).
375+
//
376+
// Comparison is intentionally case-sensitive (`== "RenderString"`),
377+
// not case-insensitive as elsewhere in this file. Hugo's
378+
// `.RenderString` is a Go method on the Page receiver; Go reflects
379+
// methods by exact name and would not dispatch `.renderstring` to
380+
// it. The Params map, by contrast, is a case-insensitive lookup —
381+
// that's why identsReferenceSummary uses EqualFold.
330382
func cmdIsRenderString(c *parse.CommandNode) bool {
331383
if len(c.Args) == 0 {
332384
return false

internal/templatecheck/templatecheck_test.go

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -215,6 +215,76 @@ func TestScan_ChainReceiver(t *testing.T) {
215215
require.Len(t, got, 1)
216216
}
217217

218+
// TestScan_ChainBoundary pins the boundary case where the
219+
// `Params`/`summary` adjacency straddles the chain receiver and
220+
// the trailing field list. Receiver ends with `Params`, Field
221+
// starts with `summary` — neither half alone has the pair, but
222+
// the flattened chain does. Also exercises nested ChainNode
223+
// receivers and the fallback recurse when tailIdents cannot
224+
// trace a complex receiver.
225+
func TestScan_ChainBoundary(t *testing.T) {
226+
cases := []struct {
227+
name string
228+
template string
229+
wantCount int
230+
}{
231+
{"params receiver dot summary bare", `{{ (.Params).summary }}`, 1},
232+
{"params receiver dot summary rendered", `{{ .RenderString (dict) (.Params).summary }}`, 0},
233+
// Nested ChainNode receiver: tailIdents recurses through
234+
// each chain level and stitches the Idents into one slice.
235+
{"nested chain with boundary", `{{ ((.A).Params).summary }}`, 1},
236+
// Function-call receiver — tailIdents returns nil (multi-arg
237+
// command); argReferencesSummary's fallback recurse into
238+
// n.Node via pipeReferencesSummary catches the summary buried
239+
// inside the printf args.
240+
{"function-call receiver", `{{ (printf "%s" .Params.summary).Field }}`, 1},
241+
}
242+
for _, tc := range cases {
243+
t.Run(tc.name, func(t *testing.T) {
244+
got, err := Scan("file.html", tc.template)
245+
require.NoError(t, err)
246+
assert.Len(t, got, tc.wantCount, "violations: %+v", got)
247+
})
248+
}
249+
}
250+
251+
// TestScan_VarAssignWithRender pins that an assignment whose
252+
// right-hand pipe routes summary through `.RenderString` is
253+
// SAFE. The bound name holds rendered HTML (template.HTML),
254+
// not raw Markdown — Hugo emits template.HTML without
255+
// re-escaping, so a later `{{ $s }}` ships rendered output.
256+
// Only assignments of the raw value are flagged.
257+
func TestScan_VarAssignWithRender(t *testing.T) {
258+
cases := []struct {
259+
name string
260+
template string
261+
wantCount int
262+
}{
263+
{
264+
"var := raw summary is flagged",
265+
`{{ $s := .Params.summary }}{{ $s }}`,
266+
1,
267+
},
268+
{
269+
"var := rendered summary is safe",
270+
`{{ $s := .RenderString (dict) .Params.summary }}{{ $s }}`,
271+
0,
272+
},
273+
{
274+
"if-var := rendered summary is safe",
275+
`{{ if $s := .RenderString (dict) .Params.summary }}{{ $s }}{{ end }}`,
276+
0,
277+
},
278+
}
279+
for _, tc := range cases {
280+
t.Run(tc.name, func(t *testing.T) {
281+
got, err := Scan("file.html", tc.template)
282+
require.NoError(t, err)
283+
assert.Len(t, got, tc.wantCount, "violations: %+v", got)
284+
})
285+
}
286+
}
287+
218288
// TestScan_SubPipeVarAssign pins detection of variable assignment
219289
// hidden inside a sub-pipeline argument:
220290
// `{{ .RenderString (dict) ($s := .Params.summary) }}` —

0 commit comments

Comments
 (0)