Prevent SelfDrop context mutation across render boundaries

[karreiro]

This PR fixes SelfDrop scoping so a self assigned in one template preserves its original scope when passed to a rendered template.

# template1.liquid
{% assign var = 42 %}
{% assign s = self %}
{% render 'template2', other_self: s %}

# template2.liquid
{% assign var = 43 %}
{{ other_self.var }}  => 43 (wrong — should be 42)

The fix renames @context to @self_context in SelfDrop, so the drop has a specific reference to its originating context instead of reusing the inherited context that gets overwritten at the context.rb level on line 220.

[aswamy]

Not sure if liquid-spec also allows render tags, but ill update it to capture your test. Thank you for this ❤️

0Shopify/liquid-spec#124

[ianks]

We likely want to undef context as well, which serves as a signal that the drop doesn’t support contextualization

[karreiro]

Thank you for the review, @ianks!

I did consider undef_method :context=, but that felt a bit unexpected, also because some places (e.g., template.rb:155) call Drop#context= without any guard. I went with def context=(_); end instead to keep the interface intact. Please let me know your thoughts!

[jg-rp]

Hi 👋

This looks like a subtle change in policy for the {% render %} tag. A deliberate mechanism for template authors to bring all of the parent template's scope into a partial template, albeit namespaced under an arbitrary variable.

I understand if you've decided this is a positive new feature. It's just that with this change partial templates are less isolated. They are potentially more tightly coupled to their parent templates, which I thought was one of the reasons for introducing {% render %} and depreciating {% include %}.

And, of course, having arbitrary aliases of self does make static analysis much harder.

[karreiro]

Thank you for the review, @jg-rp! That's a fair concern indeed.

I had the same instinct when I first saw this, but talking with @ianks clarified this in my mind:

• {% include %} did was leak the parent's entire scope implicitly, making static analysis painful and coupling invisible
• self works differently, as it's a readonly drop being explicitly passed, so there's no expensive implicit scope shared, just cheap lookups

The positive feature we're introducing with self is being able to rewrite expressions like {{ ['a'] }} to {{ self['a'] }}.

The isolation boundary is still there. It just has a handle to a drop that was deliberately handed to it.

I hope this clarifies the concern and motivation. Thanks again for the review!

[karreiro]

Thank you for the review, @ianks!

I think you meant undef :context= -- Drop only defines attr_writer :context (the setter), there's no context reader method, so undef context would raise a NameError.

That said, I trust your instinct here. But I want to flag the complexity this introduces. Because context= is defined on Drop but undefined on SelfDrop, every call site for Liquid::Drop now needs a guard:

case arg
when Liquid::Drop
  drop.context = c if drop.respond_to?(:context=)
...

Most call sites on Liquid already had this, but template.rb:155 didn't -- that's fixed in this PR too. It's a subtle new contract that other Liquid runtimes will need to implement.

If the goal is signaling that a drop is not contextualizable, a method that communicates intent directly might be cleaner:

# drop.rb
def contextualizable?
  true
end

Then SelfDrop overrides it to return false. No respond_to? guards, explicit intent, and upstream implementations get a clear signal about what's contextualizable and what isn't.

Happy to keep the undef approach if there's a reason I'm not seeing, just wanted to surface the trade-off.