Linq.Expression.Optimizer icon indicating copy to clipboard operation
Linq.Expression.Optimizer copied to clipboard

[Feature request] Simplify null == null and null != null

Open SpaceOgre opened this issue 3 years ago • 5 comments

Description

I'm buildning a library of Select Expressions for our product and have found that I would like to do something like this:

internal static class MapperDTO
{
    internal static Expression<Func<Subject, EducationEvent, PeriodData, MyDTO>> ToDTO =>
        (subject, educationEvent, periodData) => new MyDTO
        {
            FolkbokforingskommunName = periodData == null ? subject.Folkbokforingskommun.Name : periodData.Municipality.Name,
        };
}

This is then Invoked and Expanded with LinqKit and passed down to Entity Framework.

Expression<Func<EducationEvent, MyDTO>> mapper = 
    educationEvent => MapperForDTO.ToDTO.Invoke(educationEvent.Subject, educationEvent, null);

Obviously this does not work since null is not allowed for non-static parameters (even static?) but I have looked at the expression tree and this does create an IIF condition that looks like this:

IIF(null == null, subject.Folkbokforingskommun.Name, null.Municipality.Name)

Would it be much work to eliminate the IIF and just replace it with the correct branching?

SpaceOgre avatar Sep 22 '22 07:09 SpaceOgre

Basically this should already happen... I expect IIF is a ConditionalExpression ?

It says that in conditional expression, visit also the test-part here

In your case, the test part, I expect to be a BinaryExpression which should be replaced to "true" due to this rule

And then when it's a true, it should be replaced via this rule

So why is it not working... Maybe there is some problem with comparing null-expression to null-expression, e.g. can they differ the type level information? Or are the Left and Right part of your BinaryExpression something else than Constants? Like "null"s here?

Thorium avatar Sep 22 '22 11:09 Thorium

Thanks for the information and extensive reply. I will try and see if I can get more information about this tomorrow.

The IIF was probably from the ToString representation of the expression in debug mode will try and see if I can se the real ExpressionTypes instead.

SpaceOgre avatar Sep 22 '22 15:09 SpaceOgre

I have looked at it now and it seems like they differ on the type level: image

But even when I change to Object type it does not help: image

But if I change the compare part in C# to this:

FolkbokforingskommunNameSubject = null == null ? subject.Folkbokforingskommun.Name : periodData.Municipality.Namn,

Then it is reduced so I think it have to be this line that is not working for this perticilure case: https://github.com/Thorium/Linq.Expression.Optimizer/blob/68cf0e4ab046a496bb3c2624e72d72b2c4854f12/src/Code/ExpressionOptimizer.fs#L44

I added the IComparable interface to the Class in question and now it works, I did not even have to implement the CompareTo method for it to work

public int CompareTo(object obj)
{
    throw new NotImplementedException();
}

so it seems like it is only checking if the IComparable interface exist on the type but don't use it.

It would be nice if I would not have to add that interface on my class. I'm not good at writning F# (I'm a C# dev) but would it be possible to add an extra Match just for nulls? Maybe something like this?

ExpressionType.Constant, (:? ConstantExpression as ce) when (isNull ce.Value) -> Some (null)

SpaceOgre avatar Sep 23 '22 09:09 SpaceOgre

I think the proper special case here, is that IComparable should be needed for >=, >, '<', and <=, but the equality == and != should require IEquatable. So adding a support for IEquatable should help here.

I think it'd be a reasonable addition.

The reason I'm kind of cautious to create these are that if someone compares more complex objects than primitive types, like your PeriodData here, the likelihood of someone adding a side-effect to any cast or comparison increases, and eliminating these in LINQ will kill the side-effects away.

Thorium avatar Sep 23 '22 11:09 Thorium

I think that sounds like a great option.

To minimize the effect of this, maybe add some kind of settings parameter, so the user can configure if the new check is desired? This would be more work of course, but it would also make sure that the new check is not used until it is op-in, and thus it would not break any existing code-bases.

In the meantime I did this:

private static Expression<Func<Subject, EducationEvent, bool, PeriodData, MyListDTO>> BaseExpression =>
    (subject, educationEvent, usePeriodData, periodData) => new MyListDTO
    {
        FolkbokforingskommunName = usePeriodData ? periodData.Municipality.Namn : subject.Folkbokforingskommun.Name,
    };

And I set the usePeriodData to true when periodData exists. By doing it like this the Optimizer will pick the correct branch in the ConditionExpression.

SpaceOgre avatar Sep 23 '22 11:09 SpaceOgre