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

Clarify error message when using np with instance methods #432

Closed
zspitz opened this issue Oct 20, 2020 · 10 comments
Closed

Clarify error message when using np with instance methods #432

zspitz opened this issue Oct 20, 2020 · 10 comments
Assignees
Labels

Comments

@zspitz
Copy link

zspitz commented Oct 20, 2020

Given the following class:

public class Foo {
    public string? Bar {get; set;}
    public string? Baz() => "";
}

Dynamic LINQ expands the np(...) function:

np(Bar.Length)

to include a null-checking test on every property/field access in the chain:

// C# equivalent
$var0 != null && $var0.Bar != null ? $var0.Bar.Length : null

However, np doesn't work with an instance method call:

np(Baz().Length)

and fails with System.InvalidOperationException: 'Sequence contains no elements'.

Stack trace ```none at System.Linq.ThrowHelper.ThrowNoElementsException() at System.Linq.Dynamic.Core.Parser.ExpressionHelper.CollectExpressions(Boolean addSelf, Expression sourceExpression) at System.Linq.Dynamic.Core.Parser.ExpressionHelper.TryGenerateAndAlsoNotNullExpression(Expression sourceExpression, Boolean addSelf, Expression& generatedExpression) at System.Linq.Dynamic.Core.Parser.ExpressionParser.ParseFunctionNullPropagation() at System.Linq.Dynamic.Core.Parser.ExpressionParser.ParseIdentifier() at System.Linq.Dynamic.Core.Parser.ExpressionParser.ParsePrimaryStart() at System.Linq.Dynamic.Core.Parser.ExpressionParser.ParsePrimary() at System.Linq.Dynamic.Core.Parser.ExpressionParser.ParseUnary() at System.Linq.Dynamic.Core.Parser.ExpressionParser.ParseMultiplicative() at System.Linq.Dynamic.Core.Parser.ExpressionParser.ParseAdditive() at System.Linq.Dynamic.Core.Parser.ExpressionParser.ParseShiftOperator() at System.Linq.Dynamic.Core.Parser.ExpressionParser.ParseComparisonOperator() at System.Linq.Dynamic.Core.Parser.ExpressionParser.ParseLogicalAndOrOperator() at System.Linq.Dynamic.Core.Parser.ExpressionParser.ParseIn() at System.Linq.Dynamic.Core.Parser.ExpressionParser.ParseAndOperator() at System.Linq.Dynamic.Core.Parser.ExpressionParser.ParseOrOperator() at System.Linq.Dynamic.Core.Parser.ExpressionParser.ParseLambdaOperator() at System.Linq.Dynamic.Core.Parser.ExpressionParser.ParseNullCoalescingOperator() at System.Linq.Dynamic.Core.Parser.ExpressionParser.ParseConditionalOperator() at System.Linq.Dynamic.Core.Parser.ExpressionParser.Parse(Type resultType, Boolean createParameterCtor) at _DynamicLINQ.Program.Main(String[] args) in C:\Users\Spitz\source\repos\_testReflection\_DynamicLINQ\Program.cs:line 66 ```

This is understandable, because the translation would require multiple calls to the method, something which should not be done implicitly:

// C# equivalent
$var0 != null && $var0.Bar() != null ? $var0.Bar().Length : null

But the error message doesn't make this clear. Having an error message to the effect that "instance methods cannot be used in an np chain" would be better.

@zspitz zspitz changed the title Proposal: Allow use of np with instance methods Clarify error message when using np with instance methods Oct 20, 2020
@JonathanMagnan JonathanMagnan self-assigned this Oct 24, 2020
@JonathanMagnan
Copy link
Member

Hello @zspitz ,

The v1.2.5 has been released.

The expression was only working when the method has some parameters such as: public string? Baz(int i) => "";

In the newest version, null propagation should also work with a method without parameter like you in case.

Let me know if everything is now working as expected.

Best Regards,

Jon


Performance Libraries
context.BulkInsert(list, options => options.BatchSize = 1000);
Entity Framework ExtensionsEntity Framework ClassicBulk OperationsDapper Plus

Runtime Evaluation
Eval.Execute("x + y", new {x = 1, y = 2}); // return 3
C# Eval FunctionSQL Eval Function

@zspitz
Copy link
Author

zspitz commented Oct 24, 2020

@JonathanMagnan Thank you for the quick turnaround on this.

But the issue is not quite resolved. When I use np on a member chain which has a method call in the middle:

public class Foo {
    public Foo Foo1 { get; set; }
    public string Baz() => "";
}

var selector = "np(Foo1.Baz().Length)";
var prm = Parameter(typeof(Foo));
var parser = new ExpressionParser(new[] { prm }, selector, new object[] { }, ParsingConfig.Default);
var expr = parser.Parse(null));

what expression is supposed to be generated? I would expect to see the equivalent of the following C#:

$var0 != null && $var0.Foo1 != null && $var0.Foo1.Baz() != null ? $var0.Foo1.Baz().Length : null

checking for all the parts of the member/method chain. Instead, I see the following:

$var0.Foo1.Baz() != null ? $var0.Foo1.Baz().Length : null

Anything up the chain from the method call doesn't have a null-test generated.

I'm not sure what the right thing to do here. Personally, as I noted above, I think method calls should be left out entirely from the np processing, and np should be documented this way. But if method calls are being processed, then I would expect np should process both the member chain leading up to the method call's instance, and the member chains of all the arguments to the method call. In other words:

np(Foo1.Baz(Foo1.Foo1.Bar, Foo1.Foo1.Bar).Length)

should become the equivalent of (line breaks added for readability):

$var0 != null && 
$var0.Foo1 != null && 
$var0.Foo1.Foo1 != null && 
$var0.Foo1.Baz(Foo1.Foo1.Bar, Foo1.Foo1.Bar) != null ? 
    $var0.Foo1.Baz(Foo1.Foo1.Bar, Foo1.Foo1.Bar).Length : 
    null

@StefH
Copy link
Collaborator

StefH commented Oct 25, 2020

Hello @zspitz, I see your point. That code np(Foo1.Baz().Length)" is currently translated into:

Param_0 => IIF((Param_0.Foo1.Baz() != null), Convert(Param_0.Foo1.Baz().Length), null)

Which is indeed invalid (I did not test this scenario when implementing the np(...) functionality).

It should be like:

Param_0 => IIF((((Param_0 != null) AndAlso (Param_0.Foo1 != null)) AndAlso (Param_0.Foo1.Baz() != null)), Convert(Param_0.Foo1.Baz().Length), null)

@JonathanMagnan Shall I take a look at the code and see if this can be changed?

@zspitz
Copy link
Author

zspitz commented Oct 25, 2020

@StefH It's more than just the methods which are up the member chain. Once np is supposed to process method calls, I think it's also expected that it should process member chains within arguments passed to the method calls. (And of course, method calls within chains passed as arguments to method calls.)

(I noted this in my previous comment, but your comment didn't respond to this point. Just making sure you've seen it.)

NB. You may be interested in the library I've written, which provides various string representations for expression trees:

Console.WriteLine(expr.ToString("C#"));
/*
    $var0.Foo1.Baz() != null ? $var0.Foo1.Baz().Length : null
*/

Console.WriteLine(expr.ToString("Textual tree", "C#"));
/*
    Conditional (int?)
        · Test - NotEqual (bool)
            · Left - Call (string) Baz
                · Object - MemberAccess (Foo) Foo1
                    · Expression - Parameter (Foo)
            · Right - Constant (object)
        · IfTrue - Convert (int?)
            · Operand - MemberAccess (int) Length
                · Expression - Call (string) Baz
                    · Object - MemberAccess (Foo) Foo1
                        · Expression - Parameter (Foo)
        · IfFalse - Constant (int?)
*/

@JonathanMagnan
Copy link
Member

@StefH , sure you can go ahead and propose a pull

StefH added a commit that referenced this issue Oct 28, 2020
@zspitz
Copy link
Author

zspitz commented Oct 28, 2020

@StefH The unit tests look excellent, thanks. But what happens if the arguments themselves consist of member/method chains?

I realize handling this possibility means increased complexity -- we're no longer talking about a simple chain of members/methods, but rather each method could branch out into its own chain via the arguments.

But if you decide not to process method arguments, I think it important that this limitation should be clearly documented.

@zspitz
Copy link
Author

zspitz commented Oct 28, 2020

@StefH

I cannot use ExpressionTreeToString in .NET 4.5.2 test project.

Beginning with version 3.2, the library now targets .NET 4.5.2 as well. Also, you can now produce Dynamic LINQ equivalents for expressions, as noted here.

@StefH
Copy link
Collaborator

StefH commented Oct 29, 2020

Hello @zspitz,
I see your point on the arguments like np(Foo1.Baz(Foo1.Foo1.Bar, Foo1.Foo1.Bar).Length).

Would it be an option to mark this issue as resolved? And that you create a new issue regarding the arguments issue?

@zspitz
Copy link
Author

zspitz commented Oct 29, 2020

@StefH

Would it be an option to mark this issue as resolved? And that you create a new issue regarding the arguments issue?

Done (#444).

@zspitz zspitz closed this as completed Oct 29, 2020
@StefH StefH added the bug label Nov 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

3 participants