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

np operator ignores single member access when no default value passed in #520

Closed
zspitz opened this issue Jun 17, 2021 · 12 comments
Closed

Comments

@zspitz
Copy link

zspitz commented Jun 17, 2021

1. Description

When using np with only one member access, the np operator seems to be ignored:

class Person {
    public string LastName { get; set; }
}

var expr = Enumerable.Empty<Person>().AsQueryable()
        .Select("np(it.LastName)")
        .Expression;
Console.WriteLine(expr.ToString("DebugView"));
/*
      .Call System.Linq.Queryable.Select(
          .Constant<System.Linq.EnumerableQuery`1[_tests.Person]>(_tests.Person[]),
          '(.Lambda #Lambda1<System.Func`2[_tests.Person,System.String]>))
      
      .Lambda #Lambda1<System.Func`2[_tests.Person,System.String]>(_tests.Person $var1) {
          $var1.LastName
      }
*/

Using np with multiple levels of member access seems to work, e.g. np(it.LastName.ToString()):

/*
        .Call System.Linq.Queryable.Select(
            .Constant<System.Linq.EnumerableQuery`1[_tests.Person]>(_tests.Person[]),
            '(.Lambda #Lambda1<System.Func`2[_tests.Person,System.String]>))
        
        .Lambda #Lambda1<System.Func`2[_tests.Person,System.String]>(_tests.Person $var1) {
            .If (
                $var1 != null && $var1.LastName != null
            ) {
                .Call ($var1.LastName).ToString()
            } .Else {
                null
            }
        }
*/

3. Fiddle or Project

https://dotnetfiddle.net/Ig09hL

@StefH
Copy link
Collaborator

StefH commented Jun 18, 2021

Your observation is true.

However, the only scenario in which this specific np() functionality is required to use if the Person can be null, correct?

@zspitz
Copy link
Author

zspitz commented Jun 18, 2021

However, the only scenario in which this specific np() functionality is required to use if the Person can be null, correct?

Yes.

@StefH
Copy link
Collaborator

StefH commented Jun 19, 2021

@zspitz
I cannot reproduce this with the unit tests, see PR #522

@StefH
Copy link
Collaborator

StefH commented Jun 24, 2021

@zspitz Can you take a look at that PR to verify the unit tests?

@zspitz
Copy link
Author

zspitz commented Jun 24, 2021

@StefH I cannot compile the solution:

Project "C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\MSBuild\Microsoft\WindowsXaml\v16.0\Microsoft.Windows.UI.Xaml.CSharp.targets" was not imported by "C:\Program Files\dotnet\sdk\5.0.301\Sdks\Microsoft.NET.Sdk\Sdk\Sdk.targets" at (37,3), due to the file not existing.

under the System.Linq.Dynamic.Core project.

@zspitz
Copy link
Author

zspitz commented Jun 24, 2021

@StefH The unit test at ExpressionTests_NullPropagatingNested1_WithDefaultValue is passing, because when a default value is passed into np, the single-member access is evaluated correctly.

However, when no default value is passed, np doesn't transform the single-member access, as can be seen from the InlineData at line 1481.

@StefH
Copy link
Collaborator

StefH commented Jun 24, 2021

To compile it, you need
image

or just change <TargetFrameworks>net35;net40;net45;net46;netstandard1.3;netstandard2.0;uap10.0;netcoreapp2.1</TargetFrameworks> to <TargetFrameworks>net35;net40;net45;net46;netstandard1.3;netstandard2.0;netcoreapp2.1</TargetFrameworks>

And please review PR #522 to see the extra unit tests.

@zspitz
Copy link
Author

zspitz commented Jun 24, 2021

@StefH Have you seen my previous comment? I would expect a selector like "np(str)" to return a conditional expression.

@zspitz zspitz changed the title np operator seems to ignore first-level member np operator ignores single member access when no default value passed in Jun 25, 2021
@zspitz
Copy link
Author

zspitz commented Jun 25, 2021

The logic here seems incorrect. Shouldn't it be if (expressions.Count == 0 ...?

@StefH
Copy link
Collaborator

StefH commented Jun 26, 2021

@zspitz
I did write an incorrect unit-test, which I did fix.
And I fixed the logic to generate the expression.

However for non-nullable types, the logic is not 100% correct yet, I think.

public class X
{
  public int Number {get;set;}
}

With

.Select("np(it.Number)")

Generates currently with my fix: "Select(Param_0 => Param_0.Number)", however this should be one of these options:

1
"Select(Param_0 => IIF(((Param_0 != null) AndAlso (Param_0.Number!= null)), Param_0.Number, null))"
I'm not sure if this is correct, because this converts a non-nullable to a nullable.

2
"Select(Param_0 => IIF(((Param_0 != null) AndAlso (Param_0.Number!= null)), Param_0.Number, 0))"
This is correct, because the default value from an integer (which is 0) is returned.

3
Parse exception
It's also possible to throw a parse exception to indicate that the np() cannot be used on non-nullable types, and that a default value should be supplied by the user.

@zspitz
Copy link
Author

zspitz commented Jun 26, 2021

I would suggest in your Number example, it should return: "Select(Param_0 => IIF(Param_0 != null, Param_0.Number, null))". Even though the final result of the conditional is nullable, that's OK; because the result type of the corresponding C# code: x => x != null ? x.Number : null would also be nullable.

@StefH
Copy link
Collaborator

StefH commented Jun 29, 2021

Code is fixed and returns "Select(Param_0 => IIF(Param_0 != null, Param_0.Number, null))" as normal backwards compatible behavior.
However, using a config setting, this can be changed so that the default(int) is returned.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants