Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Process member/method chains within method arguments when using np #444

Closed
zspitz opened this issue Oct 29, 2020 · 5 comments
Closed

Process member/method chains within method arguments when using np #444

zspitz opened this issue Oct 29, 2020 · 5 comments
Assignees

Comments

@zspitz
Copy link

zspitz commented Oct 29, 2020

(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.

@StefH
Copy link
Collaborator

StefH commented Oct 30, 2020

@JonathanMagnan I can take a look here.

@StefH
Copy link
Collaborator

StefH commented Oct 30, 2020

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)

@zspitz
Copy link
Author

zspitz commented Oct 30, 2020

(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.

@StefH
Copy link
Collaborator

StefH commented Oct 30, 2020

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

@zspitz
Copy link
Author

zspitz commented Oct 31, 2020

@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?

@StefH StefH closed this as not planned Won't fix, can't repro, duplicate, stale Jan 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

3 participants