Eliminate bound method groups during local lowering (#17227)

Fixes #14882
Fixes #13840
Fixes #13915

src/Compilers/CSharp/Portable/BoundTree/BoundNodes.xml

@@ -1394,6 +1394,17 @@
     <!-- Non-null type is required for this node kind -->
     <Field Name="Type" Type="TypeSymbol" Override="true" Null="disallow"/>
 
+    <!-- Argument is one of the following:
+         1. A method group (before local lowering only). If the method is nonstatic or
+            converted as an extension method, the method group contains a receiver
+            expression (mg.ReceiverOpt) for the created delegate; or
+         2. A value of type dynamic (before local lowering only); or
+         3. A bound lambda (before lambda lowering only); or
+         4. A value of a delegate type with MethodOpt == null; or
+         5. The receiver of the method being converted to a delegate (after local lowering).
+            It may be a BoundTypeExpression if the method is static and not converted as an
+            extension method.
+         -->
     <Field Name="Argument" Type="BoundExpression"/>
     <Field Name="MethodOpt" Type="MethodSymbol" Null="allow"/>
     <Field Name="IsExtensionMethod" Type="bool"/>

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

@@ -14,8 +14,7 @@ private void EmitConversionExpression(BoundConversion conversion, bool used)
             switch (conversion.ConversionKind)
             {
                 case ConversionKind.MethodGroup:
-                    EmitMethodGroupConversion(conversion, used);
-                    return;
+                    throw ExceptionUtilities.UnexpectedValue(conversion.ConversionKind);
                 case ConversionKind.NullToPointer:
                     // The null pointer is represented as 0u.
                     _builder.EmitIntConstant(0);
@@ -313,11 +312,5 @@ private MethodSymbol DelegateConstructor(SyntaxNode syntax, TypeSymbol delegateT
             _diagnostics.Add(ErrorCode.ERR_BadDelegateConstructor, syntax.Location, delegateType);
             return null;
         }
-
-        private void EmitMethodGroupConversion(BoundConversion conversion, bool used)
-        {
-            var group = (BoundMethodGroup)conversion.Operand;
-            EmitDelegateCreation(conversion, group.InstanceOpt, conversion.IsExtensionMethod, conversion.SymbolOpt, conversion.Type, used);
-        }
     }
 }

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

@@ -633,8 +633,8 @@ private void EmitDupExpression(BoundDup expression, bool used)
 
         private void EmitDelegateCreationExpression(BoundDelegateCreationExpression expression, bool used)
         {
-            var mg = expression.Argument as BoundMethodGroup;
-            var receiver = mg != null ? mg.ReceiverOpt : expression.Argument;
+            Debug.Assert(expression.Argument?.Kind != BoundKind.MethodGroup);
+            var receiver = expression.Argument;
             var meth = expression.MethodOpt ?? receiver.Type.DelegateInvokeMethod();
             Debug.Assert((object)meth != null);
             EmitDelegateCreation(expression, receiver, expression.IsExtensionMethod, meth, expression.Type, used);
@@ -1269,7 +1269,7 @@ private bool CanUseCallOnRefTypeReceiver(BoundExpression receiver)
 
                         case ConversionKind.MethodGroup:
                         case ConversionKind.AnonymousFunction:
-                            return true;
+                            throw ExceptionUtilities.UnexpectedValue(conversion.ConversionKind);
 
                         case ConversionKind.ExplicitReference:
                         case ConversionKind.ImplicitReference:
@@ -3010,9 +3010,9 @@ private TypeSymbol StackMergeType(BoundExpression expr)
                     var conversion = (BoundConversion)expr;
                     var conversionKind = conversion.ConversionKind;
                     if (conversionKind.IsImplicitConversion() &&
-                        conversionKind != ConversionKind.MethodGroup &&
                         conversionKind != ConversionKind.NullLiteral)
                     {
+                        Debug.Assert(conversionKind != ConversionKind.MethodGroup);
                         return StackMergeType(conversion.Operand);
                     }
                     break;

src/Compilers/CSharp/Portable/Lowering/AsyncRewriter/AwaitExpressionSpiller.cs

@@ -856,9 +856,7 @@ public override BoundNode VisitConversion(BoundConversion node)
 
         public override BoundNode VisitMethodGroup(BoundMethodGroup node)
         {
-            BoundSpillSequenceBuilder builder = null;
-            var receiver = VisitExpression(ref builder, node.ReceiverOpt);
-            return UpdateExpression(builder, node.Update(node.TypeArgumentsOpt, node.Name, node.Methods, node.LookupSymbolOpt, node.LookupError, node.Flags, receiver, node.ResultKind));
+            throw ExceptionUtilities.Unreachable;
         }
 
         public override BoundNode VisitDelegateCreationExpression(BoundDelegateCreationExpression node)

src/Compilers/CSharp/Portable/Lowering/LambdaRewriter/ExpressionLambdaRewriter.cs

@@ -688,10 +688,15 @@ private BoundExpression DelegateCreation(BoundExpression receiver, MethodSymbol
 
         private BoundExpression VisitDelegateCreationExpression(BoundDelegateCreationExpression node)
         {
-            var mg = node.Argument as BoundMethodGroup;
-            if (mg != null)
+            if (node.Argument.Kind == BoundKind.MethodGroup)
             {
-                return DelegateCreation(mg.ReceiverOpt, node.MethodOpt, node.Type, node.MethodOpt.IsStatic && !mg.SearchExtensionMethods);
+                throw ExceptionUtilities.UnexpectedValue(BoundKind.MethodGroup);
+            }
+
+            if ((object)node.MethodOpt != null)
+            {
+                bool staticMember = node.MethodOpt.IsStatic && !node.IsExtensionMethod;
+                return DelegateCreation(node.Argument, node.MethodOpt, node.Type, staticMember);
             }
 
             var d = node.Argument.Type as NamedTypeSymbol;

src/Compilers/CSharp/Portable/Lowering/LambdaRewriter/LambdaRewriter.Analysis.cs

@@ -557,37 +557,7 @@ private BoundNode VisitSyntaxWithReceiver(SyntaxNode syntax, BoundNode receiver)
 
             public override BoundNode VisitMethodGroup(BoundMethodGroup node)
             {
-                // We only get here in error cases, as normally the enclosing node is a method group conversion
-                // whose visit (below) doesn't call this.  So we don't know which method is to be selected, and
-                // therefore don't know if the receiver is used. Assume if the receiver was provided, it is used.
-                var receiverOpt = node.ReceiverOpt;
-                if (receiverOpt != null)
-                {
-                    return VisitSyntaxWithReceiver(node.Syntax, receiverOpt);
-                }
-                return null;
-            }
-
-            public override BoundNode VisitConversion(BoundConversion node)
-            {
-                if (node.ConversionKind == ConversionKind.MethodGroup)
-                {
-                    if (node.SymbolOpt?.MethodKind == MethodKind.LocalFunction)
-                    {
-                        // Use OriginalDefinition to strip generic type parameters
-                        ReferenceVariable(node.Syntax, node.SymbolOpt.OriginalDefinition);
-                        MethodsConvertedToDelegates.Add(node.SymbolOpt.OriginalDefinition);
-                    }
-                    if (node.IsExtensionMethod || ((object)node.SymbolOpt != null && !node.SymbolOpt.IsStatic))
-                    {
-                        return VisitSyntaxWithReceiver(node.Syntax, ((BoundMethodGroup)node.Operand).ReceiverOpt);
-                    }
-                    return null;
-                }
-                else
-                {
-                    return base.VisitConversion(node);
-                }
+                throw ExceptionUtilities.Unreachable;
             }
 
             public override BoundNode VisitPropertyAccess(BoundPropertyAccess node)

src/Compilers/CSharp/Portable/Lowering/LambdaRewriter/LambdaRewriter.LocalFunctionReferenceRewriter.cs

@@ -88,31 +88,6 @@ public override BoundNode VisitDelegateCreationExpression(BoundDelegateCreationE
 
                 return base.VisitDelegateCreationExpression(node);
             }
-
-            public override BoundNode VisitConversion(BoundConversion conversion)
-            {
-                if (conversion.ConversionKind == ConversionKind.MethodGroup &&
-                    conversion.SymbolOpt?.MethodKind == MethodKind.LocalFunction)
-                {
-                    BoundExpression receiver;
-                    MethodSymbol method;
-                    var arguments = default(ImmutableArray<BoundExpression>);
-                    _lambdaRewriter.RemapLocalFunction(
-                        conversion.Syntax,
-                        conversion.SymbolOpt,
-                        out receiver,
-                        out method,
-                        ref arguments);
-
-                    return new BoundDelegateCreationExpression(
-                        conversion.Syntax,
-                        receiver,
-                        method,
-                        isExtensionMethod: false,
-                        type: conversion.Type);
-                }
-                return base.VisitConversion(conversion);
-            }
         }
 
         /// <summary>

src/Compilers/CSharp/Portable/Lowering/LambdaRewriter/LambdaRewriter.cs

@@ -1174,6 +1174,7 @@ public override BoundNode VisitDelegateCreationExpression(BoundDelegateCreationE
 
         public override BoundNode VisitConversion(BoundConversion conversion)
         {
+            Debug.Assert(conversion.ConversionKind != ConversionKind.MethodGroup);
             if (conversion.ConversionKind == ConversionKind.AnonymousFunction)
             {
                 var result = (BoundExpression)RewriteLambdaConversion((BoundLambda)conversion.Operand);
@@ -1194,20 +1195,6 @@ public override BoundNode VisitConversion(BoundConversion conversion)
                 return result;
             }
 
-            if (conversion.ConversionKind == ConversionKind.MethodGroup &&
-                conversion.SymbolOpt?.MethodKind == MethodKind.LocalFunction)
-            {
-                var rewritten = conversion.Update(
-                    conversion.Operand,
-                    conversion.Conversion,
-                    conversion.IsBaseConversion,
-                    conversion.Checked,
-                    conversion.ExplicitCastInCode,
-                    conversion.ConstantValueOpt,
-                    this.VisitType(conversion.Type));
-
-                return PartiallyLowerLocalFunctionReference(rewritten);
-            }
             return base.VisitConversion(conversion);
         }
 

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

@@ -332,6 +332,18 @@ private static bool IsFloatPointExpressionOfUnknownPrecision(BoundExpression rew
                         explicitCastInCode: explicitCastInCode,
                         rewrittenType: (NamedTypeSymbol)rewrittenType);
 
+                case ConversionKind.MethodGroup:
+                    {
+                        // we eliminate the method group conversion entirely from the bound nodes following local lowering
+                        var mg = (BoundMethodGroup)rewrittenOperand;
+                        var method = oldNode.SymbolOpt;
+                        Debug.Assert((object)method != null);
+                        var oldSyntax = _factory.Syntax;
+                        _factory.Syntax = (mg.ReceiverOpt ?? mg).Syntax;
+                        var receiver = (method.IsStatic && !oldNode.IsExtensionMethod) ? _factory.Type(method.ContainingType) : VisitExpression(mg.ReceiverOpt);
+                        _factory.Syntax = oldSyntax;
+                        return new BoundDelegateCreationExpression(syntax, argument: receiver, methodOpt: method, isExtensionMethod: oldNode.IsExtensionMethod, type: rewrittenType);
+                    }
                 default:
                     break;
             }

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

@@ -21,6 +21,17 @@ public override BoundNode VisitDelegateCreationExpression(BoundDelegateCreationE
                 return new BoundDelegateCreationExpression(node.Syntax, loweredReceiver, methodOpt: null, isExtensionMethod: false, type: node.Type);
             }
 
+            if (node.Argument.Kind == BoundKind.MethodGroup)
+            {
+                var mg = (BoundMethodGroup)node.Argument;
+                var method = node.MethodOpt;
+                var oldSyntax = _factory.Syntax;
+                _factory.Syntax = (mg.ReceiverOpt ?? mg).Syntax;
+                var receiver = (method.IsStatic && !node.IsExtensionMethod) ? _factory.Type(method.ContainingType) : VisitExpression(mg.ReceiverOpt);
+                _factory.Syntax = oldSyntax;
+                return node.Update(receiver, method, node.IsExtensionMethod, node.Type);
+            }
+
             return base.VisitDelegateCreationExpression(node);
         }
     }

src/Compilers/CSharp/Portable/Lowering/MethodToClassRewriter.cs

@@ -201,8 +201,9 @@ private Conversion RewriteConversion(Conversion conversion)
             {
                 case ConversionKind.ExplicitUserDefined:
                 case ConversionKind.ImplicitUserDefined:
-                case ConversionKind.MethodGroup:
                     return new Conversion(conversion.Kind, VisitMethodSymbol(conversion.Method), conversion.IsExtensionMethod);
+                case ConversionKind.MethodGroup:
+                    throw ExceptionUtilities.UnexpectedValue(conversion.Kind);
                 default:
                     return conversion;
             }
@@ -476,36 +477,17 @@ public override BoundNode VisitObjectCreationExpression(BoundObjectCreationExpre
             return rewritten;
         }
 
-        public override BoundNode VisitConversion(BoundConversion conversion)
-        {
-            if (conversion.ConversionKind == ConversionKind.MethodGroup && (object)conversion.SymbolOpt != null)
-            {
-                return RewriteMethodGroupConversion(conversion);
-            }
-            else
-            {
-                return base.VisitConversion(conversion);
-            }
-        }
-
         public override BoundNode VisitDelegateCreationExpression(BoundDelegateCreationExpression node)
         {
             BoundExpression originalArgument = node.Argument;
             BoundExpression rewrittenArgument = (BoundExpression)this.Visit(originalArgument);
             MethodSymbol method = node.MethodOpt;
-            if (originalArgument.Kind == BoundKind.MethodGroup && rewrittenArgument.Kind == BoundKind.MethodGroup)
-            {
-                //  if the original argument was a method group AND the receiver was BoundKind.BaseReference
-                //  and the visited argument is still a method group with receiver overridden, change the 
-                //  method to point to the wrapper method
-                var originalReceiver = ((BoundMethodGroup)originalArgument).ReceiverOpt;
-                var newReceiver = ((BoundMethodGroup)rewrittenArgument).ReceiverOpt;
-                if (BaseReferenceInReceiverWasRewritten(originalReceiver, newReceiver) && method.IsMetadataVirtual())
+
+            // if the original receiver was BoundKind.BaseReference (i.e. from a method group)
+            // and the receiver is overridden, change the method to point to a wrapper method
+            if (BaseReferenceInReceiverWasRewritten(originalArgument, rewrittenArgument) && method.IsMetadataVirtual())
             {
                 method = GetMethodWrapperForBaseNonVirtualCall(method, originalArgument.Syntax);
-                    // NOTE: we substitute the method in rewritten bound delegate 
-                    //       creation node, but leave the method group unchanged
-                }
             }
             method = VisitMethodSymbol(method);
             TypeSymbol type = this.VisitType(node.Type);
@@ -657,64 +639,6 @@ private static bool BaseReferenceInReceiverWasRewritten(BoundExpression original
                    rewrittenReceiver != null && rewrittenReceiver.Kind != BoundKind.BaseReference;
         }
 
-        private BoundNode RewriteMethodGroupConversion(BoundConversion conversion)
-        {
-            // in a method group conversion, we may need to rewrite the selected method
-            BoundMethodGroup operand = (BoundMethodGroup)conversion.Operand;
-            BoundExpression originalReceiverOpt = operand.ReceiverOpt;
-            BoundExpression receiverOpt;
-
-            if (originalReceiverOpt == null)
-            {
-                receiverOpt = null;
-            }
-            else if (!conversion.IsExtensionMethod && conversion.SymbolOpt.IsStatic)
-            {
-                receiverOpt = new BoundTypeExpression(originalReceiverOpt.Syntax, null, VisitType(originalReceiverOpt.Type));
-            }
-            else
-            {
-                receiverOpt = (BoundExpression)Visit(originalReceiverOpt);
-            }
-
-            TypeSymbol type = this.VisitType(conversion.Type);
-
-            MethodSymbol method = conversion.SymbolOpt;
-
-            //  if the original receiver was a base access and is was rewritten, 
-            //  change the method to point to the wrapper method
-            if (BaseReferenceInReceiverWasRewritten(originalReceiverOpt, receiverOpt) && method.IsMetadataVirtual())
-            {
-                method = GetMethodWrapperForBaseNonVirtualCall(method, conversion.Syntax);
-            }
-
-            method = VisitMethodSymbol(method);
-            operand = operand.Update(
-                TypeMap.SubstituteTypesWithoutModifiers(operand.TypeArgumentsOpt),
-                method.Name,
-                operand.Methods,
-                operand.LookupSymbolOpt,
-                operand.LookupError,
-                operand.Flags,
-                receiverOpt,
-                operand.ResultKind);
-
-            var conversionInfo = conversion.Conversion;
-            if (conversionInfo.Method != (object)method)
-            {
-                conversionInfo = conversionInfo.SetConversionMethod(method);
-            }
-
-            return conversion.Update(
-                operand,
-                conversionInfo,
-                isBaseConversion: conversion.IsBaseConversion,
-                @checked: conversion.Checked,
-                explicitCastInCode: conversion.ExplicitCastInCode,
-                constantValueOpt: conversion.ConstantValueOpt,
-                type: type);
-        }
-
         /// <summary>
         /// A wrapper method that is created for non-virtually calling a base-class 
         /// virtual method from other classes (like those created for lambdas...).

src/ExpressionEvaluator/CSharp/Source/ExpressionCompiler/Rewriters/CapturedVariableRewriter.cs

@@ -70,27 +70,7 @@ public override BoundNode VisitParameter(BoundParameter node)
 
         public override BoundNode VisitMethodGroup(BoundMethodGroup node)
         {
-            if ((node.Flags & BoundMethodGroupFlags.HasImplicitReceiver) == BoundMethodGroupFlags.HasImplicitReceiver &&
-                (object)_targetMethodThisParameter == null)
-            {
-                // This can happen in static contexts.
-                // NOTE: LocalRewriter has already been run, so the receiver has already been replaced with an
-                // appropriate type expression, if this is a static context.
-                // NOTE: Don't go through VisitThisReference, because it'll produce a diagnostic.
-                return node.Update(
-                    node.TypeArgumentsOpt,
-                    node.Name,
-                    node.Methods,
-                    node.LookupSymbolOpt,
-                    node.LookupError,
-                    node.Flags,
-                    receiverOpt: null,
-                    resultKind: node.ResultKind);
-            }
-            else
-            {
-                return base.VisitMethodGroup(node);
-            }
+            throw ExceptionUtilities.Unreachable;
         }
 
         public override BoundNode VisitThisReference(BoundThisReference node)