VB: Recognize and check an `unmanaged` constraint #75665

AlekseyTs · 2024-10-29T13:39:22Z

No description provided.

AlekseyTs · 2024-10-29T17:25:03Z

@dotnet/roslyn-compiler Please review

AlekseyTs · 2024-10-30T12:48:37Z

@dotnet/roslyn-compiler Please review

jaredpar · 2024-10-30T16:22:27Z

@333fred, @cston PTAL

cston · 2024-10-31T04:49:32Z

src/Compilers/Core/Portable/Binding/UseSiteInfo.cs

+            if (_diagnostics.Add(diagnosticInfo) && diagnosticInfo?.Severity == DiagnosticSeverity.Error)
+            {
+                RecordPresenceOfAnError();
+            }


This is repeated several times. Consider extracting a helper method. #Resolved

cston · 2024-10-31T04:56:48Z

src/Compilers/Core/Portable/Symbols/INamedTypeSymbolInternal.cs

+        /// <summary>
+        /// True if this type or some containing type has type parameters.
+        /// </summary>
+        public bool IsGenericType { get; }


public

Is the public qualifier needed? #Resolved

cston · 2024-10-31T05:02:59Z

src/Compilers/Core/Portable/Symbols/INamedTypeSymbolInternal.cs

+            /// without looking at its fields and Unknown otherwise.
+            /// Also returns whether or not the given type is generic.
+            /// </summary>
+            public static (ThreeState isManaged, bool hasGenerics) IsManagedTypeHelper(INamedTypeSymbolInternal type)


IsManagedTypeHelper

Consider use a top-level class and make this an extension method. #Resolved

Consider use a top-level class and make this an extension method.

I prefer to keep the code as is. I do not foresee us using this helper much in the future. Therefore, in my opinion there is no good reason to "pollute" name lookup binding space with it. If we could use Default Interface Implementations feature in our code, I would make this method a member of INamedTypeSymbolInternal.

cston · 2024-10-31T05:39:21Z

src/Compilers/VisualBasic/Portable/Symbols/NamedTypeSymbol.vb

+        End Function
+
+        ' <summary>
+        ' IsManagedType is simple for most named types:


IsManagedType

GetManagedKind()? #Resolved

cston · 2024-10-31T05:45:46Z

src/Compilers/VisualBasic/Portable/Symbols/NamedTypeSymbol.vb

+                    End If
+
+                    useSiteInfo.Add(field.GetUseSiteInfo())
+                    Dim fieldType As TypeSymbol = field.Type.GetTupleUnderlyingTypeOrSelf()


GetTupleUnderlyingTypeOrSelf()

Are we testing a case where this call makes a difference? #Resolved

Are we testing a case where this call makes a difference?

I cannot think of a situation when it would make an observable difference in behavior. That is not to say that that it is impossible to run into one in some edge error scenario. Given the difference in the way C# and VB compilers represent tuple symbols, I prefer to be conservative and explicitly unwrap tuple types.

cston · 2024-10-31T05:57:05Z

src/Compilers/VisualBasic/Portable/Symbols/TypeSymbolExtensions.vb

+            Dim namedTypeSymbol = TryCast(typeSymbol, NamedTypeSymbol)
+            Return namedTypeSymbol IsNot Nothing AndAlso
+                namedTypeSymbol.Name = "UnmanagedType" AndAlso
+                namedTypeSymbol.Arity = 0 AndAlso


namedTypeSymbol.Arity = 0 AndAlso

It looks like the C# version and related C# extension methods are not checking Arity == 0. Should we add that check to those methods? #Resolved

Indeed, added a test and opened an issue to follow up - #75681

cston · 2024-10-31T15:25:59Z

    Private Function CreateBinderForTypeDeclaration() As Binder

What does "ForTypeDeclaration" mean here? (It looks like the resulting binder is more specific than for the containing type.) Should this method be named "ForMemberDeclaration" instead?

And would it make sense to pass the BindingLocation in as an optional argument to allow this method to be used in ComputeType as well? #Resolved

Refers to: src/Compilers/VisualBasic/Portable/Symbols/Source/SourceEventSymbol.vb:741 in a55f298. [](commit_id = a55f298, deletion_comment = False)

cston · 2024-10-31T15:27:03Z

    Private Function CreateBinderForTypeDeclaration() As Binder

Same questions as in SourceEventSymbol:

Should this method be named "ForMemberDeclaration" instead?

And would it make sense to pass the BindingLocation in as an optional argument to allow this method to be used in ComputeType as well? #Resolved

Refers to: src/Compilers/VisualBasic/Portable/Symbols/Source/SourcePropertySymbol.vb:1020 in a55f298. [](commit_id = a55f298, deletion_comment = False)

AlekseyTs · 2024-10-31T15:48:41Z

    Private Function CreateBinderForTypeDeclaration() As Binder

What does "ForTypeDeclaration" mean here? (It looks like the resulting binder is more specific than for the containing type.) Should this method be named "ForMemberDeclaration" instead?

And would it make sense to pass the BindingLocation in as an optional argument to allow this method to be used in ComputeType as well?

This is an existing method that new code is not using. I do not intend to rename it or change its behavior.

In reply to: 2450165726

Refers to: src/Compilers/VisualBasic/Portable/Symbols/Source/SourceEventSymbol.vb:741 in a55f298. [](commit_id = a55f298, deletion_comment = False)

AlekseyTs · 2024-10-31T15:49:22Z

    Private Function CreateBinderForTypeDeclaration() As Binder

Same response

In reply to: 2450168231

Refers to: src/Compilers/VisualBasic/Portable/Symbols/Source/SourcePropertySymbol.vb:1020 in a55f298. [](commit_id = a55f298, deletion_comment = False)

cston · 2024-10-31T16:27:01Z

src/Compilers/VisualBasic/Test/Symbol/SymbolsTests/UnmanagedTypeConstraintTests.vb

+        o.M(New Program())
+          ~
+                                                ]]></expected>)
+        End Sub


End Sub

Are we testing type parameters?

Sub M1(Of T1, T2 As Class, T3 As Structure)(t1 as T1, t2 As T2, t3 As T3) Dim o As New Test o.M(t1) o.M(t2) o.M(t3) End Sub ``` #Resolved

Are we testing type parameters?

Type parameters are covered in UnmanagedCheck_TypeParameter and indirectly on several other tests.

cston · 2024-10-31T16:33:58Z

src/Compilers/VisualBasic/Test/Symbol/SymbolsTests/UnmanagedTypeConstraintTests.vb

+            Dim s5 = compilation.GetTypeByMetadataName("S5`1")
+            Assert.Equal(ManagedKind.Managed, s1.GetManagedKind(Nothing))
+            Assert.Equal(ManagedKind.Unmanaged, s2.GetManagedKind(Nothing))
+            Assert.Equal(ManagedKind.UnmanagedWithGenerics, s5.GetManagedKind(Nothing))


Assert.Equal

Check S3 and S4 as well? #Resolved

AlekseyTs · 2024-10-31T20:56:07Z

@333fred, @dotnet/roslyn-compiler For the second review.

AlekseyTs · 2024-11-01T15:29:41Z

@333fred, @dotnet/roslyn-compiler For the second review.

333fred

Generally lgtm. Couple of small comments.

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

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

333fred · 2024-11-01T17:29:50Z

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

+        Friend Overrides ReadOnly Property HasUnmanagedTypeConstraint As Boolean
+            Get
+                EnsureAllConstraintsAreResolved()
+                Return CType(Volatile.Read(_lazyHasIsUnmanagedConstraint), ThreeState).Value()


I don't think we actually need the Volatile.Read; the memory barrier from ImmutableInterlocked.InterlockedInitialize in ResolveConstraints should prevent writes to _lazyHasIsUnmanagedConstraint from being reordered after writes to _lazyConstraintType, and EnsureAllConstaintsAreResolved won't skip calling ResolveConstraints unless this thread has already observed that _lazyConstraintType has been initialized. And if the ordering in that method ever changes (ie, the write to _lazyConstraintType is moved before the write to _lazyHasIsUnmanagedConstraint), then this Volatile.Read won't actually help anyways. #Resolved

I prefer to keep the Volatile.Read. The "distance" between this operation and the interlocked exchange is several method calls and some of them are virtual calls. In one of the methods we check value of _lazyConstraintTypes without any memory barrier operation in the same body. Assuming that that read and this _lazyHasIsUnmanagedConstraint can be reordered, we might see the old value of _lazyHasIsUnmanagedConstraint here while the _lazyConstraintTypes check in EnsureAllConstraintsAreResolved sees the new value of _lazyConstraintTypes. If that is too paranoid, that is fine with me, Volatile.Read won't make things worse.

Assuming that that read and this _lazyHasIsUnmanagedConstraint can be reordered, we might see the old value of _lazyHasIsUnmanagedConstraint here while the _lazyConstraintTypes check in EnsureAllConstraintsAreResolved sees the new value of _lazyConstraintTypes.

The issue with this reasoning is that that is not what Volatile.Read does. Volatile.Read prevents any reads or writes after the read from being ordered before this read. It does not prevent reads that occur before this read from being reordered to after this read. If that is your concern, you would need Thread.MemoryBarrier() in between the method call and the read of _lazyHasIsUnmanagedConstraint.

I remain unconvinced. The documentation says: "Reads the value of the specified field." That is exactly what I am going after. The call reads the specified address. I am not concerned about what happens before or after the call. I am concerned about that specific read happening exactly when the call is executed. Not before the call, not after the call.

Alright, after some extensive offline conversation here, I've opened #75759 to follow up on the race condition here; given that it's present in C# version as well, and has been present for a while, I'm going to approve for now, but we'll be following up on this next week to discuss a better long-term approach for the compiler.

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

src/Compilers/VisualBasic/Portable/Symbols/NamedTypeSymbol.vb

jcouv · 2024-11-05T23:44:56Z

src/Compilers/VisualBasic/Portable/LanguageVersion.vb

@@ -21,6 +21,7 @@ Namespace Microsoft.CodeAnalysis.VisualBasic
        VisualBasic15_5 = 1505
        VisualBasic16 = 1600
        VisualBasic16_9 = 1609
+        VisualBasic17_13 = 1713


Please add the new language version and feature in:

https://github.com/dotnet/vblang/blob/main/Language-Version-History.md

https://github.com/dotnet/roslyn/blob/main/docs/Language%20Feature%20Status.md

Is there an explanation as to what this enables VB to do?

VB: Recognize and check an unmanaged constraint

a55f298

AlekseyTs added Language-VB Area-Compilers labels Oct 29, 2024

AlekseyTs requested review from a team as code owners October 29, 2024 13:39

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

dotnet-policy-service bot added the Needs API Review Needs to be reviewed by the API review council label Oct 29, 2024

jaredpar added this to the 17.13 milestone Oct 29, 2024

jaredpar assigned jaredpar, 333fred and cston and unassigned jaredpar Oct 30, 2024

AlekseyTs requested review from 333fred and cston October 30, 2024 22:34

cston reviewed Oct 31, 2024

View reviewed changes

Merge remote-tracking branch 'upstream/main' into VBUnmanaged

4e488be

PR feedback

cbf6e39

cston reviewed Oct 31, 2024

View reviewed changes

cston approved these changes Oct 31, 2024

View reviewed changes

Adjust tests

c18e059

333fred reviewed Nov 1, 2024

View reviewed changes

PR feedback

57147d9

AlekseyTs requested a review from 333fred November 1, 2024 21:34

333fred approved these changes Nov 5, 2024

View reviewed changes

cston approved these changes Nov 5, 2024

View reviewed changes

AlekseyTs merged commit f97c50e into dotnet:main Nov 5, 2024
28 checks passed

dotnet-policy-service bot modified the milestones: 17.13, Next Nov 5, 2024

jcouv reviewed Nov 5, 2024

View reviewed changes

This was referenced Nov 6, 2024

[Automated] PRs inserted in VS build main-35506.11 #75784

Closed

[Automated] PRs inserted in VS build feature.debugger.main-35507.203 #75826

Closed

dotnet-bot mentioned this pull request Nov 19, 2024

[Automated] PRs inserted in VS build feature.debugger.shadowDebug-35518.109 #75965

Closed

AnthonyDGreen mentioned this pull request Nov 23, 2024

Implementing interfaces with static members dotnet/vblang#625

Open

jjonescz modified the milestones: Next, 17.13 P2 Nov 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

VB: Recognize and check an `unmanaged` constraint #75665

VB: Recognize and check an `unmanaged` constraint #75665

AlekseyTs commented Oct 29, 2024

AlekseyTs commented Oct 29, 2024

AlekseyTs commented Oct 30, 2024

jaredpar commented Oct 30, 2024

cston Oct 31, 2024 •

edited by AlekseyTs

Loading

cston Oct 31, 2024 •

edited by AlekseyTs

Loading

cston Oct 31, 2024 •

edited by AlekseyTs

Loading

AlekseyTs Oct 31, 2024

cston Oct 31, 2024 •

edited by AlekseyTs

Loading

cston Oct 31, 2024 •

edited by AlekseyTs

Loading

AlekseyTs Oct 31, 2024

cston Oct 31, 2024 •

edited by AlekseyTs

Loading

AlekseyTs Oct 31, 2024

cston commented Oct 31, 2024 •

edited by AlekseyTs

Loading

cston commented Oct 31, 2024 •

edited by AlekseyTs

Loading

AlekseyTs commented Oct 31, 2024

AlekseyTs commented Oct 31, 2024

cston Oct 31, 2024 •

edited by AlekseyTs

Loading

AlekseyTs Oct 31, 2024

cston Oct 31, 2024 •

edited by AlekseyTs

Loading

AlekseyTs commented Oct 31, 2024

AlekseyTs commented Nov 1, 2024

333fred left a comment

333fred Nov 1, 2024 •

edited by AlekseyTs

Loading

AlekseyTs Nov 1, 2024

333fred Nov 1, 2024

AlekseyTs Nov 1, 2024 •

edited

Loading

333fred Nov 5, 2024

jcouv Nov 5, 2024

paul1956 Nov 6, 2024

VB: Recognize and check an unmanaged constraint #75665

VB: Recognize and check an unmanaged constraint #75665

Conversation

AlekseyTs commented Oct 29, 2024

AlekseyTs commented Oct 29, 2024

AlekseyTs commented Oct 30, 2024

jaredpar commented Oct 30, 2024

cston Oct 31, 2024 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

cston Oct 31, 2024 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

cston Oct 31, 2024 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston Oct 31, 2024 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

cston Oct 31, 2024 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston Oct 31, 2024 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston commented Oct 31, 2024 • edited by AlekseyTs Loading

cston commented Oct 31, 2024 • edited by AlekseyTs Loading

AlekseyTs commented Oct 31, 2024

AlekseyTs commented Oct 31, 2024

cston Oct 31, 2024 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston Oct 31, 2024 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

AlekseyTs commented Oct 31, 2024

AlekseyTs commented Nov 1, 2024

333fred left a comment

Choose a reason for hiding this comment

333fred Nov 1, 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

AlekseyTs Nov 1, 2024 • edited 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

VB: Recognize and check an `unmanaged` constraint #75665

VB: Recognize and check an `unmanaged` constraint #75665

cston Oct 31, 2024 •

edited by AlekseyTs

Loading

cston Oct 31, 2024 •

edited by AlekseyTs

Loading

cston Oct 31, 2024 •

edited by AlekseyTs

Loading

cston Oct 31, 2024 •

edited by AlekseyTs

Loading

cston Oct 31, 2024 •

edited by AlekseyTs

Loading

cston Oct 31, 2024 •

edited by AlekseyTs

Loading

cston commented Oct 31, 2024 •

edited by AlekseyTs

Loading

cston commented Oct 31, 2024 •

edited by AlekseyTs

Loading

cston Oct 31, 2024 •

edited by AlekseyTs

Loading

cston Oct 31, 2024 •

edited by AlekseyTs

Loading

333fred Nov 1, 2024 •

edited by AlekseyTs

Loading

AlekseyTs Nov 1, 2024 •

edited

Loading