VB: Check overload resolution priority for basic scenarios involving methods #75862

AlekseyTs · 2024-11-11T22:33:22Z

Speclet - dotnet/vblang#627

…methods

AlekseyTs · 2024-11-13T00:05:07Z

/azp run

azure-pipelines · 2024-11-13T00:05:27Z

Azure Pipelines successfully started running 3 pipeline(s).

AlekseyTs · 2024-11-13T14:14:28Z

src/Compilers/VisualBasic/Portable/Semantics/OverloadResolution.vb

+            candidateInfoByDeclaringType.Free()
+        End Sub
+
+        Private Shared Sub ApplyTieBreakingRulesSkippedByCombineCandidates(


ApplyTieBreakingRulesSkippedByCombineCandidates

Effect of changes to CombineCandidates with this fixup was tested across all unit-tests that we have right now - #75792.

…riority' into OverloadPriority_02

AlekseyTs · 2024-11-13T19:22:37Z

@333fred, @cston, @dotnet/roslyn-compiler Please review

AlekseyTs · 2024-11-14T15:53:15Z

@333fred, @cston, @dotnet/roslyn-compiler Please review

333fred

Done review pass

src/Compilers/VisualBasic/Test/Semantic/Semantics/OverloadResolutionPriorityTests.vb

src/Compilers/VisualBasic/Portable/Semantics/OverloadResolution.vb

AlekseyTs · 2024-11-14T19:45:42Z

/azp run

azure-pipelines · 2024-11-14T19:46:03Z

Azure Pipelines successfully started running 3 pipeline(s).

AlekseyTs · 2024-11-14T19:47:18Z

@cston, @dotnet/roslyn-compiler For the second review

AlekseyTs · 2024-11-15T14:24:04Z

@cston, @dotnet/roslyn-compiler For the second review

cston · 2024-11-15T18:25:18Z

src/Compilers/VisualBasic/Portable/Semantics/OverloadResolution.vb

+
+            Public Overrides ReadOnly Property OverloadResolutionPriority As Integer
+                Get
+                    Return m_Method.OriginalDefinition.OverloadResolutionPriority


OriginalDefinition

Are we testing cases that require OriginalDefinition in OverloadResolutionPriority or GetOverloadResolutionPriorityInfo()? #Resolved

This is not required for correctness. The effect is not observable, other than us doing less work.

cston · 2024-11-15T18:25:28Z

src/Compilers/VisualBasic/Portable/Semantics/OverloadResolution.vb

+
+            Public Overrides ReadOnly Property OverloadResolutionPriority As Integer
+                Get
+                    Return _property.OriginalDefinition.OverloadResolutionPriority


OriginalDefinition

Are we testing cases that require OriginalDefinition in OverloadResolutionPriority or GetOverloadResolutionPriorityInfo()? #Resolved

This is not required for correctness. The effect is not observable, other than us doing less work.

cston · 2024-11-15T18:43:13Z

src/Compilers/VisualBasic/Portable/Semantics/OverloadResolution.vb

@@ -1187,6 +1275,254 @@ ResolutionComplete:
            Return New OverloadResolutionResult(candidates.ToImmutable(), resolutionIsLateBound, narrowingCandidatesRemainInTheSet, asyncLambdaSubToFunctionMismatch)
        End Function

+        Private Shared ReadOnly s_poolInstance As ObjectPool(Of PooledDictionary(Of NamedTypeSymbol, NamedTypeSymbol)) =


s_poolInstance

Is this used? #Resolved

Is this used?

Thanks for catching this. It is meant to be used for candidateInfoByDeclaringType in the method below.

This wouldn't affect the result though since we are comparing definitions

Addressed in #75941

cston · 2024-11-15T19:29:27Z

src/Compilers/VisualBasic/Portable/Semantics/OverloadResolution.vb

@@ -4080,15 +4400,17 @@ Bailout:
                    '       This rule also applies to the types that extension methods are defined on.
                    '7.2.	If M and N are extension methods and the target type of M is a class or
                    '       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


What effect does this OrElse clause have, and are we testing it? #Resolved

What effect does this OrElse clause 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.

It looks like EarlyFilteringOnReceiverType_01 still passes if the entire OrElse ... clause is removed.

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.

As we discussed, I was referring to the entire OrElse (signatureMatch AndAlso Not (...)).

Inspired by a test in dotnet#75862.

cston · 2024-11-15T20:13:08Z

src/Compilers/VisualBasic/Portable/Symbols/Metadata/PE/PEPropertySymbol.vb

@@ -45,6 +46,7 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.Symbols.Metadata.PE
        Private _lazyObsoleteAttributeData As ObsoleteAttributeData = ObsoleteAttributeData.Uninitialized

        Private _lazyIsRequired As ThreeState = ThreeState.Unknown
+        Private _lazyOverloadResolutionPriority As StrongBox(Of Integer)


StrongBox

Should we add an UncommonFields class to hold most of the _lazy* fields? #Resolved

I don't think we have to, and I decided against doing this work under umbrella of this feature.

Inspired by a test in #75862.

cston · 2024-11-16T00:47:52Z

src/Compilers/VisualBasic/Test/Semantic/Semantics/OverloadResolutionPriorityTests.vb

+        End Sub
+
+        <Fact>
+        Public Sub DefaultProperty_01()


DefaultProperty_01

Is this test affected by the compiler change? Should Main() contain a call to the default property? #Resolved

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

cston · 2024-11-16T00:48:51Z

src/Compilers/VisualBasic/Test/Semantic/Semantics/OverloadResolutionPriorityTests.vb

+        End Sub
+
+        <Fact>
+        Public Sub DefaultProperty_02()


DefaultProperty_02

Is this test affected by the compiler change? #Resolved

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

VB: Check overload resolution priority for basic scenarios involving …

79b6674

…methods

AlekseyTs added Area-Compilers Feature - Overload Resolution Priority labels Nov 11, 2024

AlekseyTs requested a review from 333fred November 11, 2024 22:33

AlekseyTs requested a review from a team as a code owner November 11, 2024 22:33

dotnet-issue-labeler bot added the untriaged Issues and PRs which have not yet been triaged by a lead label Nov 11, 2024

build-analysis bot mentioned this pull request Nov 12, 2024

The active test run was aborted. Reason: Test host process crashed dotnet/dnceng#451

Open

3 tasks

jaredpar added this to the 17.13 milestone Nov 13, 2024

jaredpar assigned 333fred and cston Nov 13, 2024

jaredpar removed the untriaged Issues and PRs which have not yet been triaged by a lead label Nov 13, 2024

AlekseyTs requested a review from cston November 13, 2024 14:06

AlekseyTs commented Nov 13, 2024

View reviewed changes

AlekseyTs added 4 commits November 13, 2024 06:50

Merge remote-tracking branch 'upstream/features/VBOverloadResolutionP…

3b09547

…riority' into OverloadPriority_02

Flesh out handling of narrowing candidates

dcb398a

Merge branch 'OverloadPriority_01' into OverloadPriority_02

e9c4d4f

Optimize loading from PE

63041c9

arunchndr modified the milestones: 17.13, 17.13 P2 Nov 13, 2024

AlekseyTs added 3 commits November 13, 2024 12:24

Merge branch 'OverloadPriority_01' into OverloadPriority_02

d57511e

Handle properties

1c7878f

Break cycle in attributes

30f443b

333fred reviewed Nov 14, 2024

View reviewed changes

333fred approved these changes Nov 14, 2024

View reviewed changes

cston reviewed Nov 15, 2024

View reviewed changes

333fred mentioned this pull request Nov 15, 2024

Add params vs normal form test #75937

Merged

333fred added a commit to 333fred/roslyn that referenced this pull request Nov 15, 2024

Add params vs normal form test

7e8fb18

Inspired by a test in dotnet#75862.

cston reviewed Nov 15, 2024

View reviewed changes

333fred added a commit that referenced this pull request Nov 15, 2024

Add params vs normal form test (#75937)

75e5769

Inspired by a test in #75862.

cston reviewed Nov 16, 2024

View reviewed changes

cston approved these changes Nov 16, 2024

View reviewed changes

AlekseyTs merged commit b5f6a24 into dotnet:features/VBOverloadResolutionPriority Nov 16, 2024
28 of 31 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

VB: Check overload resolution priority for basic scenarios involving methods #75862

VB: Check overload resolution priority for basic scenarios involving methods #75862

AlekseyTs commented Nov 11, 2024 •

edited

Loading

AlekseyTs commented Nov 13, 2024

azure-pipelines bot commented Nov 13, 2024

AlekseyTs Nov 13, 2024

AlekseyTs commented Nov 13, 2024

AlekseyTs commented Nov 14, 2024

333fred left a comment

AlekseyTs commented Nov 14, 2024

azure-pipelines bot commented Nov 14, 2024

AlekseyTs commented Nov 14, 2024

AlekseyTs commented Nov 15, 2024

cston Nov 15, 2024 •

edited by AlekseyTs

Loading

AlekseyTs Nov 15, 2024 •

edited

Loading

cston Nov 15, 2024 •

edited by AlekseyTs

Loading

AlekseyTs Nov 15, 2024

cston Nov 15, 2024 •

edited by AlekseyTs

Loading

AlekseyTs Nov 15, 2024

AlekseyTs Nov 15, 2024

AlekseyTs Nov 16, 2024

cston Nov 15, 2024 •

edited by AlekseyTs

Loading

AlekseyTs Nov 15, 2024

cston Nov 15, 2024 •

edited

Loading

AlekseyTs Nov 15, 2024 •

edited

Loading

cston Nov 15, 2024

cston Nov 15, 2024 •

edited by AlekseyTs

Loading

AlekseyTs Nov 15, 2024

cston Nov 16, 2024 •

edited by AlekseyTs

Loading

AlekseyTs Nov 16, 2024

cston Nov 16, 2024 •

edited by AlekseyTs

Loading

AlekseyTs Nov 16, 2024

VB: Check overload resolution priority for basic scenarios involving methods #75862

VB: Check overload resolution priority for basic scenarios involving methods #75862

Conversation

AlekseyTs commented Nov 11, 2024 • edited Loading

AlekseyTs commented Nov 13, 2024

azure-pipelines bot commented Nov 13, 2024

Choose a reason for hiding this comment

AlekseyTs commented Nov 13, 2024

AlekseyTs commented Nov 14, 2024

333fred left a comment

Choose a reason for hiding this comment

AlekseyTs commented Nov 14, 2024

azure-pipelines bot commented Nov 14, 2024

AlekseyTs commented Nov 14, 2024

AlekseyTs commented Nov 15, 2024

cston Nov 15, 2024 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

AlekseyTs Nov 15, 2024 • edited Loading

Choose a reason for hiding this comment

cston Nov 15, 2024 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston Nov 15, 2024 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston Nov 15, 2024 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston Nov 15, 2024 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Nov 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston Nov 15, 2024 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston Nov 16, 2024 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston Nov 16, 2024 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs commented Nov 11, 2024 •

edited

Loading

cston Nov 15, 2024 •

edited by AlekseyTs

Loading

AlekseyTs Nov 15, 2024 •

edited

Loading

cston Nov 15, 2024 •

edited by AlekseyTs

Loading

cston Nov 15, 2024 •

edited by AlekseyTs

Loading

cston Nov 15, 2024 •

edited by AlekseyTs

Loading

cston Nov 15, 2024 •

edited

Loading

AlekseyTs Nov 15, 2024 •

edited

Loading

cston Nov 15, 2024 •

edited by AlekseyTs

Loading

cston Nov 16, 2024 •

edited by AlekseyTs

Loading

cston Nov 16, 2024 •

edited by AlekseyTs

Loading