System.Linq.Dynamic.Core icon indicating copy to clipboard operation
System.Linq.Dynamic.Core copied to clipboard

Process member/method chains within method arguments when using `np`

Open zspitz opened this issue 5 years ago • 5 comments

(Creating a new issure, at @StefH 's request.)

With #443, the result of method calls within a member chain when using np:

np(FooValue.Zero().Length)

are also null-checked, and the resultant expression looks like this:

Param_0 => IIF((((Param_0 != null) AndAlso (Param_0.FooValue != null)) AndAlso (Param_0.FooValue.Zero() != null)), Convert(Param_0.FooValue.Zero().Length), null)

or the C# equivalent:

$var0 => 
    $var0 != null && $var0.FooValue != null && $var0.FooValue.Zero() != null ?
        $var0.FooValue.Zero().Length :
        null

whether the method has zero, one, or more arguments.

However, if there are member/method chains within the arguments to the method:

np(FooValue.Two(
    FooValue.Zero().Length,
    FooValue.One(42).Length
).Length)

I would expect the same treatment:

$var0 => 
    $var0 != null && $var0.FooValue != null && $var0.FooValue.Zero() != null && $var0.FooValue.One(42) != null &&
            $var0.FooValue.Two($var0.FooValue.Zero().Length, $var0.FooValue.One(42).Length) != null ?
        $var0.FooValue.Zero().Length :
        null

(Note that 42 doesn't need to be checked for null, either because it's type (presumably int) cannot take null, or because it's not a member access or a method call.

zspitz avatar Oct 29 '20 06:10 zspitz

@JonathanMagnan I can take a look here.

StefH avatar Oct 30 '20 07:10 StefH

Hello @zspitz.

Actually, I think we already support this, you just have to use np() again.

Like:

np(FooValue.Two(
    np(FooValue.Zero().Length),
    np(FooValue.One(42).Length)
).Length)

This should just work.

And this also makes more sense, because you can also provide a default value. Like:

np(FooValue.Two(
    np(FooValue.Zero().Length, 77), // in case np resolves to null use value 77
    np(FooValue.One(42).Length, 88) // in case np resolves to null use value 88
).Length)

StefH avatar Oct 30 '20 09:10 StefH

(I apologize; I can't reply at length ATM.)

I think having this limitation on np would feel rather arbitrary.

But in any case, it needs to be documented.

zspitz avatar Oct 30 '20 09:10 zspitz

Technically I think it's possible to support code like:

np(FooValue.Two(FooValue.Zero().Length, FooValue.One(42).Length).Length)

However this would mean that when using np ,all arguments will be treated as np, which could lead to some undesired functionality from the 'main' np method.

So to my opinion, these options are possible:

  • keep the code as-is and update the documentation to explain how the np method works
  • update the code to support your scenario, however this will be implemented by a new NullPropagation method like np_all

StefH avatar Oct 30 '20 17:10 StefH

@StefH

Could you elaborate on this:

However this would mean that when using np ,all arguments will be treated as np, which could lead to some undesired functionality from the 'main' np method.

What undesired functionality do you anticipate?

zspitz avatar Oct 31 '20 19:10 zspitz