Support delegate or function pointer creation based on a static abstract method. #52685

AlekseyTs · 2021-04-16T22:29:55Z

No description provided.

…act method.

AlekseyTs · 2021-04-19T16:17:37Z

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

RikkiGibson · 2021-04-19T18:01:29Z

src/Compilers/CSharp/Portable/CodeGen/EmitExpression.cs

@@ -1830,7 +1830,7 @@ internal static bool MayUseCallForStructMethod(MethodSymbol method)
        {
            Debug.Assert(method.ContainingType.IsVerifierValue(), "this is not a value type");

-            if (!method.IsMetadataVirtual())
+            if (!method.IsMetadataVirtual() || method.IsStatic)


To make sure I understand the change: it looks like this lets us use 'call' instruction when calling a method that implements a static abstract method. Is that correct? #Resolved

To make sure I understand the change: it looks like this lets us use 'call' instruction when calling a method that implements a static abstract method. Is that correct?

At the moment, this chnage is not observable in any way, but static methods can be invoked only by 'call' instruction.

RikkiGibson · 2021-04-19T18:07:33Z

src/Compilers/CSharp/Portable/CodeGen/Optimizer.cs

+                receiverOpt = (BoundExpression?)this.Visit(receiverOpt);
+            }
+            else
+            {


It looks like this is necessary because this.Visit is unable to handle a BoundTypeExpression. Is that right? #Resolved

It looks like this is necessary because this.Visit is unable to handle a BoundTypeExpression. Is that right?

In genera it can do that. However, since the first pass specially visits BoundCall, we have to mirror that here as well in order to make sure that the count of nodes is in sync across passes.

RikkiGibson · 2021-04-19T18:09:32Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_Conversion.cs

@@ -379,7 +379,9 @@ private static bool IsFloatingPointExpressionOfUnknownPrecision(BoundExpression
                    {
                        var mg = (BoundMethodGroup)rewrittenOperand;
                        Debug.Assert(oldNodeOpt.SymbolOpt is { });
-                        return new BoundFunctionPointerLoad(oldNodeOpt.Syntax, oldNodeOpt.SymbolOpt, type: funcPtrType, hasErrors: false);
+                        return new BoundFunctionPointerLoad(oldNodeOpt.Syntax, oldNodeOpt.SymbolOpt,


If we made it to lowering with a static abstract method group without a type parameter receiver, does that mean we failed to give a diagnostic during binding? #Resolved

If we made it to lowering with a static abstract method group without a type parameter receiver, does that mean we failed to give a diagnostic during binding?

Pretty much.

RikkiGibson · 2021-04-19T18:27:05Z

src/Compilers/CSharp/Test/Symbol/Symbols/StaticAbstractMembersInInterfacesTests.cs

+                // (30,31): error CS0176: Member 'I1.M04()' cannot be accessed with an instance reference; qualify it with a type name instead
+                //         _ = new System.Action(x.M04);
+                Diagnostic(ErrorCode.ERR_ObjectProhibited, "x.M04").WithArguments("I1.M04()").WithLocation(30, 31),
+                // (35,31): error CS0119: 'T' is a type parameter, which is not valid in the given context


not for this PR, but I feel like we should look at changing this diagnostic message to indicate that the reason T can't be used as a receiver is because M03 is not static abstract.

AlekseyTs · 2021-04-19T20:26:43Z

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

333fred · 2021-04-19T23:14:50Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_Conversion.cs

@@ -379,7 +379,9 @@ private static bool IsFloatingPointExpressionOfUnknownPrecision(BoundExpression
                    {
                        var mg = (BoundMethodGroup)rewrittenOperand;
                        Debug.Assert(oldNodeOpt.SymbolOpt is { });
-                        return new BoundFunctionPointerLoad(oldNodeOpt.Syntax, oldNodeOpt.SymbolOpt, type: funcPtrType, hasErrors: false);
+                        return new BoundFunctionPointerLoad(oldNodeOpt.Syntax, oldNodeOpt.SymbolOpt,
+                                                            constrainedToTypeOpt: oldNodeOpt.SymbolOpt.IsStatic && oldNodeOpt.SymbolOpt.IsAbstract ? mg.ReceiverOpt?.Type : null,


?

Should we just dereference this, or give some kind of an error to handle the case? If we have a static abstract, we need to have a type or further layers are going to crash, right? #Resolved

Should we just dereference this, or give some kind of an error to handle the case? If we have a static abstract, we need to have a type or further layers are going to crash, right?

I prefer to not surface this as a NullReference exception

An exception in the emit layer is sufficient to deal with this unexpected situation

I prefer to not surface this as a NullReference exception

That's fine, but we're seriously busted at this point, right? Should we consider throwing an unreachable or something?

In reply to: 616239428 [](ancestors = 616239428)

That's fine, but we're seriously busted at this point, right? Should we consider throwing an unreachable or something?

An emit layer takes care of this

An emit layer takes care of this

I would say consider at least leaving a comment. Explicitly "dealing" with the potential null by doing a conditional access here feels to me like an expression of intent, but I don't read the intent as "let the emit layer throw". I read it as "this should have some meaning", which is not what is meant.

In reply to: 616240220 [](ancestors = 616240220)

My intent is to not perform any validation here. Null reference is not the only thing we should worry about. The code matches the intent.

333fred · 2021-04-19T23:36:21Z

src/Compilers/CSharp/Test/Symbol/Symbols/StaticAbstractMembersInInterfacesTests.cs

+@"
+public interface I1
+{
+    abstract static void M01();


Doesn't need to be in this PR, but we likely need to have some tests around UnmanagedCallersOnly. #Resolved

Doesn't need to be in this PR, but we likely need to have some tests around UnmanagedCallersOnly.

What would be unique about this scenario by comparison to consumprion of a regular static method?

What would be unique about this scenario by comparison to consumprion of a regular static method?

We don't support UnmanagedCallersOnly on instance methods at all, nor do we support taking a pointer to instance methods. We probably shouldn't support the attribute on abstract statics either, or we should make sure that users are forced to apply the attribute to implementations of an abstract static member in a way that matches up.

We don't support UnmanagedCallersOnly on instance methods at all, nor do we support taking a pointer to instance methods. We probably shouldn't support them on abstract statics either, or we should make sure that users are forced to apply the attribute to implementations of an abstract static member in a way that matches up.

Recorded in #52221

333fred

LGTM (commit 1). Consider leaving the suggested comment to make intent clearer.

Support delegate or function pointer creation based on a static abstr…

8821e68

…act method.

AlekseyTs added Area-Compilers New Feature - StaticAbstractMembersInInterfaces labels Apr 16, 2021

AlekseyTs requested review from 333fred, RikkiGibson and a team April 16, 2021 22:29

RikkiGibson self-assigned this Apr 19, 2021

RikkiGibson approved these changes Apr 19, 2021

View reviewed changes

333fred reviewed Apr 19, 2021

View reviewed changes

333fred approved these changes Apr 19, 2021

View reviewed changes

AlekseyTs merged commit 18cd442 into dotnet:features/StaticAbstractMembersInInterfaces Apr 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support delegate or function pointer creation based on a static abstract method. #52685

Support delegate or function pointer creation based on a static abstract method. #52685

AlekseyTs commented Apr 16, 2021

AlekseyTs commented Apr 19, 2021

RikkiGibson Apr 19, 2021 •

edited by AlekseyTs

Loading

AlekseyTs Apr 19, 2021

RikkiGibson Apr 19, 2021 •

edited by AlekseyTs

Loading

AlekseyTs Apr 19, 2021

RikkiGibson Apr 19, 2021 •

edited by AlekseyTs

Loading

AlekseyTs Apr 19, 2021

RikkiGibson Apr 19, 2021

AlekseyTs commented Apr 19, 2021

333fred Apr 19, 2021 •

edited by AlekseyTs

Loading

AlekseyTs Apr 19, 2021

AlekseyTs Apr 19, 2021

333fred Apr 19, 2021

AlekseyTs Apr 19, 2021

333fred Apr 19, 2021

AlekseyTs Apr 20, 2021

333fred Apr 19, 2021 •

edited by AlekseyTs

Loading

AlekseyTs Apr 20, 2021

333fred Apr 20, 2021 •

edited

Loading

AlekseyTs Apr 20, 2021

333fred left a comment

Support delegate or function pointer creation based on a static abstract method. #52685

Support delegate or function pointer creation based on a static abstract method. #52685

Conversation

AlekseyTs commented Apr 16, 2021

AlekseyTs commented Apr 19, 2021

RikkiGibson Apr 19, 2021 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RikkiGibson Apr 19, 2021 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RikkiGibson Apr 19, 2021 • 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 commented Apr 19, 2021

333fred Apr 19, 2021 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

333fred Apr 19, 2021 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

333fred Apr 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

333fred left a comment

Choose a reason for hiding this comment

RikkiGibson Apr 19, 2021 •

edited by AlekseyTs

Loading

RikkiGibson Apr 19, 2021 •

edited by AlekseyTs

Loading

RikkiGibson Apr 19, 2021 •

edited by AlekseyTs

Loading

333fred Apr 19, 2021 •

edited by AlekseyTs

Loading

333fred Apr 19, 2021 •

edited by AlekseyTs

Loading

333fred Apr 20, 2021 •

edited

Loading