Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 59 additions & 4 deletions src/EFCore.Design/Query/Internal/CSharpToLinqTranslator.cs
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,37 @@ public virtual Expression Translate(SyntaxNode node, SemanticModel semanticModel
public override Expression? Visit(SyntaxNode? node)
=> base.Visit(node);

/// <summary>
/// This method gets called when the expression context provides an expected CLR type. For example, in <c>Foo(x)</c>, x gets visited
/// with an expected type based on <c>Foo</c>'s parameter; this may determine how x gets translated, require a LINQ Convert node, or
/// similar. In contrast, in <c>var y = x</c>, there is no context providing an expected type, and the type of <c>x</c> simply
/// bubbles out.
/// </summary>
[return: NotNullIfNotNull("node")]
private Expression? Visit(SyntaxNode? node, Type? expectedType)
{
if (expectedType is null)
{
return Visit(node);
}

var result = node switch
{
ArgumentSyntax s => VisitArgument(s, expectedType),

// For lambdas, we generate a different node based on the expected type (e.g. an Action<T> rather than a Func<T, T2>, even if
// the lambda body does return a T2).
SimpleLambdaExpressionSyntax s => VisitLambdaExpression(s, expectedType),
ParenthesizedLambdaExpressionSyntax s => VisitLambdaExpression(s, expectedType),

_ => Visit(node),
};

// TODO: Insert necessary Convert nodes etc. when the expected and actual types differ
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Add issue #

Copy link
Copy Markdown
Member Author

@roji roji Jan 13, 2025

Choose a reason for hiding this comment

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

Unless you have strong feelings about this, I'd rather not systematically pollute our repo with every TODO that I put in code... In many cases that's useful, but in cases like this I really don't see the value of it, and would rather not do it just because we have a rule... Let me know - I'd ultimately rather remove the TODO, but I don't think that's a positive thing.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We can discuss this again, but generally, non-tracked TODO comments make the code look less professional and usually don't have enough context to properly reason about for someone else. I'd much rather have an issue with a checklist of all the small changes in an area so that they can be discussed and prioritized

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

OK, sure thing. I guess I don't generally consider TODO comments as looking unprofessional (regardless of whether they reference an issue or not), and I'm just looking to reduce process and needless issues. CSharpToLinqTranslator specifically is an extreme case as there really are a lot of unimplemented cases (many theoretical, some less so).

But I'm open to doing as you say here, maybe e.g. a single issue for all pending CSharpToLinqTranslator isn't too bad.


return result;
}

/// <summary>
/// This is an internal API that supports the Entity Framework Core infrastructure and not subject to
/// the same compatibility standards as public APIs. It may be changed or removed without notice in
Expand Down Expand Up @@ -180,13 +211,16 @@ public override Expression VisitAnonymousObjectCreationExpression(AnonymousObjec
/// doing so can result in application failures when updating to a new Entity Framework Core release.
/// </summary>
public override Expression VisitArgument(ArgumentSyntax argument)
=> VisitArgument(argument, expectedType: null);

private Expression VisitArgument(ArgumentSyntax argument, Type? expectedType)
{
if (!argument.RefKindKeyword.IsKind(SyntaxKind.None))
{
throw new InvalidOperationException($"Argument with ref/out: {argument}");
}

return Visit(argument.Expression);
return Visit(argument.Expression, expectedType);
}

/// <summary>
Expand Down Expand Up @@ -682,7 +716,7 @@ parameter.DefaultValue is null && parameter.ParameterType.IsValueType
// Positional argument
if (argument.NameColon is null)
{
destArguments[paramIndex] = Visit(argument);
destArguments[paramIndex] = Visit(argument, parameter.ParameterType);
continue;
}

Expand Down Expand Up @@ -1009,7 +1043,7 @@ public override Expression VisitTypeOfExpression(TypeOfExpressionSyntax typeOf)
public override Expression DefaultVisit(SyntaxNode node)
=> throw new NotSupportedException($"Unsupported syntax node of type '{node.GetType()}': {node}");

private Expression VisitLambdaExpression(AnonymousFunctionExpressionSyntax lambda)
private Expression VisitLambdaExpression(AnonymousFunctionExpressionSyntax lambda, Type? expectedType = null)
{
if (lambda.ExpressionBody is null)
{
Expand Down Expand Up @@ -1055,7 +1089,28 @@ private Expression VisitLambdaExpression(AnonymousFunctionExpressionSyntax lambd
try
{
var body = Visit(lambda.ExpressionBody);
return Lambda(body, translatedParameters);

return expectedType switch
{
// If there's no contextual expected type, we allow the lambda's type to be inferred from its parameters and the body's
// return type.
null => Lambda(body, translatedParameters),

// This is for the case where the expected type is Action<T>, but the lambda body does return something, which needs to get
// ignored; for example, the ExecuteUpdateAsync setter parameter is Action<UpdateSettersBuilder>, but the function is
// invoked with ExecuteUpdateAsync(s => s.SetProperty(...)), and SetProperty() returns UpdateSettersBuilder for further
// chaining. In this case, the body's return type is an UpdateSettersBuilder, meaning that the type of the constructed
// lambda here would be Func<UpdateSettersBuilder, UpdateSettersBuilder>, and not Action<UpdateSettersBuilder> as
// ExecuteUpdateAsync's signature requires.
// Identify this case, and explicitly type the returned lambda as an Action when necessary.
_ when expectedType.IsGenericType && expectedType.IsAssignableTo(typeof(MulticastDelegate))
=> Lambda(expectedType, body, translatedParameters),

_ when expectedType.IsGenericType && expectedType.GetGenericTypeDefinition() == typeof(Expression<>)
=> Lambda(expectedType.GetGenericArguments()[0], body, translatedParameters),

_ => throw new UnreachableException()
};
}
finally
{
Expand Down
29 changes: 16 additions & 13 deletions src/EFCore.Design/Query/Internal/PrecompiledQueryCodeGenerator.cs
Original file line number Diff line number Diff line change
Expand Up @@ -743,7 +743,7 @@ void ProcessCapturedVariables()

ExpressionTreeFuncletizer.PathNode? evaluatableRootPaths;

// ExecuteUpdate requires really special handling: the function accepts a Func<SetPropertyCalls...> argument, but
// ExecuteUpdate requires really special handling: the function accepts a Func<UpdateSettersBuilder...> argument, but
// we need to run funcletization on the setter lambdas added via that Func<>.
if (operatorMethodCall.Method is
{
Expand All @@ -753,7 +753,7 @@ or nameof(EntityFrameworkQueryableExtensions.ExecuteUpdateAsync),
}
&& operatorMethodCall.Method.DeclaringType == typeof(EntityFrameworkQueryableExtensions))
{
// First, statically convert the Func<SetPropertyCalls...> to a NewArrayExpression which represents all the
// First, statically convert the Action<UpdateSettersBuilder> to a NewArrayExpression which represents all the
// setters; since that's an expression, we can run the funcletizer on it.
var settersExpression = ProcessExecuteUpdate(operatorMethodCall);
evaluatableRootPaths = _funcletizer.CalculatePathsToEvaluatableRoots(settersExpression);
Expand All @@ -767,8 +767,11 @@ or nameof(EntityFrameworkQueryableExtensions.ExecuteUpdateAsync),
// If there were captured variables, generate code to evaluate and build the same NewArrayExpression at runtime,
// and then fall through to the normal logic, generating variable extractors against that NewArrayExpression
// (local var) instead of against the method argument.
code.AppendLine(
$"var setters = {parameterName}(new SetPropertyCalls<{sourceElementTypeName}>()).BuildSettersExpression();");
code.AppendLine($"""
var setterBuilder = new UpdateSettersBuilder<{sourceElementTypeName}>();
{parameterName}(setterBuilder);
var setters = setterBuilder.BuildSettersExpression();
""");
parameterName = "setters";
parameterType = typeof(NewArrayExpression);
}
Expand Down Expand Up @@ -1114,7 +1117,7 @@ or nameof(EntityFrameworkQueryableExtensions.ToListAsync)
=> RewriteToSync(
typeof(EntityFrameworkQueryableExtensions).GetMethod(nameof(EntityFrameworkQueryableExtensions.ExecuteDelete))),

// ExecuteUpdate is special; it accepts a non-expression-tree argument (Func<SetPropertyCalls, SetPropertyCalls>),
// ExecuteUpdate is special; it accepts a non-expression-tree argument (Action<UpdateSettersBuilder>),
// evaluates it immediately, and injects a different MethodCall node into the expression tree with the resulting setter
// expressions.
// When statically analyzing ExecuteUpdate, we have to manually perform the same thing.
Expand Down Expand Up @@ -1159,11 +1162,11 @@ MethodCallExpression RewriteToSync(MethodInfo? syncMethod)
}
}

// Accepts an expression tree representing a series of SetProperty() calls, parses them and passes them through the SetPropertyCalls
// builder; returns the resulting NewArrayExpression representing all the setters.
// Accepts an expression tree representing a series of SetProperty() calls, parses them and passes them through the
// UpdateSettersBuilder; returns the resulting NewArrayExpression representing all the setters.
private static NewArrayExpression ProcessExecuteUpdate(MethodCallExpression executeUpdateCall)
{
var setPropertyCalls = Activator.CreateInstance<SetPropertyCalls>();
var settersBuilder = new UpdateSettersBuilder();
var settersLambda = (LambdaExpression)executeUpdateCall.Arguments[1];
var settersParameter = settersLambda.Parameters.Single();
var expression = settersLambda.Body;
Expand All @@ -1175,7 +1178,7 @@ private static NewArrayExpression ProcessExecuteUpdate(MethodCallExpression exec
Method:
{
IsGenericMethod: true,
Name: nameof(SetPropertyCalls<int>.SetProperty),
Name: nameof(UpdateSettersBuilder<int>.SetProperty),
DeclaringType.IsGenericType: true,
},
Arguments:
Expand All @@ -1184,19 +1187,19 @@ private static NewArrayExpression ProcessExecuteUpdate(MethodCallExpression exec
Expression valueSelector
]
} methodCallExpression
&& methodCallExpression.Method.DeclaringType.GetGenericTypeDefinition() == typeof(SetPropertyCalls<>))
&& methodCallExpression.Method.DeclaringType.GetGenericTypeDefinition() == typeof(UpdateSettersBuilder<>))
{
if (valueSelector is UnaryExpression
{
NodeType: ExpressionType.Quote,
Operand: LambdaExpression unwrappedValueSelector
})
{
setPropertyCalls.SetProperty(propertySelector, unwrappedValueSelector);
settersBuilder.SetProperty(propertySelector, unwrappedValueSelector);
}
else
{
setPropertyCalls.SetProperty(propertySelector, valueSelector);
settersBuilder.SetProperty(propertySelector, valueSelector);
}

expression = methodCallExpression.Object;
Expand All @@ -1206,7 +1209,7 @@ Expression valueSelector
throw new InvalidOperationException(RelationalStrings.InvalidArgumentToExecuteUpdate);
}

return setPropertyCalls.BuildSettersExpression();
return settersBuilder.BuildSettersExpression();
}

/// <summary>
Expand Down
2 changes: 1 addition & 1 deletion src/EFCore.Relational/Properties/TypeForwards.cs
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,4 @@
[assembly: TypeForwardedTo(typeof(AttributeCodeFragment))]
[assembly: TypeForwardedTo(typeof(MethodCallCodeFragment))]
[assembly: TypeForwardedTo(typeof(NestedClosureCodeFragment))]
[assembly: TypeForwardedTo(typeof(SetPropertyCalls<>))]
[assembly: TypeForwardedTo(typeof(UpdateSettersBuilder<>))]
24 changes: 18 additions & 6 deletions src/EFCore/Extensions/EntityFrameworkQueryableExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3337,12 +3337,18 @@ internal static readonly MethodInfo ExecuteDeleteMethodInfo
/// <returns>The total number of rows updated in the database.</returns>
public static int ExecuteUpdate<TSource>(
this IQueryable<TSource> source,
Func<SetPropertyCalls<TSource>, SetPropertyCalls<TSource>> setPropertyCalls)
=> source.Provider.Execute<int>(
Action<UpdateSettersBuilder<TSource>> setPropertyCalls)
{
var setterBuilder = new UpdateSettersBuilder<TSource>();
setPropertyCalls(setterBuilder);
var setters = setterBuilder.BuildSettersExpression();

return source.Provider.Execute<int>(
Expression.Call(
ExecuteUpdateMethodInfo.MakeGenericMethod(typeof(TSource)),
source.Expression,
setPropertyCalls(new SetPropertyCalls<TSource>()).BuildSettersExpression()));
setters));
}

/// <summary>
/// Asynchronously updates database rows for the entity instances which match the LINQ query from the database.
Expand All @@ -3366,16 +3372,22 @@ public static int ExecuteUpdate<TSource>(
[DynamicDependency("ExecuteUpdate``1(System.Linq.IQueryable{``1},System.Collections.Generic.IReadOnlyList{ITuple})", typeof(EntityFrameworkQueryableExtensions))]
public static Task<int> ExecuteUpdateAsync<TSource>(
this IQueryable<TSource> source,
Func<SetPropertyCalls<TSource>, SetPropertyCalls<TSource>> setPropertyCalls,
Action<UpdateSettersBuilder<TSource>> setPropertyCalls,
CancellationToken cancellationToken = default)
=> source.Provider is IAsyncQueryProvider provider
{
var setterBuilder = new UpdateSettersBuilder<TSource>();
setPropertyCalls(setterBuilder);
var setters = setterBuilder.BuildSettersExpression();

return source.Provider is IAsyncQueryProvider provider
? provider.ExecuteAsync<Task<int>>(
Expression.Call(
ExecuteUpdateMethodInfo.MakeGenericMethod(typeof(TSource)),
source.Expression,
setPropertyCalls(new SetPropertyCalls<TSource>()).BuildSettersExpression()),
setters),
cancellationToken)
: throw new InvalidOperationException(CoreStrings.IQueryableProviderNotAsync);
}

private static int ExecuteUpdate<TSource>(this IQueryable<TSource> source, [NotParameterized] IReadOnlyList<ITuple> setters)
=> throw new UnreachableException("Can't call this overload directly");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,7 @@ protected override Expression VisitMethodCall(MethodCallExpression methodCallExp
var valueSelector = @new.Arguments[1];

// When the value selector is a bare value type (no lambda), a cast-to-object Convert node needs to be added
// for proper typing (see SetPropertyCalls); remove it here.
// for proper typing (see UpdateSettersBuilder); remove it here.
if (valueSelector is UnaryExpression { NodeType: ExpressionType.Convert, Operand: var unwrappedValueSelector }
&& valueSelector.Type == typeof(object))
{
Expand Down Expand Up @@ -1078,15 +1078,15 @@ protected abstract ShapedQueryExpression TranslateSelect(
/// <summary>
/// Translates
/// <see
/// cref="EntityFrameworkQueryableExtensions.ExecuteUpdate{TSource}(IQueryable{TSource}, Func{SetPropertyCalls{TSource}, SetPropertyCalls{TSource}})" />
/// cref="EntityFrameworkQueryableExtensions.ExecuteUpdate{TSource}(IQueryable{TSource}, Action{UpdateSettersBuilder{TSource}})" />
/// method
/// over the given source.
/// </summary>
/// <param name="source">The shaped query on which the operator is applied.</param>
/// <param name="setters">
/// The setters for this
/// <see
/// cref="EntityFrameworkQueryableExtensions.ExecuteUpdate{TSource}(IQueryable{TSource}, Func{SetPropertyCalls{TSource}, SetPropertyCalls{TSource}})" />
/// cref="EntityFrameworkQueryableExtensions.ExecuteUpdate{TSource}(IQueryable{TSource}, Action{UpdateSettersBuilder{TSource}})" />
/// call.
/// </param>
/// <returns>The non query after translation.</returns>
Expand All @@ -1097,7 +1097,7 @@ protected abstract ShapedQueryExpression TranslateSelect(

/// <summary>
/// Represents a single setter in an
/// <see cref="EntityFrameworkQueryableExtensions.ExecuteUpdate{TSource}(IQueryable{TSource}, Func{SetPropertyCalls{TSource}, SetPropertyCalls{TSource}})" />
/// <see cref="EntityFrameworkQueryableExtensions.ExecuteUpdate{TSource}(IQueryable{TSource}, Action{UpdateSettersBuilder{TSource}})" />
/// call, i.e. a pair of property and value selectors.
/// </summary>
public sealed record ExecuteUpdateSetter(LambdaExpression PropertySelector, Expression ValueExpression);
Expand Down
12 changes: 6 additions & 6 deletions src/EFCore/Query/SetPropertyCalls`.cs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
namespace Microsoft.EntityFrameworkCore.Query;

/// <inheritdoc />
public sealed class SetPropertyCalls<TSource> : SetPropertyCalls
public sealed class UpdateSettersBuilder<TSource> : UpdateSettersBuilder
{
/// <summary>
/// This is an internal API that supports the Entity Framework Core infrastructure and not subject to
Expand All @@ -13,7 +13,7 @@ public sealed class SetPropertyCalls<TSource> : SetPropertyCalls
/// doing so can result in application failures when updating to a new Entity Framework Core release.
/// </summary>
[EntityFrameworkInternal]
public SetPropertyCalls()
public UpdateSettersBuilder()
{
}

Expand All @@ -25,10 +25,10 @@ public SetPropertyCalls()
/// <param name="valueExpression">A value expression.</param>
/// <returns>
/// The same instance so that multiple calls to
/// <see cref="SetPropertyCalls{TSource}.SetProperty{TProperty}(Expression{Func{TSource, TProperty}}, Expression{Func{TSource, TProperty}})" />
/// <see cref="UpdateSettersBuilder{TSource}.SetProperty{TProperty}(Expression{Func{TSource, TProperty}}, Expression{Func{TSource, TProperty}})" />
/// can be chained.
/// </returns>
public SetPropertyCalls<TSource> SetProperty<TProperty>(
public UpdateSettersBuilder<TSource> SetProperty<TProperty>(
Expression<Func<TSource, TProperty>> propertyExpression,
Expression<Func<TSource, TProperty>> valueExpression)
{
Expand All @@ -44,9 +44,9 @@ public SetPropertyCalls<TSource> SetProperty<TProperty>(
/// <param name="valueExpression">A value expression.</param>
/// <returns>
/// The same instance so that multiple calls to
/// <see cref="SetPropertyCalls{TSource}.SetProperty{TProperty}(Expression{Func{TSource, TProperty}}, TProperty)" /> can be chained.
/// <see cref="UpdateSettersBuilder{TSource}.SetProperty{TProperty}(Expression{Func{TSource, TProperty}}, TProperty)" /> can be chained.
/// </returns>
public SetPropertyCalls<TSource> SetProperty<TProperty>(
public UpdateSettersBuilder<TSource> SetProperty<TProperty>(
Expression<Func<TSource, TProperty>> propertyExpression,
TProperty valueExpression)
{
Expand Down
Loading