-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Compare to all Queryable/Enumerable methods via MethodInfo, not by name #16300
Comments
Do after #16339 is merged to avoid conflict. |
Also move LinqMethodHelpers out of NavigationExpansion and consider making public for use by providers. |
@smitpatel holding off on this as discussed, since you want to replace LinqMethodHelpers (otherwise let me know and I'll do this). Regarding big switches like in QueryableMethodTranslatingExpressionVisitor, we could also do something like: case nameof(Queryable.Any) when method == LinqMethodHelpers.QueryableAnyMethodInfo: Giving the best of both worlds - a nice clean switch and a tight reference comparison with a MethodInfo that's obtained once in a central place (also less verbose than checking parameter count and types). |
That seems pretty. We can certainly do that. 👍 |
You want me to do that and tighten method lookup in LinqMethodHelpers or are you working on something on your own side? |
I will complete it. |
blocked label -> Refactoring QueryableMethodProvider in nav expansion take 2. |
I can do that once the PR is merged. |
Thanks @roji |
Isn't this done already? |
In various place, we resolve methods on Queryable/Enumerable via name, and don't verify the exact number and type of parameters, assuming overloads will never be added. We should do a pass to compare against MethodInfo.
We already have LinqMethodHelpers which should contain all commonly-used MethodInfos - this should be used rather than looking up MethodInfos in various places. However, that class currently doesn't lookup in a bulletproof way, checking number of parameters but not types.
Original conversation: #16249 (comment)
The text was updated successfully, but these errors were encountered: