Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed issue with lambdas and System.Action #995

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,19 @@ public void BindingCompiler_Invalid_LambdaParameters(string expr)
Assert.ThrowsException<AggregateException>(() => ExecuteBinding(expr, viewModel));
}

[TestMethod]
[DataRow("(TestViewModel vm) => vm.IntProp = 11")]
[DataRow("(TestViewModel vm) => vm.GetEnum()")]
[DataRow("(TestViewModel vm) => ()")]
[DataRow("(TestViewModel vm) => ;")]
public void BindingCompiler_Valid_LambdaToAction(string expr)
{
var viewModel = new TestViewModel();
var binding = ExecuteBinding(expr, new[] { viewModel }, null, expectedType: typeof(Action<TestViewModel>)) as Action<TestViewModel>;
Assert.AreEqual(typeof(Action<TestViewModel>), binding.GetType());
binding.Invoke(viewModel);
}

[TestMethod]
public void BindingCompiler_Valid_ExtensionMethods()
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ public BindingExpressionBuilder(CompiledAssemblyCache compiledAssemblyCache, Ext
this.extensionMethodsCache = extensionMethodsCache;
}

public Expression Parse(string expression, DataContextStack dataContexts, BindingParserOptions options, params KeyValuePair<string, Expression>[] additionalSymbols)
public Expression Parse(string expression, DataContextStack dataContexts, BindingParserOptions options, Type expectedType = null, params KeyValuePair<string, Expression>[] additionalSymbols)
{
try
{
Expand Down Expand Up @@ -54,7 +54,7 @@ public Expression Parse(string expression, DataContextStack dataContexts, Bindin
symbols = symbols.AddSymbols(options.ExtensionParameters.Select(p => CreateParameter(dataContexts, p.Identifier, p)));
symbols = symbols.AddSymbols(additionalSymbols);

var visitor = new ExpressionBuildingVisitor(symbols, memberExpressionFactory);
var visitor = new ExpressionBuildingVisitor(symbols, memberExpressionFactory, expectedType);
visitor.Scope = symbols.Resolve(options.ScopeParameter);
return visitor.Visit(node);
}
Expand Down Expand Up @@ -143,10 +143,10 @@ public static Expression ParseWithLambdaConversion(this IBindingExpressionBuilde
extensionParameter: new TypeConversion.MagicLambdaConversionExtensionParameter(index, p.Name, p.ParameterType)))
))
.ToArray();
return builder.Parse(expression, dataContexts, options, additionalSymbols.Concat(delegateSymbols).ToArray());
return builder.Parse(expression, dataContexts, options, expectedType, additionalSymbols.Concat(delegateSymbols).ToArray());
}
else
return builder.Parse(expression, dataContexts, options, additionalSymbols);
return builder.Parse(expression, dataContexts, options, expectedType, additionalSymbols);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
using System.Threading.Tasks;
using System.Collections.Immutable;
using System.Diagnostics.CodeAnalysis;
using System.Reflection;

namespace DotVVM.Framework.Compilation.Binding
{
Expand All @@ -17,15 +18,18 @@ public class ExpressionBuildingVisitor : BindingParserNodeVisitor<Expression>
public TypeRegistry Registry { get; set; }
public Expression? Scope { get; set; }
public bool ResolveOnlyTypeName { get; set; }
public Type? ExpectedType { get; set; }
public ImmutableDictionary<string, ParameterExpression> Variables { get; set; } =
ImmutableDictionary<string, ParameterExpression>.Empty;

private int expressionDepth;
private List<Exception>? currentErrors;
private readonly MemberExpressionFactory memberExpressionFactory;

public ExpressionBuildingVisitor(TypeRegistry registry, MemberExpressionFactory memberExpressionFactory)
public ExpressionBuildingVisitor(TypeRegistry registry, MemberExpressionFactory memberExpressionFactory, Type? expectedType = null)
{
Registry = registry;
ExpectedType = expectedType;
this.memberExpressionFactory = memberExpressionFactory;
}

Expand Down Expand Up @@ -90,13 +94,15 @@ public override Expression Visit(BindingParserNode node)
var errors = currentErrors;
try
{
expressionDepth++;
ThrowIfNotTypeNameRelevant(node);
return base.Visit(node);
}
finally
{
currentErrors = errors;
Registry = regBackup;
expressionDepth--;
}
}

Expand Down Expand Up @@ -317,7 +323,7 @@ protected override Expression VisitLambda(LambdaBindingParserNode node)
var body = Visit(node.BodyExpression);

ThrowOnErrors();
return Expression.Lambda(body, lambdaParameters);
return CreateLambdaExpression(body, lambdaParameters);
}

protected override Expression VisitLambdaParameter(LambdaParameterBindingParserNode node)
Expand All @@ -329,6 +335,28 @@ protected override Expression VisitLambdaParameter(LambdaParameterBindingParserN
return Expression.Parameter(parameterType, node.Name.ToDisplayString());
}

private Expression CreateLambdaExpression(Expression body, ParameterExpression[] parameters)
{
// Check if we have type information available
if (expressionDepth == 1 && ExpectedType != null && ReflectionUtils.IsDelegate(ExpectedType))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ExpectedType is the top level expected type. However, the lambda may be used somewhere inside of the expression. ExpectedType might be string, while the expression is MyMethod(x => Prop = x) and it will lead to the same problem with Action and Func

{
if (ExpectedType.GetMethod("Invoke", BindingFlags.Public | BindingFlags.Instance).ReturnType == typeof(void))
{
// This should be an Action<...>
// We must validate that lambda body contains a valid statement
if ((body.NodeType != ExpressionType.Default) && (body.NodeType != ExpressionType.Block) && (body.NodeType != ExpressionType.Call) && (body.NodeType != ExpressionType.Assign))
throw new DotvvmCompilationException($"Only method invocations and assignments can be used as statements.");

// Make sure the result type will be void by adding an empty expression
return Expression.Lambda(Expression.Block(body, Expression.Empty()), parameters);
}
}

// This should be an Func<...>
// Also this is the default behaviour if no further type information was provided
return Expression.Lambda(body, parameters);
}

protected override Expression VisitBlock(BlockBindingParserNode node)
{
var left = HandleErrors(node.FirstExpression, Visit) ?? Expression.Default(typeof(void));
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
#nullable enable
using System;
using System.Collections.Generic;
using System.Linq.Expressions;
using DotVVM.Framework.Compilation.ControlTree;
Expand All @@ -8,6 +9,6 @@ namespace DotVVM.Framework.Compilation
{
public interface IBindingExpressionBuilder
{
Expression Parse(string expression, DataContextStack dataContexts, BindingParserOptions options, params KeyValuePair<string, Expression>[] additionalSymbols);
Expression Parse(string expression, DataContextStack dataContexts, BindingParserOptions options, Type? expectedType = null, params KeyValuePair<string, Expression>[] additionalSymbols);
}
}