From 8c15136bf69568543bcbee4fe8025ff885b3970e Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Sun, 1 Dec 2024 22:42:17 -0800 Subject: [PATCH] Fix override implementation with generics and constraints --- ...rpImplementAbstractClassCodeFixProvider.cs | 13 +- .../ImplementAbstractClassTests.cs | 181 +++++++++++++++++- ...ctImplementAbstractClassCodeFixProvider.cs | 4 +- .../CSharp/CodeGeneration/MethodGenerator.cs | 53 ++++- 4 files changed, 232 insertions(+), 19 deletions(-) diff --git a/src/Analyzers/CSharp/CodeFixes/ImplementAbstractClass/CSharpImplementAbstractClassCodeFixProvider.cs b/src/Analyzers/CSharp/CodeFixes/ImplementAbstractClass/CSharpImplementAbstractClassCodeFixProvider.cs index aa7db53519afc..fb3597fb90917 100644 --- a/src/Analyzers/CSharp/CodeFixes/ImplementAbstractClass/CSharpImplementAbstractClassCodeFixProvider.cs +++ b/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 +[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(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; } diff --git a/src/Analyzers/CSharp/Tests/ImplementAbstractClass/ImplementAbstractClassTests.cs b/src/Analyzers/CSharp/Tests/ImplementAbstractClass/ImplementAbstractClassTests.cs index bc4c5048dcc2b..39282c7ab2fa1 100644 --- a/src/Analyzers/CSharp/Tests/ImplementAbstractClass/ImplementAbstractClassTests.cs +++ b/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 { } + + class C { } + + abstract class Problem + { + protected abstract void M(I i) where T : C; + } + + class [|Bad|] : Problem + { + } + """, + """ + #nullable enable + using System; + + interface I { } + + class C { } + + abstract class Problem + { + protected abstract void M(I i) where T : C; + } + + class Bad : Problem + { + protected override void M(I 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 { } + + class C { } + + abstract class Problem + { + protected abstract void M(I i) where T : U; + } + + class [|Bad|] : Problem + { + } + """, + """ + #nullable enable + using System; + + interface I { } + + class C { } + + abstract class Problem + { + protected abstract void M(I i) where T : U; + } + + class Bad : Problem + { + protected override void M(I 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 { } + + class C { } + + abstract class Problem + { + protected abstract void M(I i) where T : U; + } + + class [|Bad|] : Problem + { + } + """, + """ + #nullable enable + using System; + + interface I { } + + class C { } + + abstract class Problem + { + protected abstract void M(I i) where T : U; + } + + class Bad : Problem + { + protected override void M(I 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 { } + + class C { } + + abstract class Problem + { + protected abstract void M(I i) where T : U; + } + + class [|Bad|] : Problem + { + } + """, + """ + #nullable enable + using System; + + interface I { } + + class C { } + + abstract class Problem + { + protected abstract void M(I i) where T : U; + } + + class Bad : Problem + { + protected override void M(I i) where T : class + { + throw new NotImplementedException(); + } + } + """); + } } diff --git a/src/Analyzers/Core/CodeFixes/ImplementAbstractClass/AbstractImplementAbstractClassCodeFixProvider.cs b/src/Analyzers/Core/CodeFixes/ImplementAbstractClass/AbstractImplementAbstractClassCodeFixProvider.cs index 89b813f459da6..d2be95747ef31 100644 --- a/src/Analyzers/Core/CodeFixes/ImplementAbstractClass/AbstractImplementAbstractClassCodeFixProvider.cs +++ b/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); } diff --git a/src/Workspaces/SharedUtilitiesAndExtensions/Workspace/CSharp/CodeGeneration/MethodGenerator.cs b/src/Workspaces/SharedUtilitiesAndExtensions/Workspace/CSharp/CodeGeneration/MethodGenerator.cs index b907257c74beb..99ac7b7368731 100644 --- a/src/Workspaces/SharedUtilitiesAndExtensions/Workspace/CSharp/CodeGeneration/MethodGenerator.cs +++ b/src/Workspaces/SharedUtilitiesAndExtensions/Workspace/CSharp/CodeGeneration/MethodGenerator.cs @@ -233,12 +233,7 @@ private static SyntaxList 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(), @@ -248,6 +243,52 @@ private static SyntaxList GenerateDefaultCo return [.. listOfClauses]; } + private static TypeParameterConstraintSyntax GetConstraint(ITypeParameterSymbol typeParameter) + { + using var _ = PooledHashSet.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) {