fix(render): preserve scope chain when 'self:' bound on render tag#2080
Draft
EvilGenius13 wants to merge 1 commit intomainfrom
Draft
fix(render): preserve scope chain when 'self:' bound on render tag#2080EvilGenius13 wants to merge 1 commit intomainfrom
EvilGenius13 wants to merge 1 commit intomainfrom
Conversation
30eb8af to
d6a5120
Compare
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.
d6a5120 to
bb40962
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
{% render 'snippet', self: obj %}shadowed theSelfDropbecause the render tag wroteself:directly into@scopes[0]under key'self'. Subsequentself[var]lookups inside the snippet did strict key access onobjinstead of walking the scope chain, resolving to nil for any key not inobj.SelfDropnow holds an optionalbound_self;[]andkey?consult it first and fall through to the existing scope-chain walk on miss.Render#render_tagspecial-cases attribute keyExpression::SELFand routes the value toinner_context.self_drop.bound_self =instead of the generic scope write.self:users — when the key exists in the bound object, the bound value still wins.Why
PR #2060 introduced the
SelfDropand theself[var]syntax as the strict-mode replacement for bare-bracket lookups. Its tests exerciseself[var]only at the top level viatemplate.render(hash). There was no coverage for the{% render 'snippet', self: obj %}interaction — and the render tag's existing generic attribute mechanism (which writes any attribute key into@scopes[0]) silently shadows the new SelfDrop when the attribute key happens to be'self'.This PR adds that coverage and fixes the shadowing.
Implementation
lib/liquid/self_drop.rb— addedbound_selfaccessor;[]andkey?consult bound first via duck typing (matches the pattern inlib/liquid/variable_lookup.rb:59-61), fall through to@context.find_variable(key)on miss.lib/liquid/context.rb— added a memoizedself_dropreader so theSelfDropreturned byfind_variableis the same instance the render tag mutates.lib/liquid/tags/render.rb— attribute loop special-casesExpression::SELF(already used atcontext.rb:215) and routes toinner_context.self_drop.bound_self =.Lookup order is bound-first, scope-chain-on-miss. Justified because it preserves all currently-passing
self:users' hits and only changes previously-nil misses to "now resolves via scope-chain fallthrough."Each
new_isolated_subcontextcreates its ownContext, its own lazily-createdSelfDrop, and its ownbound_self— so nested renders, each passing their ownself:, stay isolated.Test plan
test/integration/self_drop_render_test.rb(8 tests, all passing):self[name]resolves correctly when noself:is bound (scope-chain walk works as before)self[name]withself:bound resolves the snippet's local assigns when the key is missing from the bound object:strict2self:bound at both levels — each snippet sees only its ownbound_selfself:does NOT leak inself[var]+ regular{{ var }}interpolationsNotes