fix: unify divergent expression evaluation paths (#176)#179
Conversation
Extract buildExecData() helper to consolidate three independent
map-building functions (evaluateActionWithVars, transformAndEvalWithVars,
transformConditionWithVars) that diverged over time, causing:
- .Type resolving as nil in mixed var+dot expressions like {{$c.Method .Type}}
- Raw "ERROR: ..." strings leaking into rendered HTML
- Partial match bugs ($col corrupted by $c replacement)
- Non-UTF8-safe capitalization corrupting multi-byte variable names
Changes:
- Move capitalizeFieldName() and mergeFieldsIntoMap() to var_context.go
as shared infrastructure
- Make capitalizeFieldName() UTF8-safe using utf8.DecodeRuneInString
- Create buildExecData() combining best of all three paths:
descending-length sort, UTF8 capitalization, root var detection,
dot context merging
- Fix evaluateActionWithVars() to return (string, error) instead of
embedding errors as rendered content
- Add cross-path parameterized tests (var_eval_test.go)
- Add standard library parity tests (stdlib_parity_test.go)
https://claude.ai/code/session_011iXr6f9X1MT3p2N6QpWGdu
Pre-existing bug found during #176 work: $ (root variable) inside {{with}} blocks resolves against the narrowed context instead of root. https://claude.ai/code/session_011iXr6f9X1MT3p2N6QpWGdu
Code ReviewGood consolidation work overall. The three divergent expression evaluation paths were a real maintenance hazard, and the bugs they caused — partial matches, ERROR strings in HTML, broken Issues1. This belongs as a GitHub issue, not a text file in the repository. The commented-out test in 2. Subtle behavioral change in the Old guard: Since 3. The function first iterates 4. The function is a single-line pass-through to Correctness Concerns5.
6. The The PR adds Positive Observations
|
There was a problem hiding this comment.
Pull request overview
This PR consolidates three previously diverged variable/dot/root expression-evaluation paths into a shared buildExecData() helper to fix correctness issues (mixed var+dot expressions, UTF-8 capitalization, partial matches) and to stop embedding "ERROR: ..." strings into rendered output.
Changes:
- Adds
buildExecData()plus shared helpers (capitalizeFieldName,mergeFieldsIntoMap) to unify$var/$.transformations and dot-context merging. - Updates action evaluation to return
(string, error)and propagate errors instead of rendering error strings. - Adds focused unit tests (
var_eval_test.go) and stdlib parity tests (stdlib_parity_test.go) to prevent regressions.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
internal/parse/var_context.go |
Introduces buildExecData() and shared helpers; updates transformAndEvalWithVars() to use unified logic. |
internal/parse/field.go |
Changes evaluateActionWithVars() to return errors and uses buildExecData(). |
internal/parse/conditional.go |
Removes bespoke condition var/root transformation and delegates to buildExecData(). |
internal/parse/field_test.go |
Adapts tests to new (string, error) signature for action evaluation. |
internal/parse/var_eval_test.go |
Adds tests for UTF-8 capitalization, partial match prevention, root handling, dot merging, and error propagation. |
internal/parse/stdlib_parity_test.go |
Adds parity tests comparing livetemplate output to html/template for core constructs. |
issue-with-root-var-bug.txt |
Adds a write-up for a known $-inside-with root resolution divergence (not fixed here). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -63,7 +62,7 @@ func handleIfNodeWithVars(node *parse.IfNode, varCtx *varContext, keyGen KeyGene | |||
| } | |||
|
|
|||
| // Condition uses variables or root - transform it | |||
| transformedCond, execData, err := transformConditionWithVars(pipeStr, varCtx, usesRoot) | |||
| transformedCond, execData, err := transformConditionWithVars(pipeStr, varCtx) | |||
There was a problem hiding this comment.
The decision to take the vars/root path is driven by usesRoot := detectsRootVariable(pipeStr, ...). detectsRootVariable() currently reports true for '$' inside quoted strings (per existing tests), which can force the execData/map evaluation path for conditions that don’t actually use root, potentially breaking dot method access. Consider tightening the root detection used for this branching decision (e.g., only treat "$." / standalone "$" as root usage, or parse the pipe and look for VariableNodes) so string literals containing '$' don’t change evaluation mode.
| if detectsRootVariable(expr, varCtx.vars) { | ||
| transformedExpr = strings.ReplaceAll(transformedExpr, "$.", ".RootData.") | ||
| execData["RootData"] = varCtx.parent | ||
| } |
There was a problem hiding this comment.
buildExecData() enables root-variable handling via detectsRootVariable(), but it only rewrites "$." to ".RootData.". This leaves standalone "$" usages (e.g. "{{$}}", "$ | func", etc.) untransformed, so the wrapper template will resolve "$" against execData rather than varCtx.parent. Consider rewriting standalone root "$" tokens to ".RootData" as well (while avoiding "$var" named variables).
| for _, varName := range sortedNames { | ||
| if strings.Contains(expr, "$"+varName) { | ||
| fieldName := capitalizeFieldName(varName) | ||
| transformedExpr = strings.ReplaceAll(transformedExpr, "$"+varName, "."+fieldName) | ||
| varValue, _ := varCtx.vars.Get(varName) | ||
| execData[fieldName] = varValue | ||
| } |
There was a problem hiding this comment.
Variable usage detection in buildExecData() uses strings.Contains(expr, "$"+varName). This can produce false positives when one var name is a prefix of another (e.g. varName="c" will match "$col"), causing unused variables to be added to execData and potentially changing semantics for expressions that use "." (e.g. len/range over dot). Consider matching variable references with token boundaries (or parsing the template/pipe AST and collecting VariableNodes) instead of substring matching.
| usesRoot := detectsRootVariable(expr, varCtx.vars) | ||
|
|
||
| if !usesVar { | ||
| if !usesVar && !usesRoot { | ||
| return evaluatePipeWithCache(ctx.TemplateName, expr, varCtx.dot, ctx) | ||
| } |
There was a problem hiding this comment.
transformAndEvalWithVars() now treats any detectsRootVariable()==true as a reason to switch to the synthetic execData map path. detectsRootVariable() currently returns true for '$' inside string literals (see existing tests), so expressions like {{eq .Val "$"}} may now be evaluated against a map (merged fields) instead of the original dot value, which can break method access and other dot semantics. Consider tightening the root-var detection used for this branch decision (e.g., only trigger on "$.", or parse the expression and look for VariableNodes) so '$' in quoted strings doesn't force the execData path.
| bug: $ (root variable) inside {{with}} resolves against narrowed context instead of root data | ||
|
|
||
| --- | ||
|
|
||
| ## Description | ||
|
|
||
| When using `$` or `$.Field` inside a `{{with}}` block, the root variable resolves against the narrowed `with` context instead of the original root data. This diverges from Go's standard `text/template` behavior where `$` always refers to the root data passed to `Execute()`. | ||
|
|
||
| ## Reproduction | ||
|
|
||
| ```go | ||
| tmpl := `{{with .Item}}{{.Name}} from {{$.Title}}{{end}}` | ||
| data := map[string]interface{}{ | ||
| "Title": "root", | ||
| "Item": map[string]interface{}{"Name": "child"}, | ||
| } | ||
| ``` | ||
|
|
||
| **Expected (stdlib `html/template`):** `child from root` | ||
| **Actual (livetemplate):** `child from ` (empty — `$.Title` resolves against the narrowed `.Item` context which has no `Title` field) | ||
|
|
||
| ## Root Cause | ||
|
|
||
| In `internal/parse/with.go`, `handleWithNode()` (the non-var path) passes the narrowed context as `data` to `buildTreeFromAST`: | ||
|
|
||
| ```go | ||
| // Line 34 — newContext is the {{with}} value, not the original root data | ||
| return buildTreeFromAST(node.List, newContext, keyGen, ctx) | ||
| ``` | ||
|
|
||
| Go's template engine tracks the root data (`$`) separately from the current dot (`.`). When `$` is used inside a `with` block, it should resolve against the original root, but `handleWithNode` loses the root reference by passing only `newContext`. | ||
|
|
||
| The var-aware path (`handleWithNodeWithVars`) handles this correctly by maintaining `varCtx.parent` as the root data. The non-var path has no equivalent mechanism. | ||
|
|
||
| ## Affected Code | ||
|
|
||
| - `internal/parse/with.go:34` — `handleWithNode()` loses root data reference | ||
| - The var-aware path (`handleWithNodeWithVars`, line 63) correctly sets `parent: varCtx.parent` | ||
|
|
||
| ## Suggested Fix | ||
|
|
||
| Pass the original `data` alongside `newContext` so that `$` can resolve against the root. Options: | ||
|
|
||
| 1. **Always use the var-aware path** for `with` blocks (convert to `varContext` even when no explicit variables exist) | ||
| 2. **Thread root data** through the non-var `buildTreeFromAST` call chain so `$` resolves correctly | ||
| 3. **Wrap the with body** in a `varContext` with `parent = data` and `dot = newContext` (minimal change) | ||
|
|
||
| ## Discovered During | ||
|
|
||
| Found by the standard library parity tests added in the PR for #176. The test is currently commented out in `internal/parse/stdlib_parity_test.go` with a note referencing this as a pre-existing divergence. | ||
|
|
||
| ## Impact | ||
|
|
||
| Any template using `$.Field` inside a `{{with}}` block without other variable declarations will silently produce empty/wrong values for the root field access. This is a correctness bug — the output diverges from what Go's standard template engine produces. |
There was a problem hiding this comment.
This looks like an ad-hoc issue report added at the repository root. To keep the repo organized and ensure it’s discoverable/maintained, consider moving this write-up into the existing docs/ structure (or tracking it as a GitHub issue and linking from docs/tests) rather than keeping a standalone root-level txt file.
| bug: $ (root variable) inside {{with}} resolves against narrowed context instead of root data | |
| --- | |
| ## Description | |
| When using `$` or `$.Field` inside a `{{with}}` block, the root variable resolves against the narrowed `with` context instead of the original root data. This diverges from Go's standard `text/template` behavior where `$` always refers to the root data passed to `Execute()`. | |
| ## Reproduction | |
| ```go | |
| tmpl := `{{with .Item}}{{.Name}} from {{$.Title}}{{end}}` | |
| data := map[string]interface{}{ | |
| "Title": "root", | |
| "Item": map[string]interface{}{"Name": "child"}, | |
| } | |
| ``` | |
| **Expected (stdlib `html/template`):** `child from root` | |
| **Actual (livetemplate):** `child from ` (empty — `$.Title` resolves against the narrowed `.Item` context which has no `Title` field) | |
| ## Root Cause | |
| In `internal/parse/with.go`, `handleWithNode()` (the non-var path) passes the narrowed context as `data` to `buildTreeFromAST`: | |
| ```go | |
| // Line 34 — newContext is the {{with}} value, not the original root data | |
| return buildTreeFromAST(node.List, newContext, keyGen, ctx) | |
| ``` | |
| Go's template engine tracks the root data (`$`) separately from the current dot (`.`). When `$` is used inside a `with` block, it should resolve against the original root, but `handleWithNode` loses the root reference by passing only `newContext`. | |
| The var-aware path (`handleWithNodeWithVars`) handles this correctly by maintaining `varCtx.parent` as the root data. The non-var path has no equivalent mechanism. | |
| ## Affected Code | |
| - `internal/parse/with.go:34` — `handleWithNode()` loses root data reference | |
| - The var-aware path (`handleWithNodeWithVars`, line 63) correctly sets `parent: varCtx.parent` | |
| ## Suggested Fix | |
| Pass the original `data` alongside `newContext` so that `$` can resolve against the root. Options: | |
| 1. **Always use the var-aware path** for `with` blocks (convert to `varContext` even when no explicit variables exist) | |
| 2. **Thread root data** through the non-var `buildTreeFromAST` call chain so `$` resolves correctly | |
| 3. **Wrap the with body** in a `varContext` with `parent = data` and `dot = newContext` (minimal change) | |
| ## Discovered During | |
| Found by the standard library parity tests added in the PR for #176. The test is currently commented out in `internal/parse/stdlib_parity_test.go` with a note referencing this as a pre-existing divergence. | |
| ## Impact | |
| Any template using `$.Field` inside a `{{with}}` block without other variable declarations will silently produce empty/wrong values for the root field access. This is a correctness bug — the output diverges from what Go's standard template engine produces. | |
| Root variable ($) resolution inside {{with}} blocks | |
| This ad-hoc issue report has been moved into the project documentation to keep the repository root clean and make the bug easier to discover and maintain over time. | |
| Please see the canonical write-up in the docs/ hierarchy (for example, a file such as: | |
| docs/known-issues/root-var-with-block.md | |
| or the corresponding tracked GitHub issue referenced from that document. |
handleWithNode() was passing the narrowed with-context as data to buildTreeFromAST, causing $ to resolve against the narrowed context instead of the original root data. This diverged from Go's stdlib behavior where $ always refers to the root data passed to Execute(). Fix: use the var-aware path (buildTreeFromASTWithVars) with a varContext that preserves the original data as parent and uses the narrowed context as dot. This matches what handleWithNodeWithVars already does correctly. https://claude.ai/code/session_011iXr6f9X1MT3p2N6QpWGdu
✅ Performance Benchmark ResultsStatus: No significant regressions detected Benchmark ComparisonThresholds
See benchmarking guide for details. |
Code ReviewThis is a well-motivated cleanup. The three divergent evaluation paths were a genuine correctness hazard, and the consolidated Issues1. The fix correctly makes bodyVarCtx := &varContext{parent: data, vars: newOrderedVars(), dot: newContext}
return buildTreeFromASTWithVars(node.List, bodyVarCtx, keyGen, ctx)But this allocates a 2. Old: After 3. The function iterates 4. The sole caller in Correctness Observations5. The test shows that vars stored with a 6. The helper reconstructs HTML by interleaving statics and dynamics, mirroring Positive Observations
|
The previous commit routes all {{with}} blocks through
buildTreeFromASTWithVars, but that path was missing:
1. CommentNode and TemplateNode handling (would error on templates
with comments inside with blocks)
2. Range comprehension embedding in buildTreeFromListWithVars
(range trees need to be nested, not merged by statics)
3. Variable declaration delegation (with blocks containing
{{$var := .}} inside their body)
Add the missing node type handlers and align
buildTreeFromListWithVars with buildTreeFromList behavior.
https://claude.ai/code/session_011iXr6f9X1MT3p2N6QpWGdu
|
test |
1 similar comment
|
test |
Code ReviewThis is a solid refactor that addresses several real bugs. Extracting What is Well Done
Issues1. Redundant computation in
2.
3. The guard 4. Missing test coverage for changed code paths
5.
SummaryThe refactor achieves its goal: three divergent paths are consolidated, the bugs are fixed, and |
The executeRangeBodyWithVars function stored range variables WITH the $
prefix (e.g., "$index"), but buildExecData searches for "$" + varName,
creating patterns like "$$index" that never match {{$index}} in
templates. This caused pipe expressions like {{$index | printf "#%d"}}
to fail with "$index" treated as a string literal.
Strip the $ prefix (matching executeRangeBodyWithVarsAndInherited) so
that buildExecData correctly constructs "$" + "index" = "$index".
Fixes test: TestE2EInvariantGuarantee
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Code ReviewThis is a well-motivated refactor that fixes several real bugs. The core idea — consolidating three divergent What's goodBug fixes are correct:
Test quality:
Issues to address1. Subtle semantic shift in // field.go
if len(execData) == 0 {
return "", nil
}The old guard was In practice this path shouldn't be reached (the caller confirms variable references exist), but the guard's intent has changed. A comment explaining why 2. This test (line ~1118 in varName := strings.TrimPrefix(node.Pipe.Decl[0].Ident[0], "$")So the 3. Unconditional
// Merge dot fields so mixed-context expressions (e.g. {{$c.Method .Type}}) work.
// This involves reflection; kept minimal by the caller's hasVars guard.
if err := mergeFieldsIntoMap(varCtx.dot, execData); err != nil {4. Double-scan in The function now scans 5. Missing test: The change to // Missing: confirms $.Field inside {{with}} resolves to original data
data := map[string]interface{}{
"Title": "root",
"Item": map[string]interface{}{"Name": "child"},
}
// {{with .Item}}{{$.Title}}{{end}} should produce "root"The Nits
SummaryThe refactor is correct and the bugs being fixed are real. The main thing to verify before merging is item #1 (the |
The core library (livetemplate/livetemplate#179) fixed a bug where range variables in executeRangeBodyWithVars were stored with the $ prefix, causing buildExecData to construct "$$index" patterns that never matched {{$index}} in templates. The fix strips the $ prefix so that pipe expressions like {{$index | printf "#%d"}} now correctly resolve to "#0", "#1", etc. instead of empty strings. Updates all affected golden files: - update_01: Initial range items now have "#0", "#1", "#2" for field 2 - update_02: Adds ["u", "todo-3", {"2": "#1"}] after remove (index shift) - update_03: Adds index updates for completed/reordered items - update_05a: Adds index updates for existing items when prepending - update_05b: Adds index updates for existing items when inserting middle - update_06: Adds index updates for multi-operation scenario Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add comment explaining len(execData) == 0 guard semantics (Claude #1) - Rename TestBuildExecData_RangeVarPrefixConvention to TestBuildExecData_DollarPrefixedVarNotMatched and reframe as defensive test since $ prefix is now stripped (Claude #2) - Add comment on mergeFieldsIntoMap explaining deliberate reflection cost for mixed-context expressions (Claude #3) - Inline transformConditionWithVars one-liner wrapper into call site and remove the dead function (Claude nit) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Code ReviewThis is a well-motivated refactoring that correctly addresses real bugs. The consolidation into What is goodBug fixes are genuine and correctly solved:
The Test coverage is solid. The parameterized unit tests in Issues1. Double call to In 2.
3. Missing stdlib parity test for the primary bug scenario The PR description cites mixed 4. The helper in 5.
Nits
SummaryThe core refactoring is correct and the bugs fixed are real. Main asks before merging:
The double |
…lsy cases
Addresses Claude bot review:
- Add end-to-end parity test for mixed $var + .DotField in range bodies
(the primary regression scenario from this PR)
- Add {{with}} falsy parity tests (nil, else, empty string)
- Document treeToHTML range limitation
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
PR Review: fix: unify divergent expression evaluation paths. This is a solid refactor that fixes four real bugs through a well-motivated consolidation. Please see the full review details attached. |
|
Full review text for the fix: unify divergent expression evaluation paths PR follows. This placeholder will be replaced. |
Contributing to LiveTemplate Core LibraryThank you for your interest in contributing to the LiveTemplate core library! This guide covers contributions to the Go server-side library only. Contributing to Other ComponentsLiveTemplate is distributed across multiple repositories. Please use the appropriate contribution guide:
This guide will help you get started with core library contributions. Table of Contents
PrerequisitesBefore you begin, ensure you have the following installed:
Note: For client library development (TypeScript), see the client repository. Setup
Development WorkflowMaking Changes
Testing Core Changes with LVT/ExamplesWhen making changes to the core library, you may want to test how they affect LVT or examples before releasing. Recommended: Go Workspace (Automatic)The easiest way is to use Go workspaces (Go 1.18+). This automatically uses local checkouts without modifying any Directory structure: One-time setup: # Clone sibling repositories
cd .. # Go to parent directory
git clone https://github.com/livetemplate/lvt.git
git clone https://github.com/livetemplate/examples.git
# Create workspace (run once)
./setup-workspace.shThat's it! Now all # Test LVT with your core changes
cd lvt
go test ./... # Automatically uses ../livetemplate
# Test examples
cd ../examples
./test-all.sh # Automatically uses ../livetemplate and ../lvt
# Build an example
cd counter
go build # Uses local livetemplateTo remove workspace: cd /path/to/parent
./setup-workspace.sh --cleanHow it works:
Alternative: Manual Replace DirectivesIf you prefer manual control or can't use workspaces, use the helper scripts in each repo: cd ../lvt
./scripts/setup-local-dev.sh
cd ../examples
./scripts/setup-local-dev.shRevert with Automated CI ChecksThe core library has automated CI checks that test LVT and examples against your PR. These checks will catch breaking changes before merge. Directory Structure (Core Library)Note: The client library, CLI tool, and examples are now in separate repositories:
Pre-commit HookThe pre-commit hook is CRITICAL for maintaining code quality. It automatically:
Important Rules
Example Hook OutputTestingTest Categories
Note: For client library tests, see the client repository. Golden FilesMany E2E tests use golden files in
To update golden files after intentional changes: UPDATE_GOLDEN=1 go test -run TestTemplate_E2E -vWriting TestsFollow these patterns: Unit test example: func TestNewFeature(t *testing.T) {
t.Run("description of test case", func(t *testing.T) {
// Arrange
input := "test input"
// Act
result := YourFunction(input)
// Assert
if result != expected {
t.Errorf("expected %v, got %v", expected, result)
}
})
}E2E browser test example: func TestFeature(t *testing.T) {
// Setup server and browser context
ctx, cancel := chromedp.NewContext(context.Background())
defer cancel()
// Run test actions
err := chromedp.Run(ctx,
chromedp.Navigate("http://localhost:8080"),
chromedp.Click("button#submit"),
chromedp.WaitVisible("#result"),
)
if err != nil {
t.Fatal(err)
}
}Code StyleGeneral Principles
Naming Conventions
Public API GuidelinesThe public API surface is minimal by design. Only export:
Keep implementation details private. Documentation
// Template represents a parsed template that can generate updates.
// It maintains state between renders to produce minimal diffs.
type Template struct {
// ...
}
// ExecuteToUpdate renders the template and returns a JSON update.
// This is more efficient than ExecuteToHTML for subsequent renders.
func (t *Template) ExecuteToUpdate(data interface{}) (*UpdateResponse, error) {
// ...
}Commit MessagesUse conventional commit format: Types
ExamplesPull RequestsBefore Submitting
PR Description Template## Description
Brief description of changes
## Motivation
Why is this change needed?
## Changes
- List of specific changes
- One per line
## Testing
How was this tested?
## Checklist
- [ ] Tests added/updated
- [ ] Documentation updated
- [ ] Pre-commit hook passes
- [ ] No breaking changes (or documented if necessary)Review Process
Where to StartGood First IssuesLook for issues labeled
Areas to Explore
For other components:
Learning the Codebase
Getting Help
LicenseBy contributing, you agree that your contributions will be licensed under the same license as the project (check LICENSE file). Thank you for contributing to LiveTemplate! |
|
PR Review: fix: unify divergent expression evaluation paths This is a solid refactor that fixes four real bugs through a well-motivated consolidation. Overall the changes are correct and the test coverage is good. What works well: buildExecData() consolidation is the right call. Three functions (evaluateActionWithVars, transformAndEvalWithVars, transformConditionWithVars) had drifted apart and the extraction cleanly captures the invariants once. The descending-length sort to prevent partial matches is the correct approach and directly fixes a real edge case. Error propagation fix in evaluateActionWithVars from returning string to (string, error) is exactly right. Error strings leaking into rendered HTML is a correctness bug masquerading as output, and the new signature makes failures first-class. UTF8-safe capitalizeFieldName using utf8.DecodeRuneInString and unicode.ToUpper is correct. The old byte-slice approach varName[:1] is valid for ASCII but silently wrong for multi-byte runes. Test files (var_eval_test.go and stdlib_parity_test.go) are well-structured. Issues to address: Issue 1: detectsRootVariable called twice in conditional path In handleIfNodeWithVars (conditional.go:57), detectsRootVariable is called to decide the fast path, then buildExecData calls it again internally (var_context.go:189). Both produce the same result. Minor redundancy, worth noting. Issue 2: buildExecData always reflects over varCtx.dot The mergeFieldsIntoMap call runs unconditionally on every buildExecData invocation, including inside range body hot paths. The function comment says kept minimal by the callers hasVars guard but that guard only exists in transformAndEvalWithVars. The other two callers (evaluateActionWithVars and handleIfNodeWithVars) do not short-circuit when dot is nil/empty. A cheap guard would suffice: check if varCtx.dot is nil before calling mergeFieldsIntoMap. Issue 3: The empty-execData guard in evaluateActionWithVars has shifted semantics Old guard: usedVars.Len() == 0 and not usesRootVar. New guard: len(execData) == 0. Since buildExecData now merges dot fields unconditionally, any non-nil varCtx.dot populates execData even if no named variables matched. The guard no longer reliably means nothing to evaluate. Worth calling out explicitly. Issue 4: handleWithNode routing change needs a TemplateNode invariant check The change in with.go from buildTreeFromAST to buildTreeFromASTWithVars is semantically significant. The stdlib parity tests cover the happy path, but there is no test for a template invocation inside a with-block hitting the new TemplateNode error path. The new code errors with: template invocation found - should be flattened. If the flattening invariant ever breaks, this surfaces as a template execution failure. Worth a comment or test confirming flattening is always applied before handleWithNode is reached. Issue 5: treeToHTML naming in stdlib_parity_test.go The helper is flat-tree-only (no RangeData). Since TestStdlibParity_RangeVarWithDotField uses a completely different reconstruction path, having two very different approaches in the same file may confuse contributors. A rename to flatTreeToHTML would signal the limitation at every call site. Summary: The core fixes (UTF8, partial-match, error propagation, dot-context merging) are correct and test coverage is solid. Main action items:
Items 1 and 3 are polish; item 2 is the one most worth resolving before merging. |
- Add nil guard before mergeFieldsIntoMap to avoid reflection when dot is nil - Rename treeToHTML to flatTreeToHTML to signal range limitation at call sites - Add flattening precondition comment in with.go for TemplateNode invariant Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
PR Review: fix: unify divergent expression evaluation paths (#176)This is a solid refactoring that tackles a genuine architectural problem. The consolidation of three divergent evaluation paths into a single What's good
Error propagation — Changing
Test coverage — Issues / Concerns1.
execData["RootData"] = varCtx.parentIf the template data legitimately has a field or map key called 2. // field.go
if len(execData) == 0 {
return "", nil
}Because 3. In // conditional.go — caller checks usesRoot before calling buildExecData
transformedCond, execData, err := buildExecData(pipeStr, varCtx)But usesRoot := detectsRootVariable(expr, varCtx.vars)
if !usesVar && !usesRoot {
return evaluatePipeWithCache(...)
}
transformedExpr, execData, err := buildExecData(expr, varCtx)This means 4. The change from
5. // NOTE: This helper does not handle RangeData (tree.Range). Any parity test
// involving {{range}} will produce incomplete HTML from the LVT side.This is correctly documented, but it means Minor
SummaryThe core idea is correct and the implementation is solid. The three issues worth addressing before merge:
Everything else is minor. Good PR. |
- Document RootData as reserved synthetic key in buildExecData - Update len(execData)==0 guard comment to clarify defensive semantics - Add range guard in lvtRender to prevent silent empty output in future tests Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Code ReviewThe consolidation into Correctness Issues1. bodyVarCtx := &varContext{
parent: data,
vars: newOrderedVars(), // ← empty!
dot: newContext,
}
return buildTreeFromASTWithVars(node.List, bodyVarCtx, keyGen, ctx)The new 2. // NOTE: "RootData" is a reserved synthetic key. If template data has a field
// named "RootData", it will be shadowed by the root variable substitution.
execData["RootData"] = varCtx.parentThis comment appears in
3. Looking at // Expression unchanged since no $ references
if transformed != "{{.Field}}" { ... }
// Dot fields should still be merged
if execData["Field"] != "value" { ... }So Missing Regression Test4. No test for the primary bug case The PR description specifically cites // $c is a value with a method; .Type comes from dot context
{{$c.SomeField}} with .Typewould directly validate the regression case and prevent it from silently re-breaking. Minor Points5. A text file as a bug tracker entry is not a convention in this repo. The commented-out test already captures the known divergence clearly. Move the context to a GitHub issue and reference the issue number from the test comment. 6. The test helper re-implements static/dynamic interleaving. If the tree format changes, parity tests could stay green while the real render diverges. Add a comment linking it to What's Good
|
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Code Review — PR #176: Unify divergent expression evaluation pathsThis is a well-motivated refactor that addresses real correctness bugs. The consolidation into Potential bugs1. In for _, varName := range sortedNames {
// ... execData[fieldName] = varValue // could set execData["RootData"]
}
if detectsRootVariable(expr, varCtx.vars) {
execData["RootData"] = varCtx.parent // silently overwrites
}If a user declares 2. for _, varName := range sortedNames {
if strings.Contains(expr, "$"+varName) { // checks ORIGINAL
...
execData[fieldName] = varValue
}
}With vars
Design observations (non-blocking)3.
4. The nil guard ( 5. If only dot fields contributed to Positives
Nits
Overall this is a sound fix. Issues 1 and 2 are worth addressing before merge; the rest are suggestions. |
* fix: update golden files for corrected $index resolution in range The core library (livetemplate/livetemplate#179) fixed a bug where range variables in executeRangeBodyWithVars were stored with the $ prefix, causing buildExecData to construct "$$index" patterns that never matched {{$index}} in templates. The fix strips the $ prefix so that pipe expressions like {{$index | printf "#%d"}} now correctly resolve to "#0", "#1", etc. instead of empty strings. Updates all affected golden files: - update_01: Initial range items now have "#0", "#1", "#2" for field 2 - update_02: Adds ["u", "todo-3", {"2": "#1"}] after remove (index shift) - update_03: Adds index updates for completed/reordered items - update_05a: Adds index updates for existing items when prepending - update_05b: Adds index updates for existing items when inserting middle - update_06: Adds index updates for multi-operation scenario Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: update go.mod to use merged core library commit Points to squash-merged PR #179 on livetemplate/livetemplate main branch which fixes $index resolution in pipe expressions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Extract buildExecData() helper to consolidate three independent
map-building functions (evaluateActionWithVars, transformAndEvalWithVars,
transformConditionWithVars) that diverged over time, causing:
Changes:
as shared infrastructure
descending-length sort, UTF8 capitalization, root var detection,
dot context merging
embedding errors as rendered content
https://claude.ai/code/session_011iXr6f9X1MT3p2N6QpWGdu