-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Statically bind invocations in presence of dynamic arguments only for local functions #73314
Conversation
… local functions, extension methods or expanded non-array params cases. Related to dotnet#72750. This is an alternative limited approach for QB mode.
… `dynamic` arguments (dotnet#72964)" This reverts compiler changes (tests changes are not reverted) made in commit 5a49045.
This warning looks very strange. I'll open an issue to follow-up. #Closed Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/DynamicTests.cs:10599 in 8c74aca. [](commit_id = 8c74aca, deletion_comment = False) |
@jcouv, @RikkiGibson, @dotnet/roslyn-compiler Please review |
1 similar comment
@jcouv, @RikkiGibson, @dotnet/roslyn-compiler Please review |
Let's refresh dotnet/csharplang#8027 to capture the final design |
I am planning to do that. The change mostly will be as simple as moving the "Dynamic" section into the alternatives section. |
addMethods = filterOutBadGenericMethods(addMethodBinder, syntax, methodGroup, analyzedArguments, resolution, finalApplicableCandidates, ref useSiteInfo); | ||
result = !addMethods.IsEmpty; | ||
|
||
if (!result) | ||
{ | ||
diagnostics.Add(ErrorCode.ERR_CollectionExpressionMissingAdd, syntax, methodGroup.ReceiverOpt.Type); | ||
} | ||
else if (addMethods.Length == 1) |
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.
It's hard to tell whether we're keeping the correspondence with BindMethodGroupInvocation
. What does this correspond to?
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.
It's hard to tell whether we're keeping the correspondence with
BindMethodGroupInvocation
. What does this correspond to?
It corresponds to the code at the end of bindInvocationExpressionContinued
below. We are not going after exact match with BindMethodGroupInvocation
because the argument that we are using is fake and we aways end up here when we have a single applicable candidate. Before the change we were ending up in bindInvocationExpressionContinued
through tryEarlyBindSingleCandidateInvocationWithDynamicArgument
.
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.
LGTM Thanks (iteration 8)
@RikkiGibson, @dotnet/roslyn-compiler For the second review |
@RikkiGibson, @dotnet/roslyn-compiler For the second review |
Taking a look |
Opened #73374 In reply to: 2091415087 Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/DynamicTests.cs:10599 in 8c74aca. [](commit_id = 8c74aca, deletion_comment = False) |
Fixes #72750.
This implements the latest LDM decision.
In order to make sure all artifacts of the previous fix (#72964) were removed, I reverted all implementation (but not test changes) from that PR by using 'git revert`. All cleanups/refactorings that are still relevant were manually ported back.