Fall out from #36502 #36550
Fall out from #36502 #36550
Conversation
| new ProjectionBindingExpression(_selectExpression, _projectionMembers.Peek(), typeof(ValueBuffer))); | ||
| #pragma warning disable EF1001 | ||
| // This is to handle have correct type for the shaper expression. It is later fixed in MatchTypes. |
There was a problem hiding this comment.
| // This is to handle have correct type for the shaper expression. It is later fixed in MatchTypes. | |
| // This is to handle have correct type for the shaper expression. It is later fixed in MatchTypes. | |
| // This mirrors for structural types what we do for scalars. |
| /// Changes the <see cref="Type" /> of this expression to be nullable. | ||
| /// </summary> | ||
| [EntityFrameworkInternal] | ||
| public virtual void MakeTypeNullable() |
There was a problem hiding this comment.
Naming nit: we try to use ClrType to refer to the .NET type, to avoid confusion with EF model structural types (the Type property just above comes from .NET LINQ, which is why it doesn't have the Clr prefix). Can you please add that to the new methods and private variable?
| @@ -371,8 +371,13 @@ protected override Expression VisitExtension(Expression extensionExpression) | |||
|
|
|||
| _projectionMapping[_projectionMembers.Peek()] = projection; | |||
|
|
|||
| return shaper.Update( | |||
| var result = shaper.Update( | |||
There was a problem hiding this comment.
nit: any particular reason to not make MakeTypeNullable(), to simplify the code here (and replace the switch in MatchTypes with a conditional)?
| } | ||
|
|
||
| private void AddInitializeExpressions( | ||
| HashSet<IPropertyBase> properties, | ||
| ParameterBindingInfo bindingInfo, | ||
| Expression instanceVariable, | ||
| MethodCallExpression valueBufferExpression, |
There was a problem hiding this comment.
| MethodCallExpression valueBufferExpression, | |
| MethodCallExpression getValueBufferExpression, |
| && (IsNullable(complexType) || parameters.AllowNullable == true) | ||
| structuralType is IComplexType complexType | ||
| && ReadComplexTypeDirectly(complexType) | ||
| && parameters.ClrType.IsNullableType() | ||
| ? HandleNullableComplexTypeMaterialization( |
There was a problem hiding this comment.
Now that HandleNullableComplexTypeMaterialization is only called once, maybe just inline it here? Just have a comment explaining why it's necessary etc.
| @@ -681,23 +664,15 @@ private static void CreateServiceInstances( | |||
| } | |||
| } | |||
|
|
|||
| private Expression HandleNullableComplexTypeMaterialization(IComplexType complexType, Type clrType, Expression materializeExpression, ParameterBindingInfo bindingInfo) | |||
| private Expression HandleNullableComplexTypeMaterialization( | |||
There was a problem hiding this comment.
Looking at this again, I think it's not quite right for the MaterializerSource to be handling the nullable value type problem... With the current design, the MaterializerSource really seems to be about materializing the structural type, and this logic would be better outside, in the shaper generator.
However, we already have a mess with unclear separation of concerns between the MaterializerSource and the shaper generator - let's leave it at that for now.
| /// <param name="QueryTrackingBehavior"> | ||
| /// The query tracking behavior, or <see langword="null" /> if this materialization is not from a query. | ||
| /// </param> | ||
| public readonly record struct StructuralTypeMaterializerSourceParameters( | ||
| ITypeBase StructuralType, | ||
| string InstanceName, | ||
| bool? AllowNullable, | ||
| Type ClrType, |
There was a problem hiding this comment.
Can you add an assertion in the constructor here that ClrType is either equal to StructuralType.Type, or is a nullable over it? To clear document (and enforce) that there can't be any arbitrary CLR type here.
Another idea is to have another constructor that doesn't accept ClrType as a parameter, and initializes it from StructuralType.ClrType, so that callers only need to specify it when it differs.
No description provided.