Fix override implementation with generics and constraints (#76223)

Author: CyrusNajmabadi

src/Analyzers/CSharp/CodeFixes/ImplementAbstractClass/CSharpImplementAbstractClassCodeFixProvider.cs

@@ -12,18 +12,13 @@ namespace Microsoft.CodeAnalysis.CSharp.ImplementAbstractClass;
 
 [ExportCodeFixProvider(LanguageNames.CSharp, Name = PredefinedCodeFixProviderNames.ImplementAbstractClass), Shared]
 [ExtensionOrder(After = PredefinedCodeFixProviderNames.GenerateType)]
-internal sealed class CSharpImplementAbstractClassCodeFixProvider :
-    AbstractImplementAbstractClassCodeFixProvider<TypeDeclarationSyntax>
+[method: ImportingConstructor]
+[method: SuppressMessage("RoslynDiagnosticsReliability", "RS0033:Importing constructor should be [Obsolete]", Justification = "Used in test code: https://github.com/dotnet/roslyn/issues/42814")]
+internal sealed class CSharpImplementAbstractClassCodeFixProvider()
+    : AbstractImplementAbstractClassCodeFixProvider<TypeDeclarationSyntax>(CS0534)
 {
     private const string CS0534 = nameof(CS0534); // 'Program' does not implement inherited abstract member 'Goo.bar()'
 
-    [ImportingConstructor]
-    [SuppressMessage("RoslynDiagnosticsReliability", "RS0033:Importing constructor should be [Obsolete]", Justification = "Used in test code: https://github.com/dotnet/roslyn/issues/42814")]
-    public CSharpImplementAbstractClassCodeFixProvider()
-        : base(CS0534)
-    {
-    }
-
     protected override SyntaxToken GetClassIdentifier(TypeDeclarationSyntax classNode)
         => classNode.Identifier;
 }

src/Analyzers/CSharp/Tests/ImplementAbstractClass/ImplementAbstractClassTests.cs

@@ -2,6 +2,7 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 // See the LICENSE file in the project root for more information.
 
+using System.Diagnostics.CodeAnalysis;
 using System.Threading.Tasks;
 using Microsoft.CodeAnalysis.CodeFixes;
 using Microsoft.CodeAnalysis.CodeStyle;
@@ -38,8 +39,8 @@ private OptionsCollection AllOptionsOff
         };
 
     internal Task TestAllOptionsOffAsync(
-        string initialMarkup,
-        string expectedMarkup,
+        [StringSyntax(PredefinedEmbeddedLanguageNames.CSharpTest)] string initialMarkup,
+        [StringSyntax(PredefinedEmbeddedLanguageNames.CSharpTest)] string expectedMarkup,
         int index = 0,
         OptionsCollection? options = null,
         ParseOptions? parseOptions = null)
@@ -2553,4 +2554,180 @@ protected override void M()
             }
             """);
     }
+
+    [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/71225")]
+    public async Task TestConstrainedTypeParameter1()
+    {
+        await TestAllOptionsOffAsync(
+            """
+            #nullable enable
+            using System;
+
+            interface I<out T> { }
+
+            class C { }
+
+            abstract class Problem
+            {
+                protected abstract void M<T>(I<T?> i) where T : C;
+            }
+
+            class [|Bad|] : Problem
+            {
+            }
+            """,
+            """
+            #nullable enable
+            using System;
+
+            interface I<out T> { }
+            
+            class C { }
+            
+            abstract class Problem
+            {
+                protected abstract void M<T>(I<T?> i) where T : C;
+            }
+            
+            class Bad : Problem
+            {
+                protected override void M<T>(I<T?> i) where T : class
+                {
+                    throw new NotImplementedException();
+                }
+            }
+            """);
+    }
+
+    [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/71225")]
+    public async Task TestConstrainedTypeParameter2()
+    {
+        await TestAllOptionsOffAsync(
+            """
+            #nullable enable
+            using System;
+
+            interface I<out T> { }
+
+            class C { }
+
+            abstract class Problem<U>
+            {
+                protected abstract void M<T>(I<T?> i) where T : U;
+            }
+
+            class [|Bad|] : Problem<int>
+            {
+            }
+            """,
+            """
+            #nullable enable
+            using System;
+
+            interface I<out T> { }
+            
+            class C { }
+            
+            abstract class Problem<U>
+            {
+                protected abstract void M<T>(I<T?> i) where T : U;
+            }
+            
+            class Bad : Problem<int>
+            {
+                protected override void M<T>(I<T> i) where T : struct
+                {
+                    throw new NotImplementedException();
+                }
+            }
+            """);
+    }
+
+    [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/71225")]
+    public async Task TestConstrainedTypeParameter3()
+    {
+        await TestAllOptionsOffAsync(
+            """
+            #nullable enable
+            using System;
+
+            interface I<out T> { }
+
+            class C { }
+
+            abstract class Problem<U>
+            {
+                protected abstract void M<T>(I<T?> i) where T : U;
+            }
+
+            class [|Bad|] : Problem<string>
+            {
+            }
+            """,
+            """
+            #nullable enable
+            using System;
+
+            interface I<out T> { }
+            
+            class C { }
+            
+            abstract class Problem<U>
+            {
+                protected abstract void M<T>(I<T?> i) where T : U;
+            }
+            
+            class Bad : Problem<string>
+            {
+                protected override void M<T>(I<T?> i) where T : class
+                {
+                    throw new NotImplementedException();
+                }
+            }
+            """);
+    }
+
+    [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/71225")]
+    public async Task TestConstrainedTypeParameter4()
+    {
+        await TestAllOptionsOffAsync(
+            """
+            #nullable enable
+            using System;
+
+            interface I<out T> { }
+
+            class C { }
+
+            abstract class Problem<U>
+            {
+                protected abstract void M<T>(I<T?> i) where T : U;
+            }
+
+            class [|Bad|] : Problem<int[]>
+            {
+            }
+            """,
+            """
+            #nullable enable
+            using System;
+
+            interface I<out T> { }
+            
+            class C { }
+            
+            abstract class Problem<U>
+            {
+                protected abstract void M<T>(I<T?> i) where T : U;
+            }
+            
+            class Bad : Problem<int[]>
+            {
+                protected override void M<T>(I<T?> i) where T : class
+                {
+                    throw new NotImplementedException();
+                }
+            }
+            """);
+    }
 }

src/Analyzers/Core/CodeFixes/ImplementAbstractClass/AbstractImplementAbstractClassCodeFixProvider.cs

@@ -49,7 +49,7 @@ public sealed override async Task RegisterCodeFixesAsync(CodeFixContext context)
         context.RegisterCodeFix(
             CodeAction.Create(
                 AnalyzersResources.Implement_abstract_class,
-                c => data.ImplementAbstractClassAsync(throughMember: null, canDelegateAllMembers: null, c),
+                cancellationToken => data.ImplementAbstractClassAsync(throughMember: null, canDelegateAllMembers: null, cancellationToken),
                 id),
             context.Diagnostics);
 
@@ -63,7 +63,7 @@ public sealed override async Task RegisterCodeFixesAsync(CodeFixContext context)
             context.RegisterCodeFix(
                 CodeAction.Create(
                     string.Format(AnalyzersResources.Implement_through_0, through.Name),
-                    c => data.ImplementAbstractClassAsync(through, canDelegateAllMembers, c),
+                    cancellationToken => data.ImplementAbstractClassAsync(through, canDelegateAllMembers, cancellationToken),
                     id),
                 context.Diagnostics);
         }

src/Workspaces/SharedUtilitiesAndExtensions/Workspace/CSharp/CodeGeneration/MethodGenerator.cs

@@ -257,12 +257,7 @@ private static SyntaxList<TypeParameterConstraintClauseSyntax> GenerateDefaultCo
             if (!referencedTypeParameters.Contains(typeParameter))
                 continue;
 
-            var constraint = typeParameter switch
-            {
-                { HasReferenceTypeConstraint: true } => s_classConstraint,
-                { HasValueTypeConstraint: true } => s_structConstraint,
-                _ => s_defaultConstraint
-            };
+            var constraint = GetConstraint(typeParameter);
 
             listOfClauses.Add(TypeParameterConstraintClause(
                 typeParameter.Name.ToIdentifierName(),
@@ -272,6 +267,52 @@ private static SyntaxList<TypeParameterConstraintClauseSyntax> GenerateDefaultCo
         return [.. listOfClauses];
     }
 
+    private static TypeParameterConstraintSyntax GetConstraint(ITypeParameterSymbol typeParameter)
+    {
+        using var _ = PooledHashSet<ITypeParameterSymbol>.GetInstance(out var visited);
+        var constraint = GetConstraintRecursive(typeParameter);
+
+        return constraint ?? s_defaultConstraint;
+
+        TypeParameterConstraintSyntax? GetConstraintRecursive(ITypeParameterSymbol typeParameter)
+        {
+            if (visited.Add(typeParameter))
+            {
+                // If it is explicitly marked as `T : struct` or `T : class` then we want to have the same constraint on the override.
+                if (typeParameter.HasValueTypeConstraint)
+                    return s_structConstraint;
+
+                if (typeParameter.HasReferenceTypeConstraint)
+                    return s_classConstraint;
+
+                foreach (var constraintType in typeParameter.ConstraintTypes)
+                {
+                    // If we ended up being constrained on a value type, then we have to have the `T : struct`
+                    // constraint to align with that.
+                    if (constraintType.IsValueType)
+                        return s_structConstraint;
+
+                    // For all reference types *except* interfaces, we want the `T : class` constraint.  An interface
+                    // can be implemented by a value type or a referernce type, so it adds no information to the
+                    // constraints.
+                    if (constraintType.IsReferenceType && constraintType.TypeKind != TypeKind.Interface)
+                        return s_classConstraint;
+
+                    // If we have `where T : U` then peek into the other contraint to see if it adds information.
+                    if (constraintType is ITypeParameterSymbol constraintTypeParameter)
+                    {
+                        var constraint = GetConstraintRecursive(constraintTypeParameter);
+                        if (constraint != null)
+                            return constraint;
+                    }
+                }
+            }
+
+            // We learned nothing from this constraint.
+            return null;
+        }
+    }
+
     private static TypeParameterListSyntax? GenerateTypeParameterList(
         IMethodSymbol method, CSharpCodeGenerationContextInfo info)
     {