Specially handle more scenarios for type parameters with 'allows ref struct' constraint #73111

AlekseyTs · 2024-04-19T22:12:15Z

No description provided.

…struct' constraint

AlekseyTs · 2024-04-22T17:12:35Z

@cston, @jjonescz, @dotnet/roslyn-compiler Please review

cston · 2024-04-22T18:13:29Z

src/Compilers/CSharp/Portable/Binder/Semantics/Conversions/ConversionsBase.cs

@@ -2829,7 +2829,8 @@ public bool HasImplicitTypeParameterConversion(TypeParameterSymbol source, TypeS
                return true;
            }

-            if ((destination.TypeKind == TypeKind.TypeParameter) &&
+            if (destination is TypeParameterSymbol { AllowByRefLike: false } &&
+                !source.AllowByRefLike &&


!source.AllowByRefLike &&

Are we testing this case? #Resolved

Are we testing this case?

I tried, but given the convoluted nature of the conversion classification algorithms, I was not able to find a scenario where we get here with source.AllowByRefLike equals true. Nevertheless, I am fairly comfortable making this change regardless of the fact.

cston · 2024-04-22T18:30:53Z

src/Compilers/CSharp/Portable/Binder/Semantics/Conversions/ConversionsBase.cs

@@ -2868,7 +2872,7 @@ private bool HasImplicitReferenceTypeParameterConversion(TypeParameterSymbol sou
            }

            // * From T to a type parameter U, provided T depends on U.
-            if ((destination.TypeKind == TypeKind.TypeParameter) &&
+            if (destination is TypeParameterSymbol { AllowByRefLike: false } &&


if (destination is TypeParameterSymbol { AllowByRefLike: false } &&

Why is AllowByRefLike: false required? Isn't it sufficient to check source.AllowByRefLike (above) and source.DependsOn((TypeParameterSymbol)destination)? #Resolved

Why is AllowByRefLike: false required? Isn't it sufficient to check source.AllowByRefLike (above) and source.DependsOn((TypeParameterSymbol)destination)?

Why do you think it should be sufficient? Dependency between type parameters says nothing about their AllowByRefLike property (dependent type parameters can have different values for it) and reference conversions do not exist for those that have AllowByRefLike: true.

cston · 2024-04-22T19:00:37Z

src/Compilers/CSharp/Portable/Binder/Semantics/Conversions/ConversionsBase.cs

@@ -3221,8 +3225,8 @@ private bool HasImplicitBoxingTypeParameterConversion(TypeParameterSymbol source
            }

            // SPEC: From T to a type parameter U, provided T depends on U
-            if ((destination.TypeKind == TypeKind.TypeParameter) &&
-                source.DependsOn((TypeParameterSymbol)destination))
+            if (destination is TypeParameterSymbol { AllowByRefLike: false } d &&


AllowByRefLike: false

Why is AllowByRefLike: false required? #Resolved

Why is AllowByRefLike: false required?

Same response as for the change on line 2875

cston · 2024-04-22T19:39:49Z

src/Compilers/CSharp/Test/Emit3/RefStructInterfacesTests.cs

+        // IL_0000:  ldarg.0
+        // IL_0001:  box        ""U""
+        // IL_0006:  unbox.any  ""T""
+        // IL_000b:  ret
    }
 }


Consider testing:

static U Test9<T, U>(T x) where T : class where U : T, allows ref struct { return x; } static U Test10<T, U>(T x) where T : class where U : T, allows ref struct { return (U)x; } ``` #Closed

cston · 2024-04-23T17:30:52Z

src/Compilers/CSharp/Test/Emit3/RefStructInterfacesTests.cs

+        }
+
+        [Fact]
+        public void IllegalBoxing_05()


IllegalBoxing_05

Is this distinct from IllegalBoxing_02? #Resolved

Is this distinct from IllegalBoxing_02?

It is true that some scenarios are duplicated (not all scenarios), but I do not mind and do not plan to make any changes because of that.

cston · 2024-04-23T17:40:30Z

src/Compilers/CSharp/Test/Emit3/RefStructInterfacesTests.cs

+                //         => y.M;
+                Diagnostic(ErrorCode.ERR_MethDelegateMismatch, "M").WithArguments("M", "System.Action").WithLocation(12, 14)
+                );
+        }


}

Are we testing the receiver for anonymous delegate types?

static void Test3(T x) { var f = x.M; } ``` #Closed

Are we testing the receiver for anonymous delegate types?

Adding a couple of tests for completeness.

cston · 2024-04-23T18:47:49Z

src/Compilers/CSharp/Test/Emit3/RefStructInterfacesTests.cs

+                //         h2.M2(y);
+                Diagnostic(ErrorCode.ERR_BadDynamicMethodArg, "y").WithArguments("S").WithLocation(15, 15)
+                );
+        }


}

Are we testing a ref struct receiver and dynamic argument?

class Helper1<T> where T : I1, allows ref struct { static void Test1(T x, dynamic d) { x.M(d); } } interface I1 { void M<T>(T t); } ``` #Closed

Are we testing a ref struct receiver and dynamic argument?

Adding DynamicAccess_03 for completeness.

AlekseyTs · 2024-04-24T21:39:37Z

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

jjonescz · 2024-04-25T10:23:35Z

src/Compilers/CSharp/Test/Emit3/RefStructInterfacesTests.cs

+{
+    static void Test1()
+    {
+        _ = new T[] {};


Consider testing T[][]

Specially handle more scenarios for type parameters with 'allows ref …

4aa6af4

…struct' constraint

AlekseyTs added Area-Compilers Feature - RefStructInterfaces labels Apr 19, 2024

AlekseyTs requested review from jjonescz and cston April 19, 2024 22:12

AlekseyTs requested a review from a team as a code owner April 19, 2024 22:12

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

cston reviewed Apr 22, 2024

View reviewed changes

cston reviewed Apr 23, 2024

View reviewed changes

Add suggested tests

a851aa8

AlekseyTs requested a review from cston April 24, 2024 19:50

cston approved these changes Apr 24, 2024

View reviewed changes

jjonescz approved these changes Apr 25, 2024

View reviewed changes

src/Compilers/CSharp/Test/Emit3/RefStructInterfacesTests.cs

{

static void Test1()

{

_ = new T[] {};

Copy link

Member

jjonescz Apr 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider testing T[][]

AlekseyTs merged commit 81992c9 into dotnet:features/RefStructInterfaces Apr 25, 2024
24 checks passed

This was referenced May 21, 2024

[Automated] PRs inserted in VS build main-34920.65 #73605

Closed

[Automated] PRs inserted in VS build feature.debugger.main-34920.221 #73610

Closed

dotnet-bot mentioned this pull request Jun 27, 2024

[Automated] PRs inserted in VS build feature.dotnetVS-35026.97 #74176

Closed

dibarbet mentioned this pull request Jul 25, 2024

Find-All-References results incomplete and unstable dotnet/vscode-csharp#7391

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Specially handle more scenarios for type parameters with 'allows ref struct' constraint #73111

Specially handle more scenarios for type parameters with 'allows ref struct' constraint #73111

AlekseyTs commented Apr 19, 2024

AlekseyTs commented Apr 22, 2024

cston Apr 22, 2024 •

edited by AlekseyTs

Loading

AlekseyTs Apr 24, 2024

cston Apr 22, 2024 •

edited by AlekseyTs

Loading

AlekseyTs Apr 24, 2024

cston Apr 22, 2024 •

edited by AlekseyTs

Loading

AlekseyTs Apr 24, 2024

cston Apr 22, 2024 •

edited

Loading

cston Apr 23, 2024 •

edited by AlekseyTs

Loading

AlekseyTs Apr 24, 2024 •

edited

Loading

cston Apr 23, 2024 •

edited

Loading

AlekseyTs Apr 24, 2024

cston Apr 23, 2024 •

edited

Loading

AlekseyTs Apr 24, 2024

AlekseyTs commented Apr 24, 2024

jjonescz Apr 25, 2024

Specially handle more scenarios for type parameters with 'allows ref struct' constraint #73111

Specially handle more scenarios for type parameters with 'allows ref struct' constraint #73111

Conversation

AlekseyTs commented Apr 19, 2024

AlekseyTs commented Apr 22, 2024

cston Apr 22, 2024 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

AlekseyTs Apr 24, 2024

Choose a reason for hiding this comment

cston Apr 22, 2024 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

AlekseyTs Apr 24, 2024

Choose a reason for hiding this comment

cston Apr 22, 2024 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

AlekseyTs Apr 24, 2024

Choose a reason for hiding this comment

cston Apr 22, 2024 • edited Loading

Choose a reason for hiding this comment

cston Apr 23, 2024 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

AlekseyTs Apr 24, 2024 • edited Loading

Choose a reason for hiding this comment

cston Apr 23, 2024 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Apr 24, 2024

Choose a reason for hiding this comment

cston Apr 23, 2024 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Apr 24, 2024

Choose a reason for hiding this comment

AlekseyTs commented Apr 24, 2024

jjonescz Apr 25, 2024

Choose a reason for hiding this comment

cston Apr 22, 2024 •

edited by AlekseyTs

Loading

cston Apr 22, 2024 •

edited by AlekseyTs

Loading

cston Apr 22, 2024 •

edited by AlekseyTs

Loading

cston Apr 22, 2024 •

edited

Loading

cston Apr 23, 2024 •

edited by AlekseyTs

Loading

AlekseyTs Apr 24, 2024 •

edited

Loading

cston Apr 23, 2024 •

edited

Loading

cston Apr 23, 2024 •

edited

Loading