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

Consider relaxing ExpressionParser validation of method call return type #44

Closed
glopesdev opened this issue Feb 15, 2016 · 2 comments

Comments

@glopesdev
Copy link
Contributor

commented Feb 15, 2016

Currently ExpressionParser disallows method calls that return void. There is really no reason to do this, and it prevents ExpressionParser from trivially supporting the parsing of Action<> delegates.

The final Expression.Lambda call would already throw in cases where the method is not compatible with the specified delegate type.

Also, the current guard throws a ParseException, which is not strictly true: parsing is fine, the semantic model only fails to bind when the final Lambda constructor is called. Semantic binding should be left for Expression calls, or at most the guard should be moved to check only the default ParseLambda overloads.

Proposed fix:
Remove the following 3 lines starting at https://github.com/kahanu/System.Linq.Dynamic/blob/master/Src/System.Linq.Dynamic/DynamicLinq.cs#L1445

                        if (method.ReturnType == typeof(void))
                            throw ParseError(errorPos, Res.MethodIsVoid,
                                id, GetTypeName(method.DeclaringType));
@ryanvgates

This comment has been minimized.

Copy link
Collaborator

commented Feb 16, 2016

Where is the code for this?

Currently ExpressionParser disallows method calls that return void.

@glopesdev

This comment has been minimized.

Copy link
Contributor Author

commented Feb 17, 2016

Sorry if I wasn't clear. The link to the offending code was provided above:
https://github.com/kahanu/System.Linq.Dynamic/blob/master/Src/System.Linq.Dynamic/DynamicLinq.cs#L1445

If you open the link, it should open the GitHub code browser automatically positioned at line 1445, where the redundant check is.

This for me is the most pressing of these last 3 issues to be fixed, as it poses serious limitations to the use of DynamicExpression with no clear gain. I will try to submit the other pull requests soon.

@ryanvgates ryanvgates closed this in 6ee95c6 Mar 8, 2016
ryanvgates added a commit that referenced this issue Mar 8, 2016
Fixed #44. Removed unnecessary return type check from member method access.

Unit test and code change look good.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.