Adjust Better Betterness rules around method group conversions in order to...

Adjust Better Betterness rules around method group conversions in order to restore backward compatibility.

Fixes #6560.

src/Compilers/CSharp/Portable/Binder/Binder_Conversions.cs

@@ -555,7 +555,7 @@ private static bool WasImplicitReceiver(BoundExpression receiverOpt)
         /// <summary>
         /// This method implements the checks in spec section 15.2.
         /// </summary>
-        private bool MethodGroupIsCompatibleWithDelegate(BoundExpression receiverOpt, bool isExtensionMethod, MethodSymbol method, NamedTypeSymbol delegateType, Location errorLocation, DiagnosticBag diagnostics)
+        internal bool MethodGroupIsCompatibleWithDelegate(BoundExpression receiverOpt, bool isExtensionMethod, MethodSymbol method, NamedTypeSymbol delegateType, Location errorLocation, DiagnosticBag diagnostics)
         {
             Debug.Assert(delegateType.TypeKind == TypeKind.Delegate);
             Debug.Assert((object)delegateType.DelegateInvokeMethod != null && !delegateType.DelegateInvokeMethod.HasUseSiteError,
@@ -613,22 +613,6 @@ private bool MethodGroupIsCompatibleWithDelegate(BoundExpression receiverOpt, bo
             }
 
             diagnostics.Add(errorLocation, useSiteDiagnostics);
-
-            if (method.IsConditional)
-            {
-                // CS1618: Cannot create delegate with '{0}' because it has a Conditional attribute
-                Error(diagnostics, ErrorCode.ERR_DelegateOnConditional, errorLocation, method);
-                return false;
-            }
-
-            var sourceMethod = method as SourceMemberMethodSymbol;
-            if ((object)sourceMethod != null && sourceMethod.IsPartialWithoutImplementation)
-            {
-                // CS0762: Cannot create delegate from method '{0}' because it is a partial method without an implementing declaration
-                Error(diagnostics, ErrorCode.ERR_PartialMethodToDelegate, errorLocation, method);
-                return false;
-            }
-
             return true;
         }
 
@@ -660,6 +644,21 @@ private bool MethodGroupIsCompatibleWithDelegate(BoundExpression receiverOpt, bo
                 return true;
             }
 
+            if (selectedMethod.IsConditional)
+            {
+                // CS1618: Cannot create delegate with '{0}' because it has a Conditional attribute
+                Error(diagnostics, ErrorCode.ERR_DelegateOnConditional, syntax.Location, selectedMethod);
+                return true;
+            }
+
+            var sourceMethod = selectedMethod as SourceMemberMethodSymbol;
+            if ((object)sourceMethod != null && sourceMethod.IsPartialWithoutImplementation)
+            {
+                // CS0762: Cannot create delegate from method '{0}' because it is a partial method without an implementing declaration
+                Error(diagnostics, ErrorCode.ERR_PartialMethodToDelegate, syntax.Location, selectedMethod);
+                return true;
+            }
+
             if (selectedMethod.HasUnsafeParameter() || selectedMethod.ReturnType.IsUnsafe())
             {
                 return ReportUnsafeIfNotAllowed(syntax, diagnostics);

src/Compilers/CSharp/Portable/Binder/Semantics/OverloadResolution/OverloadResolution.cs

@@ -1807,7 +1807,7 @@ private BetterResult BetterConversionFromExpression(BoundExpression node, TypeSy
             }
 
             // - T1 is a better conversion target than T2
-            return BetterConversionTarget(lambdaOpt, t1, t2, ref useSiteDiagnostics, out okToDowngradeToNeither);
+            return BetterConversionTarget(node, t1, conv1, t2, conv2, ref useSiteDiagnostics, out okToDowngradeToNeither);
         }
 
         private bool ExpressionMatchExactly(BoundExpression node, TypeSymbol t, ref HashSet<DiagnosticInfo> useSiteDiagnostics)
@@ -1946,7 +1946,13 @@ public override BoundNode VisitReturnStatement(BoundReturnStatement node)
             }
         }
 
-        private BetterResult BetterConversionTarget(UnboundLambda lambdaOpt, TypeSymbol type1, TypeSymbol type2, ref HashSet<DiagnosticInfo> useSiteDiagnostics, out bool okToDowngradeToNeither)
+        private BetterResult BetterConversionTarget(TypeSymbol type1, TypeSymbol type2, ref HashSet<DiagnosticInfo> useSiteDiagnostics)
+        {
+            bool okToDowngradeToNeither;
+            return BetterConversionTarget(null, type1, default(Conversion), type2, default(Conversion), ref useSiteDiagnostics, out okToDowngradeToNeither);
+        }
+
+        private BetterResult BetterConversionTarget(BoundExpression node, TypeSymbol type1, Conversion conv1, TypeSymbol type2, Conversion conv2, ref HashSet<DiagnosticInfo> useSiteDiagnostics, out bool okToDowngradeToNeither)
         {
             okToDowngradeToNeither = false;
 
@@ -1960,6 +1966,7 @@ private BetterResult BetterConversionTarget(UnboundLambda lambdaOpt, TypeSymbol
             // and at least one of the following holds:
             bool type1ToType2 = Conversions.ClassifyImplicitConversion(type1, type2, ref useSiteDiagnostics).IsImplicit;
             bool type2ToType1 = Conversions.ClassifyImplicitConversion(type2, type1, ref useSiteDiagnostics).IsImplicit;
+            UnboundLambda lambdaOpt = node as UnboundLambda;
 
             if (type1ToType2)
             {
@@ -1980,7 +1987,6 @@ private BetterResult BetterConversionTarget(UnboundLambda lambdaOpt, TypeSymbol
                 return BetterResult.Right;
             }
 
-            bool ignore;
             var task_T = Compilation.GetWellKnownType(WellKnownType.System_Threading_Tasks_Task_T);
 
             if ((object)task_T != null)
@@ -1990,11 +1996,9 @@ private BetterResult BetterConversionTarget(UnboundLambda lambdaOpt, TypeSymbol
                     if (type2.OriginalDefinition == task_T)
                     {
                         // - T1 is Task<S1>, T2 is Task<S2>, and S1 is a better conversion target than S2
-                        return BetterConversionTarget(null,
-                                                      ((NamedTypeSymbol)type1).TypeArgumentsNoUseSiteDiagnostics[0],
+                        return BetterConversionTarget(((NamedTypeSymbol)type1).TypeArgumentsNoUseSiteDiagnostics[0],
                                                       ((NamedTypeSymbol)type2).TypeArgumentsNoUseSiteDiagnostics[0],
-                                                      ref useSiteDiagnostics,
-                                                      out ignore);
+                                                      ref useSiteDiagnostics);
                     }
 
                     // A shortcut, Task<T> type cannot satisfy other rules.
@@ -2025,23 +2029,49 @@ private BetterResult BetterConversionTarget(UnboundLambda lambdaOpt, TypeSymbol
                     {
                         TypeSymbol r1 = invoke1.ReturnType;
                         TypeSymbol r2 = invoke2.ReturnType;
+                        BetterResult delegateResult = BetterResult.Neither;
 
                         if (r1.SpecialType != SpecialType.System_Void)
                         {
                             if (r2.SpecialType == SpecialType.System_Void)
                             {
                                 // - D2 is void returning
-                                return BetterResult.Left;
+                                delegateResult = BetterResult.Left;
                             }
                         }
                         else if (r2.SpecialType != SpecialType.System_Void)
                         {
                             // - D2 is void returning
-                            return BetterResult.Right;
+                            delegateResult = BetterResult.Right;
+                        }
+
+                        if (delegateResult == BetterResult.Neither)
+                        {
+                            //  - D2 has a return type S2, and S1 is a better conversion target than S2
+                            delegateResult = BetterConversionTarget(r1, r2, ref useSiteDiagnostics);
+                        }
+
+                        // Downgrade result to Neither if conversion used by the winner isn't actually valid method group conversion.
+                        // This is necessary to preserve compatibility, otherwise we might dismiss "worse", but trully applicable candidate
+                        // based on a "better", but, in reality, erroneous one.
+                        if (node?.Kind == BoundKind.MethodGroup)
+                        {
+                            var group = (BoundMethodGroup)node;
+
+                            if (delegateResult == BetterResult.Left)
+                            {
+                                if (IsMethodGroupConversionIncompatibleWithDelegate(group, d1, conv1))
+                                {
+                                    return BetterResult.Neither;
+                                }
+                            }
+                            else if (delegateResult == BetterResult.Right && IsMethodGroupConversionIncompatibleWithDelegate(group, d2, conv2))
+                            {
+                                return BetterResult.Neither;
+                            }
                         }
 
-                        //  - D2 has a return type S2, and S1 is a better conversion target than S2
-                        return BetterConversionTarget(null, r1, r2, ref useSiteDiagnostics, out ignore);
+                        return delegateResult;
                     }
                 }
 
@@ -2074,6 +2104,19 @@ private BetterResult BetterConversionTarget(UnboundLambda lambdaOpt, TypeSymbol
             return BetterResult.Neither;
         }
 
+        private bool IsMethodGroupConversionIncompatibleWithDelegate(BoundMethodGroup node, NamedTypeSymbol delegateType, Conversion conv)
+        {
+            if (conv.IsMethodGroup)
+            {
+                DiagnosticBag ignore = DiagnosticBag.GetInstance();
+                bool result = !_binder.MethodGroupIsCompatibleWithDelegate(node.ReceiverOpt, conv.IsExtensionMethod, conv.Method, delegateType, Location.None, ignore);
+                ignore.Free();
+                return result;
+            }
+
+            return false;
+        }
+
         private bool CanDowngradeConversionFromLambdaToNeither(BetterResult currentResult, UnboundLambda lambda, TypeSymbol type1, TypeSymbol type2, ref HashSet<DiagnosticInfo> useSiteDiagnostics, bool fromTypeAnalysis)
         {
             // DELIBERATE SPEC VIOLATION: See bug 11961.
@@ -2149,7 +2192,6 @@ private bool CanDowngradeConversionFromLambdaToNeither(BetterResult currentResul
 #if DEBUG
                         if (fromTypeAnalysis)
                         {
-                            bool ignore;
                             // Since we are dealing with variance delegate conversion and delegates have identical parameter
                             // lists, return types must be implicitly convertible in the same direction.
                             // Or we might be dealing with error return types and we may have one error delegate matching exactly
@@ -2157,7 +2199,7 @@ private bool CanDowngradeConversionFromLambdaToNeither(BetterResult currentResul
                             Debug.Assert(
                                 r1.IsErrorType() ||
                                 r2.IsErrorType() ||
-                                currentResult == BetterConversionTarget(null, r1, r2, ref useSiteDiagnostics, out ignore));
+                                currentResult == BetterConversionTarget(r1, r2, ref useSiteDiagnostics));
                         }
 #endif
                     }

src/Compilers/CSharp/Test/Semantic/Semantics/OverloadResolutionTests.cs

@@ -472,7 +472,7 @@ static void Main()
         }
 
         [Fact]
-        public void BetterDelegateType()
+        public void BetterDelegateType_01()
         {
             string source1 = @"
 using System;
@@ -520,6 +520,61 @@ static void Main()
                 );
         }
 
+        [Fact, WorkItem(6560, "https://github.com/dotnet/roslyn/issues/6560")]
+        public void BetterDelegateType_02()
+        {
+            string source1 = @"
+using System;
+
+class C
+{
+    public static void Main()
+    {
+        Run1(() => MethodReturnsVoid());
+        Run1(MethodReturnsVoid);
+        Run2(() => MethodReturnsVoid());
+        Run2(MethodReturnsVoid);
+    }
+
+    public static object Run1(Action action)
+    {
+        Console.WriteLine(""Run1(Action action)"");
+        action();
+        return null;
+    }
+
+    public static object Run1(Func<object> action, bool optional = false)
+    {
+        Console.WriteLine(""Run1(Func<object> action, bool optional = false)"");
+        return action();
+    }
+
+    public static object Run2(Func<object> action, bool optional = false)
+    {
+        Console.WriteLine(""Run2(Func<object> action, bool optional = false)"");
+        return action();
+    }
+
+    public static object Run2(Action action)
+    {
+        Console.WriteLine(""Run2(Action action)"");
+        action();
+        return null;
+    }
+
+    private static void MethodReturnsVoid()
+    {
+    }
+}
+";
+
+            CompileAndVerify(source1, expectedOutput:
+@"Run1(Action action)
+Run1(Action action)
+Run2(Action action)
+Run2(Action action)");
+        }
+
         [Fact]
         public void TestBug9851()
         {