Child Scope Does Not Modify Outer Scope Variables

When entering a child scope, variables defined in the parent scope can be read, but changes to their values do not propagate when the child scope is released. I would expect that changes made to a variable would persist to the outermost scope in which the variable is defined, is that correct? We are migrating to Fluid from DotLiquid and the behavior is different.

Here is an example (failing) unit test to demonstrate the issue, tested against Fluid v2.3.1:

        [Fact]
        public void ChildScope_Can_Modify_Outer_Scope_Variables()
        {
            var parser = new CustomParser();

            parser.RegisterExpressionBlock("repeat", async (value, statements, writer, encoder, context) =>
            {
                context.EnterChildScope();
                var repeatCount = (int)value.EvaluateAsync(context).Result.ToNumberValue();
                for (var i = 0; i < repeatCount; i++)
                {
                    await statements.RenderStatementsAsync(writer, encoder, context);
                }
                context.ReleaseScope();

                return Completion.Normal;
            });

            var template = parser.Parse("{% assign outer = 10 %}Outer={{ outer }}||{% repeat 3 %}{% assign outer = outer | plus: 1 %}Outer={{ outer }},{%- endrepeat %}||Outer={{ outer }}");
            var result = template.Render();

            Assert.Equal("Outer=10||Outer=11,Outer=12,Outer=13,||Outer=13", result);
        }

Expected Output:
Outer=10||Outer=11,Outer=12,Outer=13,||Outer=13
Actual Output:
Outer=10||Outer=11,Outer=12,Outer=13,||Outer=10

@sebastienros - is there any further information I can provide to move this issue along or start a conversation about how scoping does/should work? Am I wrong in my assumption about the way scoping should work in my example?

@sebastienros Just wanted to check-in to get your perspective on whether you feel this is an issue or not. Someone in our community is reporting it to us. Thanks for all you do!

Is that true for things like {% for %} loops, as in for any existing block? Or is it only when you create custom blocks?

If this is just for custom ones then we could add an option to apply updates to outer scopes in the API. Will do some tests. I think there is already one like this, for loops, a custom context.

I confirm that a for loop grants access to the parent scope. And Fluid handles that correctly.
There is a public method that you can use in your block to provide the same behavior, by replacing EnterChildScope() by EnterForLoopScope()

NB: I'll be in Tempe in November for the ironman, maybe we'll bump into each other.

@sebastienros thanks for the heads up and code reference! I'll have @MrUpsideDown on our team confirm this and follow-up. Ping me when you're in town jon(at)sparkdevnetwork.org, would love to meet up!

I found some videos your community made about the migration from dotliquid -> fluid in Rocks. Some of the points that were made could be filed as bugs, you should not hesitate. I just published a PR fixing one of them (empty raw tags). Some others were explained as "different behavior" instead of explaining that this is to match the reference behavior of Shopify's liquid, e.g. binary operator precedence, truncate words length, ... Following the specs will help interoperability and documentation. I use their site to verify what's a bug or not, even tried with this current issue to confirm it.

Thanks for looking into this issue @sebastienros. I don't think this is a bug, but the behavior is different from the DotLiquid framework we migrated from and it seems unexpected to me. Also, I've had a lot of difficulty finding clarification on the variable scoping rules in Liquid.
This issue only applies to custom blocks when using the EnterChildScope() method. Your suggestion to use the EnterForLoopScope() as an alternative is good, but I think it only gives us a partial solution. To try and clarify, here's my understanding of the actual/desired behaviors…

ChildScope.SetValue() Behavior
If Variable Defined in Parent Scope: Creates child scope variable.
If Variable Undefined in Parent Scope: Creates child scope variable.

ForLoopScope.SetValue() Behavior
If Variable Defined in Parent Scope: Modifies parent scope variable.
If Variable Undefined in Parent Scope: Creates parent scope variable.

Desired SetValue() Behavior
If Variable Defined in Parent Scope: Modifies parent scope variable.
If Variable Undefined in Parent Scope: Creates child scope variable.

** In all cases, GetValue() has the expected behavior of returning the innermost scope in which the variable is defined.

I hope this helps to define the issue more clearly. Perhaps what we are looking for is a new "EnterInheritedScope()" method?

I don't think this is a bug, but the behavior is different from the DotLiquid framework we migrated from and it seems unexpected to me.

My comment was unrelated to this issue, just generic remark about the other issues talked about during the conferences, it's all good.

Perhaps what we are looking for is a new "EnterInheritedScope()" method

Totally acceptable option. You could already replicate this behavior by tracking new local properties and adding them to the parent scope when you exit the local scope.

However that is a very suspicious behavior. Updating parent ones but creating local ones, seems counter intuitive, and definitely not following the reference behavior.

What kind of problem would it create when the new properties are creating in the parent scope (like for loops) ?

If you still need this behavior we currently can't just pass custom scope instances, it's internal, so we'll have to add a new method that allows us to set the scope instead of creating the two existing options.

@sebastienros we much prefer the decisions you've made in Fluid (being closer to the Liquid spec) that our previous library dotLiquid.

However that is a very suspicious behavior. Updating parent ones but creating local ones, seems counter intuitive, and definitely not following the reference behavior.

@sebastienros We want to stay as close to the Liquid reference behavior as possible. Do you think it's accurate to say that the scoping rule in Liquid is that a variable is read/write accessible at any point in the template after it has been created, aside from a few specific exceptions (such as the for loop)?

Based on your comments, I'm following up with one our team to see if the child scope is necessary in the custom block that is the cause of this issue. I'll post an answer when I have one. Thanks again for taking the time to look into this.

I think I am understanding the issue differently now, I understand why you would want to update the parent's scope, because otherwise how would you increment a global counter. But maybe for this tag you really don't need to create a new scope. I will check for other similar tags already existing in Liquid to see how they behave.

I'm following up with one our team to see if the child scope is necessary in the custom block that is the cause of this issue

Exactly

Also, looking at the ForLoopScope I believe it's made to be able to have local properties (index, accumulator) without interfering with parent for loops.

Looking at some official liquid tags and any tags in Fluid, I believe you should not call EnterChildScope.

I found some videos your... Some others were explained as "different behavior" instead of explaining that this is to match the reference behavior of Shopify's liquid.

That's a good point and we didn't mean to cast any shade to Fluid. To correct this we just added notes under the videos we found and our 'Fluid Differences' page. Thanks again for your work in this excellent component.

@sebastienros - Our use case is that we are injecting a number of known variables into the context for a custom block which contain the results of an operation performed by that block, and some of these variables have the same name as variables already defined in the render context. We want to isolate these result variables so that their values can be referenced inside the custom block, but when the custom block is closed these values should be discarded without affecting the original value in the parent context. At the same time, we also want to allow other context variables to have read/write access as expected in regular Liquid.

{% assign myCounter = 1 %}
My Counter is 1: {{ myCounter }}
The parent workflow ID is {{ WorkflowId }}
{% workflowactivate workflowtype:'123' %}
	The child workflow ID is {{ WorkflowId }}.
	{{ assign myCounter = 2 %}
{% endworkflowactivate %}
The parent workflow ID is {{ WorkflowId }}
My Counter should be 2: {{ myCounter }}

In this code sample, we are executing a template and passing a render context that defines a variable WorkflowId representing the identifier of the currently active workflow. Within that template, we can activate a second workflow using a custom tag workflowactivate. The identifier of the new Workflow is also added to the context as WorkflowId (this behavior is hard-coded), but that new value should be isolated to references within the workflowactivate block. Once the block is terminated, the value of WorkflowId should revert to the original value that was passed into the top-level render context. On the other hand, the value of myCounter should follow the same non-scoped access rules as regular Liquid.

Do you think there is some way we can achieve this without resorting to a new scoping behavior?

I believe what you are describing is the EnterForLoopScope. If you look at the ForStatement you'll see that it accesses the _properties property of the scope such that they are only available in this scope, but any other ones are from the parent's.

https://github.com/sebastienros/fluid/blob/main/Fluid/Ast/ForStatement.cs#L116

But this is internal ... https://github.com/sebastienros/fluid/blob/main/Fluid/Scope.cs#L10

So we need to change that and give an official way to add 'owned' properties to the scope.

Yes, I think that would do it.
The missing piece of the puzzle is the access to _properties. If we had the ability to get/set private variables in the ForLoopScope, we could achieve the hybrid behavior we are looking for.

And maybe this should not be called ForLoopScope ... I will change that

@sebastienros Thanks for fixing this issue, much appreciated. I have a working solution now, with one minor workaround - the TemplateContext.LocalScope property is internal, so I had to access it through Reflection to use the new SetOwnValue method.

fluid/Fluid/TemplateContext.cs

Line 118 in e4bb7d1

internal Scope LocalScope { get; set; }

Any thoughts on making the LocalScope property public in a future release?

Didn't think about that :/ Something I can also do is return the scope that is created when calling EnterChildScope/EnterForLoopScope. If LocalScope was made public that would break its current encapsulation in TemplateContext. Maybe in next major version this should be made public instead since it's almost useless to try to hide it right now.