Closer, some failing tests

nblumhardt · nblumhardt · commit 7e11741d8fbe · 2021-03-19T08:23:36.000+10:00
diff --git a/src/Serilog.Expressions/Expressions/Ast/AmbientNameExpression.cs b/src/Serilog.Expressions/Expressions/Ast/AmbientNameExpression.cs
@@ -2,15 +2,15 @@
 
 namespace Serilog.Expressions.Ast
 {
-    class AmbientPropertyExpression : Expression
+    class AmbientNameExpression : Expression
     {
         readonly bool _requiresEscape;
 
-        public AmbientPropertyExpression(string propertyName, bool isBuiltIn)
+        public AmbientNameExpression(string Name, bool isBuiltIn)
         {
-            PropertyName = propertyName ?? throw new ArgumentNullException(nameof(propertyName));
+            PropertyName = Name ?? throw new ArgumentNullException(nameof(Name));
             IsBuiltIn = isBuiltIn;
-            _requiresEscape = !SerilogExpression.IsValidIdentifier(propertyName);
+            _requiresEscape = !SerilogExpression.IsValidIdentifier(Name);
         }
 
         public string PropertyName { get; }
@@ -25,4 +25,4 @@ public override string ToString()
             return (IsBuiltIn ? "@" : "") + PropertyName;
         }
     }
-}
+}
diff --git a/src/Serilog.Expressions/Expressions/Ast/LocalNameExpression.cs b/src/Serilog.Expressions/Expressions/Ast/LocalNameExpression.cs
@@ -2,9 +2,9 @@
 
 namespace Serilog.Expressions.Ast
 {
-    class NamedLocalExpression : Expression
+    class LocalNameExpression : Expression
     {
-        public NamedLocalExpression(string name)
+        public LocalNameExpression(string name)
         {
             Name = name ?? throw new ArgumentNullException(nameof(name));
         }
diff --git a/src/Serilog.Expressions/Expressions/Compilation/Linq/LinqExpressionCompiler.cs b/src/Serilog.Expressions/Expressions/Compilation/Linq/LinqExpressionCompiler.cs
@@ -134,7 +134,7 @@ protected override ExpressionBody Transform(ConstantExpression cx)
             return LX.Constant(cx.Constant);
         }
 
-        protected override ExpressionBody Transform(AmbientPropertyExpression px)
+        protected override ExpressionBody Transform(AmbientNameExpression px)
         {
             if (px.IsBuiltIn)
             {
@@ -161,7 +161,7 @@ protected override ExpressionBody Transform(AmbientPropertyExpression px)
             return Splice(context => Intrinsics.GetPropertyValue(context, propertyName));
         }
 
-        protected override ExpressionBody Transform(NamedLocalExpression nlx)
+        protected override ExpressionBody Transform(LocalNameExpression nlx)
         {
             // Don't close over the AST node.
             var name = nlx.Name;
diff --git a/src/Serilog.Expressions/Expressions/Compilation/Pattern.cs b/src/Serilog.Expressions/Expressions/Compilation/Pattern.cs
@@ -8,7 +8,7 @@ static class Pattern
     {
         public static bool IsAmbientProperty(Expression expression, string name, bool isBuiltIn)
         {
-            return expression is AmbientPropertyExpression px &&
+            return expression is AmbientNameExpression px &&
                    px.PropertyName == name &&
                    px.IsBuiltIn == isBuiltIn;
         }
diff --git a/src/Serilog.Expressions/Expressions/Compilation/Properties/PropertiesObjectAccessorTransformer.cs b/src/Serilog.Expressions/Expressions/Compilation/Properties/PropertiesObjectAccessorTransformer.cs
@@ -15,7 +15,7 @@ protected override Expression Transform(AccessorExpression ax)
             if (!Pattern.IsAmbientProperty(ax.Receiver, BuiltInProperty.Properties, true))
                 return base.Transform(ax);
             
-            return new AmbientPropertyExpression(ax.MemberName, false);
+            return new AmbientNameExpression(ax.MemberName, false);
         }
 
         protected override Expression Transform(IndexerExpression ix)
@@ -24,7 +24,7 @@ protected override Expression Transform(IndexerExpression ix)
                 !Pattern.IsStringConstant(ix.Index, out var name))
                 return base.Transform(ix);
             
-            return new AmbientPropertyExpression(name, false);
+            return new AmbientNameExpression(name, false);
         }
     }
 }
diff --git a/src/Serilog.Expressions/Expressions/Compilation/Transformations/FilterExpressionTransformer`1.cs b/src/Serilog.Expressions/Expressions/Compilation/Transformations/FilterExpressionTransformer`1.cs
@@ -12,7 +12,8 @@ protected virtual TResult Transform(Expression expression)
                 CallExpression call => Transform(call),
                 ConstantExpression constant => Transform(constant),
                 AccessorExpression accessor => Transform(accessor),
-                AmbientPropertyExpression property => Transform(property),
+                AmbientNameExpression property => Transform(property),
+                LocalNameExpression local => Transform(local),
                 LambdaExpression lambda => Transform(lambda),
                 ParameterExpression parameter => Transform(parameter),
                 IndexerWildcardExpression wildcard => Transform(wildcard),
@@ -28,8 +29,8 @@ protected virtual TResult Transform(Expression expression)
 
         protected abstract TResult Transform(CallExpression lx);
         protected abstract TResult Transform(ConstantExpression cx);
-        protected abstract TResult Transform(AmbientPropertyExpression px);
-        protected abstract TResult Transform(NamedLocalExpression nlx);
+        protected abstract TResult Transform(AmbientNameExpression px);
+        protected abstract TResult Transform(LocalNameExpression nlx);
         protected abstract TResult Transform(AccessorExpression spx);
         protected abstract TResult Transform(LambdaExpression lmx);
         protected abstract TResult Transform(ParameterExpression prx);
diff --git a/src/Serilog.Expressions/Expressions/Compilation/Transformations/IdentityTransformer.cs b/src/Serilog.Expressions/Expressions/Compilation/Transformations/IdentityTransformer.cs
@@ -33,12 +33,12 @@ protected override Expression Transform(ConstantExpression cx)
             return cx;
         }
 
-        protected override Expression Transform(AmbientPropertyExpression px)
+        protected override Expression Transform(AmbientNameExpression px)
         {
             return px;
         }
 
-        protected override Expression Transform(NamedLocalExpression nlx)
+        protected override Expression Transform(LocalNameExpression nlx)
         {
             return nlx;
         }
diff --git a/src/Serilog.Expressions/Expressions/Compilation/Wildcards/WildcardSearch.cs b/src/Serilog.Expressions/Expressions/Compilation/Wildcards/WildcardSearch.cs
@@ -26,12 +26,12 @@ class WildcardSearch : SerilogExpressionTransformer<IndexerExpression?>
             return null;
         }
         
-        protected override IndexerExpression? Transform(AmbientPropertyExpression px)
+        protected override IndexerExpression? Transform(AmbientNameExpression px)
         {
             return null;
         }
 
-        protected override IndexerExpression? Transform(NamedLocalExpression nlx)
+        protected override IndexerExpression? Transform(LocalNameExpression nlx)
         {
             return null;
         }
diff --git a/src/Serilog.Expressions/Expressions/Parsing/ExpressionTokenParsers.cs b/src/Serilog.Expressions/Expressions/Parsing/ExpressionTokenParsers.cs
@@ -98,8 +98,8 @@ from value in Token.EqualTo(ExpressionToken.Colon)
             select (Member) new PropertyMember(
                 key.ToStringValue(),
                 value ?? (key.Kind == ExpressionToken.BuiltInIdentifier ?
-                    new AmbientPropertyExpression(key.ToStringValue().Substring(1), true) :
-                    new AmbientPropertyExpression(key.ToStringValue(), false)));
+                    new AmbientNameExpression(key.ToStringValue().Substring(1), true) :
+                    new AmbientNameExpression(key.ToStringValue(), false)));
 
         static readonly TokenListParser<ExpressionToken, Member> StringMember =
             from key in Token.EqualTo(ExpressionToken.String).Apply(ExpressionTextParsers.String)
@@ -123,8 +123,8 @@ from rbrace in Token.EqualTo(ExpressionToken.RBrace)
         
         static readonly TokenListParser<ExpressionToken, Expression> RootProperty =
             (from notFunction in Parse.Not(Token.EqualTo(ExpressionToken.Identifier).IgnoreThen(Token.EqualTo(ExpressionToken.LParen)))
-                from p in Token.EqualTo(ExpressionToken.BuiltInIdentifier).Select(b => (Expression) new AmbientPropertyExpression(b.ToStringValue().Substring(1), true))
-                    .Or(Token.EqualTo(ExpressionToken.Identifier).Select(t => (Expression) new AmbientPropertyExpression(t.ToStringValue(), false)))
+                from p in Token.EqualTo(ExpressionToken.BuiltInIdentifier).Select(b => (Expression) new AmbientNameExpression(b.ToStringValue().Substring(1), true))
+                    .Or(Token.EqualTo(ExpressionToken.Identifier).Select(t => (Expression) new AmbientNameExpression(t.ToStringValue(), false)))
                 select p).Named("property");
 
         static readonly TokenListParser<ExpressionToken, Expression> String =
diff --git a/src/Serilog.Expressions/Expressions/Parsing/ExpressionTokenizer.cs b/src/Serilog.Expressions/Expressions/Parsing/ExpressionTokenizer.cs
@@ -24,7 +24,9 @@ class ExpressionTokenizer : Tokenizer<ExpressionToken>
             new ExpressionKeyword("then", ExpressionToken.Then),
             new ExpressionKeyword("else", ExpressionToken.Else),
             new ExpressionKeyword("end", ExpressionToken.End),
-            new ExpressionKeyword("ci", ExpressionToken.CI)
+            new ExpressionKeyword("ci", ExpressionToken.CI),
+            new ExpressionKeyword("each", ExpressionToken.Each),
+            new ExpressionKeyword("delimit", ExpressionToken.Delimit),
         };
 
         public ExpressionTokenizer()
diff --git a/src/Serilog.Expressions/Templates/Compilation/CompiledRepetition.cs b/src/Serilog.Expressions/Templates/Compilation/CompiledRepetition.cs
@@ -56,10 +56,9 @@ public override void Evaluate(EvaluationContext ctx, TextWriter output, IFormatP
                         continue; // Should have been invalid but Serilog didn't check and so this does occur in the wild.
                     
                     if (first)
-                    {
                         first = false;
+                    else
                         _delimiter?.Evaluate(ctx, output, formatProvider);
-                    }
 
                     var local = _keyOrElementName != null
                         ? new EvaluationContext(ctx.LogEvent, Locals.Set(ctx.Locals, _keyOrElementName, element))
@@ -83,10 +82,9 @@ public override void Evaluate(EvaluationContext ctx, TextWriter output, IFormatP
                 foreach (var member in structure.Properties)
                 {
                     if (first)
-                    {
                         first = false;
+                    else
                         _delimiter?.Evaluate(ctx, output, formatProvider);
-                    }
 
                     var local = _keyOrElementName != null
                         ? new EvaluationContext(ctx.LogEvent, Locals.Set(ctx.Locals, _keyOrElementName, new ScalarValue(member.Name)))
@@ -112,10 +110,9 @@ public override void Evaluate(EvaluationContext ctx, TextWriter output, IFormatP
                 foreach (var element in dict.Elements)
                 {
                     if (first)
-                    {
                         first = false;
+                    else
                         _delimiter?.Evaluate(ctx, output, formatProvider);
-                    }
 
                     var local = _keyOrElementName != null
                         ? new EvaluationContext(ctx.LogEvent, Locals.Set(ctx.Locals, _keyOrElementName, element.Key))
diff --git a/src/Serilog.Expressions/Templates/Compilation/NameResolution/ExpressionLocalNameBinder.cs b/src/Serilog.Expressions/Templates/Compilation/NameResolution/ExpressionLocalNameBinder.cs
@@ -21,10 +21,10 @@ public static Expression BindLocalValueNames(Expression expression, IReadOnlyCol
             _localNames = localNames ?? throw new ArgumentNullException(nameof(localNames));
         }
         
-        protected override Expression Transform(AmbientPropertyExpression px)
+        protected override Expression Transform(AmbientNameExpression px)
         {
             if (!px.IsBuiltIn && _localNames.Contains(px.PropertyName))
-                return new NamedLocalExpression(px.PropertyName);
+                return new LocalNameExpression(px.PropertyName);
             
             return base.Transform(px);
         }
diff --git a/src/Serilog.Expressions/Templates/ExpressionTemplate.cs b/src/Serilog.Expressions/Templates/ExpressionTemplate.cs
@@ -6,6 +6,7 @@
 using Serilog.Expressions.Compilation;
 using Serilog.Formatting;
 using Serilog.Templates.Compilation;
+using Serilog.Templates.Compilation.NameResolution;
 using Serilog.Templates.Parsing;
 
 namespace Serilog.Templates
@@ -61,7 +62,9 @@ public static bool TryParse(
                 return false;
             }
 
-            result = new ExpressionTemplate(TemplateCompiler.Compile(parsed, DefaultFunctionNameResolver.Build(nameResolver)), formatProvider);
+            var planned = TemplateLocalNameBinder.BindLocalValueNames(parsed);
+
+            result = new ExpressionTemplate(TemplateCompiler.Compile(planned, DefaultFunctionNameResolver.Build(nameResolver)), formatProvider);
             return true;
         }
         
@@ -90,7 +93,9 @@ public ExpressionTemplate(
             if (!templateParser.TryParse(template, out var parsed, out var error))
                 throw new ArgumentException(error);
             
-            _compiled = TemplateCompiler.Compile(parsed, DefaultFunctionNameResolver.Build(nameResolver));
+            var planned = TemplateLocalNameBinder.BindLocalValueNames(parsed);
+            
+            _compiled = TemplateCompiler.Compile(planned, DefaultFunctionNameResolver.Build(nameResolver));
             _formatProvider = formatProvider;
         }
 
diff --git a/test/Serilog.Expressions.Tests/Cases/template-evaluation-cases.asv b/test/Serilog.Expressions.Tests/Cases/template-evaluation-cases.asv
@@ -24,3 +24,5 @@ A{#if false}B{#else if true}C{#end}                  ⇶ AC
 {#each a in [1,2,3]}<{a}>{#delimit},{#end}           ⇶ <1>,<2>,<3>
 {#each a in {x: 1, y: 2}}{a}{#end}                   ⇶ xy
 {#each a, b in {x: 1, y: 2}}{a}.{b}{#end}            ⇶ x.1y.2
+{#if true}A{#each a in [1]}B{a}{#end}C{#end}D        ⇶ AB1CD
+{#each a in []}{a}!{#else}none{#end}                 ⇶ none

Original file line number	Diff line number	Diff line change
`@@ -2,15 +2,15 @@`
`2`	`2`
`3`	`3`	`namespace Serilog.Expressions.Ast`
`4`	`4`	`{`
`5`		`- class AmbientPropertyExpression : Expression`
	`5`	`+ class AmbientNameExpression : Expression`
`6`	`6`	`{`
`7`	`7`	`readonly bool _requiresEscape;`
`8`	`8`
`9`		`- public AmbientPropertyExpression(string propertyName, bool isBuiltIn)`
	`9`	`+ public AmbientNameExpression(string Name, bool isBuiltIn)`
`10`	`10`	`{`
`11`		`- PropertyName = propertyName ?? throw new ArgumentNullException(nameof(propertyName));`
	`11`	`+ PropertyName = Name ?? throw new ArgumentNullException(nameof(Name));`
`12`	`12`	`IsBuiltIn = isBuiltIn;`
`13`		`- _requiresEscape = !SerilogExpression.IsValidIdentifier(propertyName);`
	`13`	`+ _requiresEscape = !SerilogExpression.IsValidIdentifier(Name);`
`14`	`14`	`}`
`15`	`15`
`16`	`16`	`public string PropertyName { get; }`
`@@ -25,4 +25,4 @@ public override string ToString()`
`25`	`25`	`return (IsBuiltIn ? "@" : "") + PropertyName;`
`26`	`26`	`}`
`27`	`27`	`}`
`28`		`-}`
	`28`	`+}`
Original file line number	Diff line number	Diff line change
`@@ -2,9 +2,9 @@`
`2`	`2`
`3`	`3`	`namespace Serilog.Expressions.Ast`
`4`	`4`	`{`
`5`		`- class NamedLocalExpression : Expression`
	`5`	`+ class LocalNameExpression : Expression`
`6`	`6`	`{`
`7`		`- public NamedLocalExpression(string name)`
	`7`	`+ public LocalNameExpression(string name)`
`8`	`8`	`{`
`9`	`9`	`Name = name ?? throw new ArgumentNullException(nameof(name));`
`10`	`10`	`}`
Original file line number	Diff line number	Diff line change
`@@ -134,7 +134,7 @@ protected override ExpressionBody Transform(ConstantExpression cx)`
`134`	`134`	`return LX.Constant(cx.Constant);`
`135`	`135`	`}`
`136`	`136`
`137`		`- protected override ExpressionBody Transform(AmbientPropertyExpression px)`
	`137`	`+ protected override ExpressionBody Transform(AmbientNameExpression px)`
`138`	`138`	`{`
`139`	`139`	`if (px.IsBuiltIn)`
`140`	`140`	`{`
`@@ -161,7 +161,7 @@ protected override ExpressionBody Transform(AmbientPropertyExpression px)`
`161`	`161`	`return Splice(context => Intrinsics.GetPropertyValue(context, propertyName));`
`162`	`162`	`}`
`163`	`163`
`164`		`- protected override ExpressionBody Transform(NamedLocalExpression nlx)`
	`164`	`+ protected override ExpressionBody Transform(LocalNameExpression nlx)`
`165`	`165`	`{`
`166`	`166`	`// Don't close over the AST node.`
`167`	`167`	`var name = nlx.Name;`
Original file line number	Diff line number	Diff line change
`@@ -8,7 +8,7 @@ static class Pattern`
`8`	`8`	`{`
`9`	`9`	`public static bool IsAmbientProperty(Expression expression, string name, bool isBuiltIn)`
`10`	`10`	`{`
`11`		`- return expression is AmbientPropertyExpression px &&`
	`11`	`+ return expression is AmbientNameExpression px &&`
`12`	`12`	`px.PropertyName == name &&`
`13`	`13`	`px.IsBuiltIn == isBuiltIn;`
`14`	`14`	`}`
Original file line number	Diff line number	Diff line change
`@@ -15,7 +15,7 @@ protected override Expression Transform(AccessorExpression ax)`
`15`	`15`	`if (!Pattern.IsAmbientProperty(ax.Receiver, BuiltInProperty.Properties, true))`
`16`	`16`	`return base.Transform(ax);`
`17`	`17`
`18`		`- return new AmbientPropertyExpression(ax.MemberName, false);`
	`18`	`+ return new AmbientNameExpression(ax.MemberName, false);`
`19`	`19`	`}`
`20`	`20`
`21`	`21`	`protected override Expression Transform(IndexerExpression ix)`
`@@ -24,7 +24,7 @@ protected override Expression Transform(IndexerExpression ix)`
`24`	`24`	`!Pattern.IsStringConstant(ix.Index, out var name))`
`25`	`25`	`return base.Transform(ix);`
`26`	`26`
`27`		`- return new AmbientPropertyExpression(name, false);`
	`27`	`+ return new AmbientNameExpression(name, false);`
`28`	`28`	`}`
`29`	`29`	`}`
`30`	`30`	`}`
Original file line number	Diff line number	Diff line change
`@@ -33,12 +33,12 @@ protected override Expression Transform(ConstantExpression cx)`
`33`	`33`	`return cx;`
`34`	`34`	`}`
`35`	`35`
`36`		`- protected override Expression Transform(AmbientPropertyExpression px)`
	`36`	`+ protected override Expression Transform(AmbientNameExpression px)`
`37`	`37`	`{`
`38`	`38`	`return px;`
`39`	`39`	`}`
`40`	`40`
`41`		`- protected override Expression Transform(NamedLocalExpression nlx)`
	`41`	`+ protected override Expression Transform(LocalNameExpression nlx)`
`42`	`42`	`{`
`43`	`43`	`return nlx;`
`44`	`44`	`}`
Original file line number	Diff line number	Diff line change
`@@ -26,12 +26,12 @@ class WildcardSearch : SerilogExpressionTransformer<IndexerExpression?>`
`26`	`26`	`return null;`
`27`	`27`	`}`
`28`	`28`
`29`		`- protected override IndexerExpression? Transform(AmbientPropertyExpression px)`
	`29`	`+ protected override IndexerExpression? Transform(AmbientNameExpression px)`
`30`	`30`	`{`
`31`	`31`	`return null;`
`32`	`32`	`}`
`33`	`33`
`34`		`- protected override IndexerExpression? Transform(NamedLocalExpression nlx)`
	`34`	`+ protected override IndexerExpression? Transform(LocalNameExpression nlx)`
`35`	`35`	`{`
`36`	`36`	`return null;`
`37`	`37`	`}`