Simplify find_variable

[shopmike]

This was introduced but never actually used. The introduction of environments.last as the fallback for unfound variables means this functionality is already somewhat broken.

See full discussion and history of this https://github.com/Shopify/liquid/pull/1132

[shopmike]

Why do this?

Because it simplifies scopes and environments access to be the same method. This allows for future improvements to optimise the time complexity of lookups across both scopes and environments. Also with the addition of a 3rd static_environments coming this is going to keep getting worse unless fixed.

[shopmike]

@Shopify/liquid @Shopify/guardians-of-the-liquid

@samdoiron this will greatly simplify your new changes as well

[ashmaroli]

Is this a change in behavior? The change to the unit test seems to suggest so: What formerly raised an exception now returns a result..?

[shopmike]

Yes this changes behaviour, it disables scopes and environments from running default_proc. There is a lot of details about what this was about here https://github.com/Shopify/liquid/pull/1132#discussion_r318447148 and what it may break.

[shopmike]

Basically it was designed around a single scope and over time environments have been added and other code and it has turned in to a block of code that is only designed to keep that test passing. Its possible to provide legacy support but I really doubt anyone is using this as using strict variables would do the same thing and you could catch that and handle in your custom logic.

[fw42]

No objections in general but maybe we should add some logging/instrumentation to this codepath first and confirm that it's not used in production?

[fw42]

Should we have a test with a variable which has a Proc as a value and the Proc returns nil? And then assert that it's using this nil now instead of treating it as a missing variable?

[tobi]

This branch indeed changes the behavior on this test:

  def test_hash_with_default_proc
    template = Template.parse(%(Hello {{ test }}))
    assigns = Hash.new { |h, k| "Unknown person '#{k}'" }
    assigns['test'] = 'Tobi'
    assert_equal 'Hello Tobi', template.render(assigns)
    assigns.delete('test')
    assert_equal "Hello Unknown person 'test'", template.render(assigns)
  end

(i rewrote it to remove the weird raise part that just made it hard to follow).

I think we should retain this feature unless the performance penalty is too big. It looks to me that most optimizations in this patch are independent of it?

[shopmike]

@tobi @pushrax @fw42 @ashmaroli I've modified this so default_proc support still exists, but this just simplifies find_variables

[shopmike]

I would still love to get rid of the || !s.default_proc.nil? support in future, but for now this just keeps the pull request to a single decision

[shopmike]

It also fixes the new static_environment to be simple before it also starts ignoring nil values

[shopmike]

I've actually got an idea how to do all this without causing issues for Shopify and other users of Liquid through some logging/instrumentation like @fw42 has suggested. It's pretty much opt-in based tooling that provides minimal overhead. Then a process of x days to report usage before a new method can be considered.

[shopmike]

This is currently on hold until I release usage tracking on default_proc usage. If nobody is using this then it makes sense to remove this so that scope, environment and static_envirornment all use the same method.

If they all use the same method this means variable_lookup can possibly look in a single hash that is constructed at the beginning of render that combines all environments and scopes