VB: Check overload resolution priority for basic scenarios involving methods#75862
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
| candidateInfoByDeclaringType.Free() | ||
| End Sub | ||
|
|
||
| Private Shared Sub ApplyTieBreakingRulesSkippedByCombineCandidates( |
There was a problem hiding this comment.
Effect of changes to CombineCandidates with this fixup was tested across all unit-tests that we have right now - #75792.
src/Compilers/VisualBasic/Test/Semantic/Semantics/OverloadResolutionPriorityTests.vb
Show resolved
Hide resolved
src/Compilers/VisualBasic/Test/Semantic/Semantics/OverloadResolutionPriorityTests.vb
Show resolved
Hide resolved
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
@cston, @dotnet/roslyn-compiler For the second review |
1 similar comment
|
@cston, @dotnet/roslyn-compiler For the second review |
|
|
||
| Public Overrides ReadOnly Property OverloadResolutionPriority As Integer | ||
| Get | ||
| Return m_Method.OriginalDefinition.OverloadResolutionPriority |
There was a problem hiding this comment.
This is not required for correctness. The effect is not observable, other than us doing less work.
|
|
||
| Public Overrides ReadOnly Property OverloadResolutionPriority As Integer | ||
| Get | ||
| Return _property.OriginalDefinition.OverloadResolutionPriority |
There was a problem hiding this comment.
This is not required for correctness. The effect is not observable, other than us doing less work.
| Return New OverloadResolutionResult(candidates.ToImmutable(), resolutionIsLateBound, narrowingCandidatesRemainInTheSet, asyncLambdaSubToFunctionMismatch) | ||
| End Function | ||
|
|
||
| Private Shared ReadOnly s_poolInstance As ObjectPool(Of PooledDictionary(Of NamedTypeSymbol, NamedTypeSymbol)) = |
There was a problem hiding this comment.
Is this used?
Thanks for catching this. It is meant to be used for candidateInfoByDeclaringType in the method below.
There was a problem hiding this comment.
This wouldn't affect the result though since we are comparing definitions
| ' structure and the target type of N is an interface, eliminate N from the set. | ||
| If ShadowBasedOnReceiverType(existingCandidate, newCandidate, existingWins, newWins, useSiteInfo) Then | ||
| If (Not someCandidatesHaveOverloadResolutionPriority OrElse | ||
| (signatureMatch AndAlso Not (existingCandidate.Candidate.IsExtensionMethod OrElse newCandidate.Candidate.IsExtensionMethod))) AndAlso |
There was a problem hiding this comment.
What effect does this OrElse clause have, and are we testing it? #Resolved
There was a problem hiding this comment.
What effect does this
OrElseclause have, and are we testing it?
EarlyFilteringOnReceiverType_01 fails if we remove it. The meaning of it is pretty much similar to what the comment before the previous If says. I.e. priority filtering for extension methods must come before filtering by receiver. Because for extension methods the filtering by receiver is not about shadowing by signature between methods declared in base and derived types.
There was a problem hiding this comment.
It looks like EarlyFilteringOnReceiverType_01 still passes if the entire OrElse ... clause is removed.
There was a problem hiding this comment.
I am not sure what you did, but when I remove AndAlso Not (existingCandidate.Candidate.IsExtensionMethod OrElse newCandidate.Candidate.IsExtensionMethod) part, the test fails for me. Perhaps that should be taken offline.
There was a problem hiding this comment.
As we discussed, I was referring to the entire OrElse (signatureMatch AndAlso Not (...)).
Inspired by a test in dotnet#75862.
| Private _lazyObsoleteAttributeData As ObsoleteAttributeData = ObsoleteAttributeData.Uninitialized | ||
|
|
||
| Private _lazyIsRequired As ThreeState = ThreeState.Unknown | ||
| Private _lazyOverloadResolutionPriority As StrongBox(Of Integer) |
There was a problem hiding this comment.
I don't think we have to, and I decided against doing this work under umbrella of this feature.
| End Sub | ||
|
|
||
| <Fact> | ||
| Public Sub DefaultProperty_01() |
There was a problem hiding this comment.
Is this test affected by the compiler change?
No. But the test demonstrates that there are no interesting scenarios when parameter-less property is marked default
Should
Main()contain a call to the default property?
I do not find it interesting, given the declaration error
| End Sub | ||
|
|
||
| <Fact> | ||
| Public Sub DefaultProperty_02() |
There was a problem hiding this comment.
Is this test affected by the compiler change?
No. But the test demonstrates that there are no interesting scenarios when a parameter-less property is overloaded with default property
Speclet - dotnet/vblang#627