Fix partial decorator detection in partial with blocks with outer range break or continue #14334
Conversation
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. |
There was a problem hiding this comment.
Pull request overview
This PR fixes an issue with partial decorator detection when break or continue statements exist in with blocks that wrap partial calls but are outside of any range block. The fix addresses issue #14333 by adding guards to detect this edge case and handling partial calls wrapped in parentheses (PipeNode).
Key changes:
- Added detection logic to identify break/continue statements outside of range blocks within with statements
- Enhanced
isWithPartialto handle partial calls wrapped in PipeNodes (parentheses) - Modified
handleWithPartialto skip decorator transformation when break/continue exists outside a range
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| tpl/tplimpl/templatetransform.go | Adds hasBreakOrContinueNotInRange function to detect problematic break/continue placement, updates isWithPartial to handle PipeNode wrapping, and adds guard in handleWithPartial to prevent incorrect transformations |
| tpl/templates/decorator_integration_test.go | Adds comprehensive test coverage for partial decorators in parentheses, break/continue within range blocks inside with statements, and the specific issue #14333 scenario |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| case *parse.IfNode: | ||
| if c.hasBreakOrContinueNotInRange(x.List) { | ||
| return true | ||
| } | ||
| if c.hasBreakOrContinueNotInRange(x.ElseList) { | ||
| return true | ||
| } |
There was a problem hiding this comment.
The function should check if x.List and x.ElseList are nil before recursing into them. In Go's text/template parse tree, ElseList can be nil when there's no else branch. While List is typically non-nil, defensive nil checking is warranted here for consistency and safety.
| case *parse.WithNode: | ||
| if c.hasBreakOrContinueNotInRange(x.List) { | ||
| return true | ||
| } | ||
| if c.hasBreakOrContinueNotInRange(x.ElseList) { | ||
| return true | ||
| } |
There was a problem hiding this comment.
The function should check if x.List and x.ElseList are nil before recursing into them. In Go's text/template parse tree, ElseList can be nil when there's no else branch. While List is typically non-nil, defensive nil checking is warranted here for consistency and safety.
| if id1, ok := args[0].(*parse.IdentifierNode); ok && (id1.Ident == "partial" || id1.Ident == "partialCached") { | ||
| first := args[0] | ||
|
|
||
| if pn, ok := first.(*parse.PipeNode); ok { |
There was a problem hiding this comment.
The function accesses pn.Cmds[0] without checking if pn.Cmds is empty or if pn.Cmds[0].Args is accessible. This could cause a panic if the PipeNode has no commands. Add bounds checking before accessing pn.Cmds[0].Args.
| if pn, ok := first.(*parse.PipeNode); ok { | |
| if pn, ok := first.(*parse.PipeNode); ok { | |
| // Ensure there is at least one command before accessing pn.Cmds[0].Args. | |
| if len(pn.Cmds) == 0 || pn.Cmds[0] == nil { | |
| return false | |
| } |
| sn2 := setContext.(*parse.PipeNode).Cmds[0].Args[1].(*parse.PipeNode).Cmds[0].Args[0].(*parse.StringNode) | ||
| sn2.Text = innerHash | ||
| sn2.Quoted = fmt.Sprintf("%q", sn2.Text) | ||
| if pn, ok := withNode.Pipe.Cmds[0].Args[0].(*parse.PipeNode); ok { |
There was a problem hiding this comment.
The code accesses pn.Cmds[0].Args without checking if pn.Cmds is empty. This could cause a panic if the PipeNode has no commands. Add bounds checking before accessing pn.Cmds[0].Args.
| if pn, ok := withNode.Pipe.Cmds[0].Args[0].(*parse.PipeNode); ok { | |
| if pn, ok := withNode.Pipe.Cmds[0].Args[0].(*parse.PipeNode); ok && len(pn.Cmds) > 0 { |
tpl/tplimpl/templatetransform.go
Outdated
| var templatesInnerRe = regexp.MustCompile(`{{\s*(templates\.Inner\b|inner\b)`) | ||
|
|
||
| // hasBreakOrContinueNotInRange returns true if the given list node contains a break or continue statement without being nested in a range. | ||
| func (c *templateTransformContext) hasBreakOrContinueNotInRange(n *parse.ListNode) bool { |
There was a problem hiding this comment.
The function name suggests it returns true when break/continue is "not in range", but the implementation actually checks for break/continue statements that are not nested within a RangeNode. Consider renaming to "hasBreakOrContinueOutsideRange" which more clearly expresses this behavior.
…ge break or continue
E.g.:
```handlebars
{{- $items := slice "a" "b" "c" }}
{{- range $items }}
{{- with partial "b" . -}}
{{break}}
{{- else }}
else: {{ . -}}
{{- end }}
{{- end }}
```
Fixes gohugoio#14333
bep
force-pushed
the
fix/withpartial-14333
branch
from
df31713 to
7c58a37
Compare
1 participant
E.g.:
Fixes #14333