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

Fix/parse expression with expected types #190

Closed
wants to merge 6 commits into from

Conversation

davideicardi
Copy link
Member

@davideicardi davideicardi commented Nov 20, 2021

WARNING: This PR is still in progress and doesn't yet compile...

In this PR I want to try to change how arguments inside expressions are parsed. Before we first parse the arguments and then we try to search for a method that match the arguments. The problem of this approach is that in some cases we cannot parse the arguments without knowing what is the expected type.
For example in a case like:

list.Where(p => p.SomeMethod())

The paramenter p => p.SomeMethod() cannot be parsed without knowing the type of the list variable and the Where method signature.

The idea is that to instead check if each method is acceptable by parsing the arguments with the expected type, so for example in the above case parse the p => p.SomeMethod() knowin that we expect a Func<TSource,bool> predicate as a first argument ...

This change require to invert some logic and to pass within all the parse stack the expected return type (see expectedReturnType).
Also from now on I want to use typeof(Object) whenever we don't know the type instead of using typeof(void).

In particular I have changed these methods: ParseLambdaInvocation, ParseDelegateInvocation and ParseMethodGroupInvocation.

PrepareDelegateInvoke has been deleted and replaced with FindInvokeMethod.

The goal is to also fix #185 .

@metoule
Copy link
Contributor

metoule commented Nov 24, 2021

I agree that the type inference for generic methods can be greatly improved! I like that you got rid of PrepareDelegateInvoke(Type type, ref Expression[] args): the fact that it changed the arguments list made it really difficult to test different candidates, which I encountered in ParseMethodGroupInvocation.

For reference, here's the C# specification on type inference : https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/language-specification/expressions#type-inference

From what I read, I don't think it's necessary to pass the expected return type all the way down the stack, but instead try to infer the return type of the lambda expression (instead of forcing it from the method parameter type), and let the MethodHasPriority ensure that it's compatible with the caller.

@davideicardi
Copy link
Member Author

@metoule Thanks for your feedback.
Probably you are right. It is not necessary to pass the type in the whole stack ... but in that case I'm not sure how we can fix #185.

Probably for now it is better to abbandon this PR and just port the PrepareDelegateInvoke inside #186? And until we don't have a better solution use that one.

What do you think?

@metoule
Copy link
Contributor

metoule commented Nov 25, 2021

#186 should fix #185, even though it's not particularly elegant. I'll keep working on the type inference for a lambda expression return type because it's not really satisfying. In the meantime, porting PrepareDelegateInvoke is a good idea! I'll do that right away.

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

Successfully merging this pull request may close these issues.

Bug when trying to upcast a delegate return type
2 participants