Skip to content

Commit bb40962

Browse files
committed
fix(render): preserve scope chain when self: bound on render tag
The render tag wrote `self:` attributes directly into @scopes[0] under key 'self', which shadowed the SelfDrop. `self[var]` lookups inside the snippet then did strict key access on the bound object instead of walking the scope chain, resolving to nil for any key not in the object. SelfDrop now holds an optional `bound_self`; `[]` and `key?` consult it first via duck typing (matches lib/liquid/variable_lookup.rb), then fall through to the scope-chain walk on miss. Render#render_tag routes attribute key Expression::SELF to inner_context.self_drop.bound_self= instead of the generic scope write. Lookup order is bound-first; existing self: users' hits stay intact, previously-nil misses now resolve via fallthrough. PR #2060 introduced the SelfDrop and bare-bracket prohibition but had no coverage for the {% render 'snippet', self: obj %} interaction. Adds 8 tests including two-deep nested renders with leak detection and a composite chain combining renders, top-level self[var], and regular variables. Discovered while investigating SFR strict-parser migration parity diffs.
1 parent 1954a26 commit bb40962

4 files changed

Lines changed: 266 additions & 4 deletions

File tree

lib/liquid/context.rb

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -200,6 +200,10 @@ def evaluate(object)
200200
object.respond_to?(:evaluate) ? object.evaluate(self) : object
201201
end
202202

203+
def self_drop
204+
@self_drop ||= SelfDrop.new(self)
205+
end
206+
203207
# Fetches an object starting at the local scope and then moving up the hierachy
204208
def find_variable(key, raise_on_not_found: true)
205209
# This was changed from find() to find_index() because this is a very hot
@@ -208,7 +212,7 @@ def find_variable(key, raise_on_not_found: true)
208212

209213
# `self` resolves to a SelfDrop (enabling `self['var']` lookups),
210214
# but only when it hasn't been explicitly assigned as a local variable.
211-
return SelfDrop.new(self) if key == Expression::SELF && !index
215+
return self_drop if key == Expression::SELF && !index
212216

213217
variable = if index
214218
lookup_and_evaluate(@scopes[index], key, raise_on_not_found: raise_on_not_found)

lib/liquid/self_drop.rb

Lines changed: 21 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,23 +16,42 @@ module Liquid
1616
# then the local value takes precedence over the `self` object.
1717
# @liquid_access global
1818
class SelfDrop < Drop
19+
attr_accessor :bound_self
20+
1921
def initialize(context)
2022
super()
2123
@context = context
24+
@bound_self = nil
2225
end
2326

2427
def [](key)
25-
@context.find_variable(key)
28+
if @bound_self && bound_has?(key)
29+
bound_lookup(key)
30+
else
31+
@context.find_variable(key)
32+
end
2633
rescue UndefinedVariable
2734
nil
2835
end
2936

3037
def key?(key)
31-
@context.variable_defined?(key)
38+
(@bound_self && bound_has?(key)) || @context.variable_defined?(key)
3239
end
3340

3441
def to_liquid
3542
self
3643
end
44+
45+
private
46+
47+
def bound_has?(key)
48+
@bound_self.respond_to?(:key?) && @bound_self.key?(key)
49+
end
50+
51+
def bound_lookup(key)
52+
return unless @bound_self.respond_to?(:[])
53+
54+
@bound_self[key]
55+
end
3756
end
3857
end

lib/liquid/tags/render.rb

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,12 @@ def render_tag(context, output)
6666
inner_context['forloop'] = forloop if forloop
6767

6868
@attributes.each do |key, value|
69-
inner_context[key] = context.evaluate(value)
69+
evaluated = context.evaluate(value)
70+
if key == Expression::SELF
71+
inner_context.self_drop.bound_self = evaluated
72+
else
73+
inner_context[key] = evaluated
74+
end
7075
end
7176
inner_context[context_variable_name] = var unless var.nil?
7277
partial.render_to_output_buffer(inner_context, output)
Lines changed: 234 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,234 @@
1+
# frozen_string_literal: true
2+
3+
require 'test_helper'
4+
5+
# Tests for self[var] lookup behavior across {% render %} boundaries,
6+
# including the `self:` bound-parameter shape used by the rewriter.
7+
class SelfDropRenderTest < Minitest::Test
8+
include Liquid
9+
10+
# Snippet body using the rewriter's `self[name_var]` form. `item_1_title`
11+
# is a template-local assign; `name` is built at runtime; the rewriter
12+
# produces `self[name]` because bare brackets are forbidden in :strict2.
13+
REWRITTEN_SNIPPET = <<~LIQUID
14+
{%- liquid
15+
assign item_1_title = 'Cookware Set'
16+
-%}
17+
{%- for i in (1..1) -%}
18+
{%- liquid
19+
assign name = 'item_' | append: i | append: '_title'
20+
assign title = self[name]
21+
-%}
22+
[{{ title }}]
23+
{%- endfor -%}
24+
LIQUID
25+
26+
# Original (pre-rewrite) snippet body using bare-bracket lookup. Rejected
27+
# at parse time by :strict2 -- which is why the rewriter exists.
28+
ORIGINAL_SNIPPET = <<~LIQUID
29+
{%- liquid
30+
assign item_1_title = 'Cookware Set'
31+
-%}
32+
{%- for i in (1..1) -%}
33+
{%- liquid
34+
assign name = 'item_' | append: i | append: '_title'
35+
assign title = [name]
36+
-%}
37+
[{{ title }}]
38+
{%- endfor -%}
39+
LIQUID
40+
41+
EXPECTED_OUTPUT = '[Cookware Set]'
42+
43+
# Baseline: parent does NOT pass `self:` to the snippet. The SelfDrop is
44+
# returned by find_variable (no scope has the `self` key), and its `[]`
45+
# walks back through the scope chain to find the for-loop-local
46+
# `item_1_title`. This is the parity-safe case for the rewriter's
47+
# transform; passing today.
48+
def test_rewritten_self_lookup_without_self_named_param_resolves_local_assign
49+
assert_template_result(
50+
EXPECTED_OUTPUT,
51+
"{% render 'snippet' %}",
52+
partials: { 'snippet' => REWRITTEN_SNIPPET },
53+
error_mode: :strict2,
54+
)
55+
end
56+
57+
# PRODUCTION FAILURE SHAPE.
58+
#
59+
# Parent passes `self:` as a named render parameter. Render's
60+
# `inner_context[key] = context.evaluate(value)` (render.rb:68-70)
61+
# writes `my_obj` to `inner_context['self']`, which lands in
62+
# @scopes[0] (context.rb:172-174). Now find_variable's check at
63+
# context.rb:209-213 sees `self` defined in scope[0] and skips the
64+
# SelfDrop fallthrough -- `self[name]` becomes a literal key-access
65+
# against `my_obj`, which has no `item_1_title` key, returning nil.
66+
# Output is empty.
67+
#
68+
# This test asserts the INTENDED behavior (output should be the
69+
# snippet-local title). It FAILS today. It should pass once the
70+
# rewriter's transform is corrected to preserve scope-chain semantics
71+
# across `{% render 'snippet', self: ... %}` boundaries (or, less
72+
# likely, once SelfDrop's lookup precedence is changed in
73+
# find_variable).
74+
#
75+
# Failure message reads:
76+
# Expected: "[Cookware Set]"
77+
# Actual: "[]"
78+
# which directly says "the snippet's template-local item_1_title was
79+
# not found via self[name] when self: was bound on render".
80+
def test_rewritten_self_lookup_with_self_named_param_loses_local_assign
81+
assert_template_result(
82+
EXPECTED_OUTPUT,
83+
"{% render 'snippet', self: my_obj %}",
84+
{ 'my_obj' => { 'unrelated_key' => 'foo' } },
85+
partials: { 'snippet' => REWRITTEN_SNIPPET },
86+
error_mode: :strict2,
87+
)
88+
end
89+
90+
# Pins the prohibition that motivates the rewriter migration:
91+
# bare-bracket access must raise at parse time in :strict2. Documents
92+
# WHY the rewriter rewrites `[name]` to `self[name]` in the first
93+
# place. Passing today; serves as a guard against accidental
94+
# regression of PR #2060's strict2 enforcement.
95+
def test_original_bare_bracket_lookup_raises_in_strict2
96+
error = assert_raises(Liquid::SyntaxError) do
97+
Liquid::Template.parse(ORIGINAL_SNIPPET, error_mode: :strict2)
98+
end
99+
assert_match(
100+
/Bare bracket access is not allowed\. Use self\['\.\.\.'\] instead/,
101+
error.message,
102+
)
103+
end
104+
105+
# Coverage extension: the bug is not a one-off of the empty-string-built
106+
# variable name. Confirm `self[name]` still misses when `name` is sourced
107+
# directly from the forloop index (no string concatenation), so a future
108+
# rewriter fix cannot accidentally pass tests by special-casing
109+
# constructed strings.
110+
#
111+
# `forloop.index` is a number; we cast to string via `| append: ''` to
112+
# form `item_1_title` in a different way. Same expected failure: empty
113+
# output today, should be `[Cookware Set]` once fixed.
114+
def test_rewritten_self_lookup_with_forloop_constructed_key_loses_local_assign
115+
snippet = <<~LIQUID
116+
{%- liquid
117+
assign item_1_title = 'Cookware Set'
118+
-%}
119+
{%- for i in (1..1) -%}
120+
{%- assign suffix = forloop.index | append: '_title' -%}
121+
{%- assign name = 'item_' | append: suffix -%}
122+
{%- assign title = self[name] -%}
123+
[{{ title }}]
124+
{%- endfor -%}
125+
LIQUID
126+
127+
assert_template_result(
128+
EXPECTED_OUTPUT,
129+
"{% render 'snippet', self: my_obj %}",
130+
{ 'my_obj' => { 'unrelated_key' => 'foo' } },
131+
partials: { 'snippet' => snippet },
132+
error_mode: :strict2,
133+
)
134+
end
135+
136+
# If it fails: Inner snippet's SelfDrop saw outer bound self OR outer locals;
137+
# isolation broken.
138+
def test_nested_render_each_level_resolves_its_own_local_via_bound_self
139+
snippet_a = <<~LIQUID
140+
{%- assign label_a = 'A_local' -%}
141+
{%- assign key_a = 'label_a' -%}
142+
A=[{{ self[key_a] }}]{% render 'b', self: obj_b %}
143+
LIQUID
144+
snippet_b = <<~LIQUID
145+
{%- assign label_b = 'B_local' -%}
146+
{%- assign key_b = 'label_b' -%}
147+
B=[{{ self[key_b] }}]
148+
LIQUID
149+
parent = "{% render 'a', self: obj_a %}"
150+
assigns = {
151+
'obj_a' => { 'unrelated_a' => 'xa' },
152+
'obj_b' => { 'unrelated_b' => 'xb' },
153+
}
154+
assert_template_result(
155+
"A=[A_local]B=[B_local]\n\n",
156+
parent,
157+
assigns,
158+
partials: { 'a' => snippet_a, 'b' => snippet_b },
159+
error_mode: :strict2,
160+
)
161+
end
162+
163+
# If it fails: Bound self leaked across `new_isolated_subcontext` boundary;
164+
# SelfDrop carries state across subcontexts.
165+
def test_nested_render_inner_without_self_walks_only_inner_scope
166+
snippet_a = <<~LIQUID
167+
{%- assign label_a = 'A_local' -%}
168+
A=[{{ self['label_a'] }}]{% render 'b' %}
169+
LIQUID
170+
snippet_b = <<~LIQUID
171+
{%- assign label_b = 'B_local' -%}
172+
{%- assign key_b = 'label_b' -%}
173+
B=[{{ self[key_b] }}]
174+
LIQUID
175+
parent = "{% render 'a', self: obj_a %}"
176+
assigns = { 'obj_a' => { 'label_b' => 'LEAK_FROM_OBJ_A' } }
177+
assert_template_result(
178+
"A=[A_local]B=[B_local]\n\n",
179+
parent,
180+
assigns,
181+
partials: { 'a' => snippet_a, 'b' => snippet_b },
182+
error_mode: :strict2,
183+
)
184+
end
185+
186+
# If it fails: Specific segment in concatenated output names the broken layer
187+
# (top-level, snippet_a local, snippet_a bound, snippet_b local, snippet_b
188+
# bound).
189+
def test_full_chain_top_level_plus_nested_renders_with_mixed_self_binding
190+
snippet_a = <<~LIQUID
191+
{%- assign a_local = 'A!' -%}
192+
{%- assign a_key = 'a_local' -%}
193+
[a:{{ self[a_key] }}|reg:{{ regular_var }}|bound:{{ self['shared'] }}]{% render 'b', self: obj_b %}
194+
LIQUID
195+
snippet_b = <<~LIQUID
196+
{%- assign b_local = 'B!' -%}
197+
{%- assign b_key = 'b_local' -%}
198+
[b:{{ self[b_key] }}|bound:{{ self['only_in_b'] }}]
199+
LIQUID
200+
template = <<~LIQUID
201+
{%- assign top_key = 'top_var' -%}
202+
top:{{ self[top_key] }}|lit:LITERAL|{% render 'a', self: obj_a, regular_var: 'REG' %}
203+
LIQUID
204+
assigns = {
205+
'top_var' => 'TOP!',
206+
'obj_a' => { 'shared' => 'SHARED_A' },
207+
'obj_b' => { 'only_in_b' => 'B_BOUND', 'shared' => 'SHARED_B_NOT_USED' },
208+
}
209+
expected = "top:TOP!|lit:LITERAL|[a:A!|reg:REG|bound:SHARED_A][b:B!|bound:B_BOUND]\n\n\n"
210+
assert_template_result(
211+
expected,
212+
template,
213+
assigns,
214+
partials: { 'a' => snippet_a, 'b' => snippet_b },
215+
error_mode: :strict2,
216+
)
217+
end
218+
219+
# If it fails: Lookup precedence flipped from bound-first to scope-first;
220+
# section C invariant lost.
221+
def test_bound_self_key_hit_returns_bound_value_not_scope_value
222+
snippet = <<~LIQUID
223+
{%- assign shared = 'SCOPE_VALUE' -%}
224+
[{{ self['shared'] }}]
225+
LIQUID
226+
assert_template_result(
227+
"[BOUND_VALUE]\n",
228+
"{% render 'snippet', self: my_obj %}",
229+
{ 'my_obj' => { 'shared' => 'BOUND_VALUE' } },
230+
partials: { 'snippet' => snippet },
231+
error_mode: :strict2,
232+
)
233+
end
234+
end

0 commit comments

Comments
 (0)