Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 1 commit into from
Apr 20, 2021
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 12 additions & 15 deletions src/Compilers/CSharp/Portable/Binder/Binder_Operators.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2207,25 +2207,22 @@ private bool CheckConstraintLanguageVersionAndRuntimeSupportForOperator(SyntaxNo
{
bool result = true;

if (methodOpt?.ContainingType?.IsInterface == true)
if (methodOpt?.ContainingType?.IsInterface == true && methodOpt.IsStatic && methodOpt.IsAbstract)
{
if (methodOpt.IsStatic && methodOpt.IsAbstract)
if (constrainedToTypeOpt is not TypeParameterSymbol)
{
if (constrainedToTypeOpt is not TypeParameterSymbol)
{
Error(diagnostics, ErrorCode.ERR_BadAbstractStaticMemberAccess, node);
return false;
}
Error(diagnostics, ErrorCode.ERR_BadAbstractStaticMemberAccess, node);
return false;
}

if (Compilation.SourceModule != methodOpt.ContainingModule)
{
result = CheckFeatureAvailability(node, MessageID.IDS_FeatureStaticAbstractMembersInInterfaces, diagnostics);
if (Compilation.SourceModule != methodOpt.ContainingModule)
{
result = CheckFeatureAvailability(node, MessageID.IDS_FeatureStaticAbstractMembersInInterfaces, diagnostics);

if (!Compilation.Assembly.RuntimeSupportsStaticAbstractMembersInInterfaces)
{
Error(diagnostics, ErrorCode.ERR_RuntimeDoesNotSupportStaticAbstractMembersInInterfaces, node);
return false;
}
if (!Compilation.Assembly.RuntimeSupportsStaticAbstractMembersInInterfaces)
{
Error(diagnostics, ErrorCode.ERR_RuntimeDoesNotSupportStaticAbstractMembersInInterfaces, node);
return false;
}
}
}
Expand Down
1 change: 1 addition & 0 deletions src/Compilers/CSharp/Portable/BoundTree/BoundNodes.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -333,6 +333,7 @@
<!-- Represents a resolved AddressOf function pointer. Not used in initial binding. -->
<Node Name="BoundFunctionPointerLoad" Base="BoundExpression">
<Field Name="TargetMethod" Type="MethodSymbol"/>
<Field Name="ConstrainedToTypeOpt" Type="TypeSymbol?" Null="allow"/>
<!-- Non-null type is required for this node kind -->
<Field Name="Type" Type="TypeSymbol" Override="true" Null="disallow"/>
</Node>
Expand Down
13 changes: 12 additions & 1 deletion src/Compilers/CSharp/Portable/CodeGen/EmitConversion.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -324,6 +324,17 @@ private void EmitDelegateCreation(BoundExpression node, BoundExpression receiver
if (isStatic)
{
_builder.EmitNullConstant();

if (method.IsAbstract)
{
if (receiver is not BoundTypeExpression { Type: { TypeKind: TypeKind.TypeParameter } })
{
throw ExceptionUtilities.Unreachable;
}

_builder.EmitOpCode(ILOpCode.Constrained);
EmitSymbolToken(receiver.Type, receiver.Syntax);
}
}
else
{
Expand All @@ -339,7 +350,7 @@ private void EmitDelegateCreation(BoundExpression node, BoundExpression receiver
// Metadata Spec (II.14.6):
// Delegates shall be declared sealed.
// The Invoke method shall be virtual.
if (method.IsMetadataVirtual() && !method.ContainingType.IsDelegateType() && !receiver.SuppressVirtualCalls)
if (!method.IsStatic && method.IsMetadataVirtual() && !method.ContainingType.IsDelegateType() && !receiver.SuppressVirtualCalls)
{
// NOTE: method.IsMetadataVirtual -> receiver != null
_builder.EmitOpCode(ILOpCode.Dup);
Expand Down
17 changes: 14 additions & 3 deletions src/Compilers/CSharp/Portable/CodeGen/EmitExpression.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1525,7 +1525,7 @@ private void EmitStaticCallExpression(BoundCall call, UseKind useKind)
Debug.Assert(method.IsStatic);

EmitArguments(arguments, method.Parameters, call.ArgumentRefKindsOpt);
int stackBehavior = GetCallStackBehavior(call.Method, arguments);
int stackBehavior = GetCallStackBehavior(method, arguments);

if (method.IsAbstract)
{
Expand Down Expand Up @@ -1685,7 +1685,7 @@ private void EmitInstanceCallExpression(BoundCall call, UseKind useKind)
}

EmitArguments(arguments, method.Parameters, call.ArgumentRefKindsOpt);
int stackBehavior = GetCallStackBehavior(call.Method, arguments);
int stackBehavior = GetCallStackBehavior(method, arguments);
switch (callKind)
{
case CallKind.Call:
Expand Down Expand Up @@ -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)
Copy link
Contributor

@RikkiGibson RikkiGibson Apr 19, 2021 •
edited by AlekseyTs
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

{
return true;
}
Expand Down Expand Up @@ -3519,6 +3519,17 @@ private void EmitLoadFunction(BoundFunctionPointerLoad load, bool used)

if (used)
{
if (load.TargetMethod.IsAbstract && load.TargetMethod.IsStatic)
{
if (load.ConstrainedToTypeOpt is not { TypeKind: TypeKind.TypeParameter })
{
throw ExceptionUtilities.Unreachable;
}

_builder.EmitOpCode(ILOpCode.Constrained);
EmitSymbolToken(load.ConstrainedToTypeOpt, load.Syntax);
}

_builder.EmitOpCode(ILOpCode.Ldftn);
EmitSymbolToken(load.TargetMethod, load.Syntax, optArgList: null);
}
Expand Down
33 changes: 31 additions & 2 deletions src/Compilers/CSharp/Portable/CodeGen/Optimizer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2076,8 +2076,7 @@ public override BoundNode VisitAssignmentOperator(BoundAssignmentOperator node)

// do actual assignment

// PROTOTYPE(StaticAbstractMembersInInterfaces): This assert fires for a user-defined prefix increment/decrement. Open an issue.
//Debug.Assert(locInfo.LocalDefs.Any((d) => _nodeCounter == d.Start && _nodeCounter <= d.End));
Debug.Assert(locInfo.LocalDefs.Any((d) => _nodeCounter == d.Start && _nodeCounter <= d.End));
var isLast = IsLastAccess(locInfo, _nodeCounter);

if (isLast)
Expand All @@ -2093,6 +2092,36 @@ public override BoundNode VisitAssignmentOperator(BoundAssignmentOperator node)
}
}

#nullable enable
public override BoundNode VisitCall(BoundCall node)
{
BoundExpression? receiverOpt = node.ReceiverOpt;

if (node.Method.RequiresInstanceReceiver)
{
receiverOpt = (BoundExpression?)this.Visit(receiverOpt);
}
else
{
Copy link
Contributor

@RikkiGibson RikkiGibson Apr 19, 2021 •
edited by AlekseyTs
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

_nodeCounter++;

if (receiverOpt is BoundTypeExpression { AliasOpt: null, BoundContainingTypeOpt: null, BoundDimensionsOpt: { IsEmpty: true }, Type: { TypeKind: TypeKind.TypeParameter } } typeExpression)
{
receiverOpt = typeExpression.Update(aliasOpt: null, boundContainingTypeOpt: null, boundDimensionsOpt: ImmutableArray<BoundExpression>.Empty,
typeWithAnnotations: typeExpression.TypeWithAnnotations, type: this.VisitType(typeExpression.Type));
}
else if (receiverOpt is not null)
{
throw ExceptionUtilities.Unreachable;
}
}

ImmutableArray<BoundExpression> arguments = this.VisitList(node.Arguments);
TypeSymbol? type = this.VisitType(node.Type);
return node.Update(receiverOpt, node.Method, arguments, node.ArgumentNamesOpt, node.ArgumentRefKindsOpt, node.IsDelegateCall, node.Expanded, node.InvokedAsExtensionMethod, node.ArgsToParamsOpt, node.DefaultArguments, node.ResultKind, node.OriginalMethodsOpt, type);
}
#nullable disable

public override BoundNode VisitCatchBlock(BoundCatchBlock node)
{
var exceptionSource = node.ExceptionSourceOpt;
Expand Down
23 changes: 15 additions & 8 deletions src/Compilers/CSharp/Portable/Generated/BoundNodes.xml.Generated.cs
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Lowering/ClosureConversion/ClosureConversion.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1334,7 +1334,7 @@ public override BoundNode VisitFunctionPointerLoad(BoundFunctionPointerLoad node
receiver.Kind == BoundKind.TypeExpression &&
remappedMethod is { RequiresInstanceReceiver: false, IsStatic: true });

return node.Update(remappedMethod, node.Type);
return node.Update(remappedMethod, constrainedToTypeOpt: node.ConstrainedToTypeOpt, node.Type);
}

return base.VisitFunctionPointerLoad(node);
Expand Down
4 changes: 4 additions & 0 deletions src/Compilers/CSharp/Portable/Lowering/DiagnosticsPass_ExpressionTrees.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -785,6 +785,10 @@ private void CheckMethodGroup(BoundMethodGroup node, MethodSymbol method, bool p
{
Error(ErrorCode.ERR_AddressOfMethodGroupInExpressionTree, node);
}
else if (method is not null && method.IsAbstract && method.IsStatic)
{
Error(ErrorCode.ERR_ExpressionTreeContainsAbstractStaticMemberAccess, node);
}
}

CheckReceiverIfField(node.ReceiverOpt);
Expand Down
6 changes: 4 additions & 2 deletions src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_Conversion.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -379,7 +379,9 @@ private BoundExpression MakeConversionNodeCore(
{
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,
Copy link
Contributor

@RikkiGibson RikkiGibson Apr 19, 2021 •
edited by AlekseyTs
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

constrainedToTypeOpt: oldNodeOpt.SymbolOpt.IsStatic && oldNodeOpt.SymbolOpt.IsAbstract ? mg.ReceiverOpt?.Type : null,
Copy link
Member

@333fred 333fred Apr 19, 2021 •
edited by AlekseyTs
Loading

Choose a reason for hiding this comment

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

?

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

type: funcPtrType, hasErrors: false);
}

case ConversionKind.MethodGroup:
Expand All @@ -391,7 +393,7 @@ private BoundExpression MakeConversionNodeCore(
Debug.Assert(method is { });
var oldSyntax = _factory.Syntax;
_factory.Syntax = (mg.ReceiverOpt ?? mg).Syntax;
var receiver = (!method.RequiresInstanceReceiver && !oldNodeOpt.IsExtensionMethod) ? _factory.Type(method.ContainingType) : mg.ReceiverOpt;
var receiver = (!method.RequiresInstanceReceiver && !oldNodeOpt.IsExtensionMethod && !method.IsAbstract) ? _factory.Type(method.ContainingType) : mg.ReceiverOpt;
Debug.Assert(receiver is { });
_factory.Syntax = oldSyntax;
return new BoundDelegateCreationExpression(syntax, argument: receiver, methodOpt: method,
Expand Down
2 changes: 1 addition & 1 deletion ...pilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_DelegateCreationExpression.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ public override BoundNode VisitDelegateCreationExpression(BoundDelegateCreationE
Debug.Assert(method is { });
var oldSyntax = _factory.Syntax;
_factory.Syntax = (mg.ReceiverOpt ?? mg).Syntax;
var receiver = (!method.RequiresInstanceReceiver && !node.IsExtensionMethod) ? _factory.Type(method.ContainingType) : VisitExpression(mg.ReceiverOpt)!;
var receiver = (!method.RequiresInstanceReceiver && !node.IsExtensionMethod && !method.IsAbstract) ? _factory.Type(method.ContainingType) : VisitExpression(mg.ReceiverOpt)!;
_factory.Syntax = oldSyntax;
return node.Update(receiver, method, node.IsExtensionMethod, node.Type);
}
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Lowering/MethodToClassRewriter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -512,7 +512,7 @@ public override BoundNode VisitDelegateCreationExpression(BoundDelegateCreationE

public override BoundNode VisitFunctionPointerLoad(BoundFunctionPointerLoad node)
{
return node.Update(VisitMethodSymbol(node.TargetMethod), VisitType(node.Type));
return node.Update(VisitMethodSymbol(node.TargetMethod), VisitType(node.ConstrainedToTypeOpt), VisitType(node.Type));
}

public override BoundNode VisitLoweredConditionalAccess(BoundLoweredConditionalAccess node)
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Symbols/OverriddenOrHiddenMembersHelpers.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1019,7 +1019,7 @@ internal static MethodSymbol GetFirstRuntimeOverriddenMethodIgnoringNewSlot(this
const bool ignoreInterfaceImplementationChanges = true;

wasAmbiguous = false;
if (!method.IsMetadataVirtual(ignoreInterfaceImplementationChanges))
if (!method.IsMetadataVirtual(ignoreInterfaceImplementationChanges) || method.IsStatic)
{
return null;
}
Expand Down
Loading