[SMALL] Fix to #30996 - Incorrect translation of comparison of current value with owned type default value #31714
Conversation
| @@ -1797,7 +1797,7 @@ bool TryRewriteEntityEquality([NotNullWhen(true)] out Expression? result) | |||
| CreatePropertyAccessExpression(nonNullEntityReference, p), | |||
| Expression.Constant(null, p.ClrType.MakeNullable()), | |||
| nodeType != ExpressionType.Equal)) | |||
| .Aggregate((l, r) => nodeType == ExpressionType.Equal ? Expression.OrElse(l, r) : Expression.AndAlso(l, r)); | |||
| .Aggregate((l, r) => nodeType == ExpressionType.Equal ? Expression.AndAlso(l, r) : Expression.OrElse(l, r)); | |||
There was a problem hiding this comment.
There's a very similar Aggregate just above and another one below - do these also need to be changed or is it specific to this one only?
I'm also not sure I fully understand this (but who does understand optional dependents...)... Isn't the point here that at least one required property is null (as per the variable name atLeastOneNonNullValueInNullablePropertyCondition), and therefore OrElse is the right thing to do for Equal?
There was a problem hiding this comment.
the aggregate above is for the required properties (at least one must be null to satisfy the entity == null condition), and the aggregate below is for a regular case (i.e. non optional dependant with table sharing), where we just compare keys to null - also only one of them must be null to conclude that the entity itself is null.
In the aggregate that is changed we are comparing nullable properties, since there were no non-nullable properties we could rely on. Will change the variable name as it's confusing. Depending on the nodeType of the entity comparison it does different things :) if we check that entity is null, we need to check that all the nullable properties are null. In case of entity != null we need to check that at least one of them is non null.
Also, see #29206 for extra context
There was a problem hiding this comment.
Thanks for explaining @maumar! FWIW having that text as comments in the source would help a lot in this non-obvious piece of logic. Still not sure about everything in this area but looks good enough :)
There was a problem hiding this comment.
yup, I added some comments above. Don't fully understand the whole logic either but this this part I'm fairly confident about.
…with owned type default value In optional dependent table sharing scenario, when comparing the dependent to null we first look for any required properties (at least one of them needs to be null), and if we don't have any required properties we look at optional ones (even though this is somewhat ambiguous). Error was that we did similar check as with required properties (i.e. at least one of them must be null), but what we should be doing is checking that all of them are null. Fixes #30996
| @@ -1797,7 +1797,7 @@ bool TryRewriteEntityEquality([NotNullWhen(true)] out Expression? result) | |||
| CreatePropertyAccessExpression(nonNullEntityReference, p), | |||
| Expression.Constant(null, p.ClrType.MakeNullable()), | |||
| nodeType != ExpressionType.Equal)) | |||
| .Aggregate((l, r) => nodeType == ExpressionType.Equal ? Expression.OrElse(l, r) : Expression.AndAlso(l, r)); | |||
| .Aggregate((l, r) => nodeType == ExpressionType.Equal ? Expression.AndAlso(l, r) : Expression.OrElse(l, r)); | |||
There was a problem hiding this comment.
Thanks for explaining @maumar! FWIW having that text as comments in the source would help a lot in this non-obvious piece of logic. Still not sure about everything in this area but looks good enough :)
In optional dependent table sharing scenario, when comparing the dependent to null we first look for any required properties (at least one of them needs to be null), and if we don't have any required properties we look at optional ones (even though this is somewhat ambiguous). Error was that we did similar check as with required properties (i.e. at least one of them must be null, but what we should be doing is checking that all of them are null.
Fixes #30996