-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Improve order of returned items when overload resolution fails #81572
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
base: main
Are you sure you want to change the base?
Conversation
|
@dotnet/roslyn-compiler ptal. |
| methodBuilder.Free(); | ||
| return filteredMethodBuilder.ToOneOrManyAndFree(); | ||
|
|
||
| static bool argumentTypesMatchParameterTypes(MethodSymbol method, ImmutableArray<BoundExpression> arguments) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dotnet/roslyn-compiler is there some existing function that would help with this. For example, i could imagine taking advantage of refkind/optional-argument info.
This was the simplest approach given that i don'tk now what is available in the rest of the compiler. But if there is something i could make use of, that would be nice.
Basically, a sort of light-weight overload resolution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jcouv i think you wrote such a thing for the IDE for sig-help. Do you know if compiler has anything like that? Note: the goal is not to be perfect. Just to help order things
| Assert.Equal("IApplicationBuilder IApplicationBuilder.Use(System.Func<RequestDelegate, RequestDelegate> middleware)", symbolInfo1.CandidateSymbols[0].ToTestDisplayString()); | ||
| Assert.Equal("IApplicationBuilder IApplicationBuilder.Use(System.Func<HttpContext, System.Func<System.Threading.Tasks.Task>, System.Threading.Tasks.Task> middleware)", symbolInfo1.CandidateSymbols[1].ToTestDisplayString()); | ||
| Assert.Equal("IApplicationBuilder IApplicationBuilder.Use(System.Func<HttpContext, System.Func<System.Threading.Tasks.Task>, System.Threading.Tasks.Task> middleware)", symbolInfo1.CandidateSymbols[0].ToTestDisplayString()); | ||
| Assert.Equal("IApplicationBuilder IApplicationBuilder.Use(System.Func<RequestDelegate, RequestDelegate> middleware)", symbolInfo1.CandidateSymbols[1].ToTestDisplayString()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these results changed order. the new order is better and reflects that the Use method was passed an async lambda (which then should be returning a Task). Same with the test below.
|
Note: we do not need to take this PR. I worked around this on the IDE side by reorderign the results. But if tehre is interest, i'll keep this open. |
| } | ||
| private static Func<JToken, object> CreateDeserializeDelegate(JToken key) | ||
| private static object CreateDeserializeDelegate(JToken key) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the PREFERED result. The code in question is: _deserializeHelpers.GetOrAdd(type, key => [|CreateDeserializeDelegate|](key));
So since we know we're padding a type we know the second argument is a Func<JToken, object>, which makes key => a JToken. And since we need a Func<JToken, object> that means we want the return type of CreateDeserializeDelegate to be object to properly match that delegate sig.
|
the challenge doing this at IDE layer is that it means every callsite in the IDE that goes through GetSymbolInfo now needs to go through the GetSymbolInfoButApplyHeuristicsToMakeupForLackOfOrderingInCompilerResult. |
Fixes #78533
Sorts the candidate symbols returned by GetSymbolInfo in overload resolution error cases, prioritizing candidates that matched better