From 0bc834424d7bc82264aee4a230b4409ec38d0a57 Mon Sep 17 00:00:00 2001 From: Maciej Tromiczak Date: Thu, 17 Mar 2022 10:59:43 +0100 Subject: [PATCH] Apply 'require accessibility modifiers' setting on interface methods (#47448) --- .../CSharpAddAccessibilityModifiers.cs | 99 ++++++++++------- ...ccessibilityModifiersDiagnosticAnalyzer.cs | 12 +-- .../AddAccessibilityModifiersTests.cs | 101 ++++++++++++++++++ .../AddAccessibilityModifiersHelpers.cs | 3 + 4 files changed, 166 insertions(+), 49 deletions(-) diff --git a/src/Analyzers/CSharp/Analyzers/AddAccessibilityModifiers/CSharpAddAccessibilityModifiers.cs b/src/Analyzers/CSharp/Analyzers/AddAccessibilityModifiers/CSharpAddAccessibilityModifiers.cs index 402a4ecc0e428..e2038857e4a07 100644 --- a/src/Analyzers/CSharp/Analyzers/AddAccessibilityModifiers/CSharpAddAccessibilityModifiers.cs +++ b/src/Analyzers/CSharp/Analyzers/AddAccessibilityModifiers/CSharpAddAccessibilityModifiers.cs @@ -37,51 +37,74 @@ public override bool ShouldUpdateAccessibilityModifier( // This analyzer bases all of its decisions on the accessibility var accessibility = accessibilityFacts.GetAccessibility(member); - // Omit will flag any accessibility values that exist and are default - // The other options will remove or ignore accessibility - var isOmit = option == AccessibilityModifiersRequired.OmitIfDefault; + if (accessibility == Accessibility.NotApplicable) + { + return ShouldAddAccessibilityModifier(member, option); + } + else + { + return ShouldRemoveAccessibilityModifier(member, accessibility, option); + } + } - if (isOmit) + private static bool ShouldAddAccessibilityModifier(MemberDeclarationSyntax member, + AccessibilityModifiersRequired option) + { + if (member.Parent.IsKind(SyntaxKind.InterfaceDeclaration)) { - if (accessibility == Accessibility.NotApplicable) - return false; + return option == AccessibilityModifiersRequired.Always; + } + + return option != AccessibilityModifiersRequired.OmitIfDefault; + } - var parentKind = member.GetRequiredParent().Kind(); - switch (parentKind) + private static bool ShouldRemoveAccessibilityModifier(MemberDeclarationSyntax member, + Accessibility accessibility, + AccessibilityModifiersRequired option) + { + if (member.Parent.IsKind(SyntaxKind.InterfaceDeclaration)) + { + if (option == AccessibilityModifiersRequired.Always || accessibility != Accessibility.Public) { - // Check for default modifiers in namespace and outside of namespace - case SyntaxKind.CompilationUnit: - case SyntaxKind.FileScopedNamespaceDeclaration: - case SyntaxKind.NamespaceDeclaration: - { - // Default is internal - if (accessibility != Accessibility.Internal) - return false; - } - - break; - - case SyntaxKind.ClassDeclaration: - case SyntaxKind.RecordDeclaration: - case SyntaxKind.StructDeclaration: - case SyntaxKind.RecordStructDeclaration: - { - // Inside a type, default is private - if (accessibility != Accessibility.Private) - return false; - } - - break; - - default: - return false; // Unknown parent kind, don't do anything + return false; } + + return true; } - else + else if (option != AccessibilityModifiersRequired.OmitIfDefault) { - // Mode is always, so we have to flag missing modifiers - if (accessibility != Accessibility.NotApplicable) - return false; + return false; + } + + var parentKind = member.GetRequiredParent().Kind(); + switch (parentKind) + { + // Check for default modifiers in namespace and outside of namespace + case SyntaxKind.CompilationUnit: + case SyntaxKind.FileScopedNamespaceDeclaration: + case SyntaxKind.NamespaceDeclaration: + { + // Default is internal + if (accessibility != Accessibility.Internal) + return false; + } + + break; + + case SyntaxKind.ClassDeclaration: + case SyntaxKind.RecordDeclaration: + case SyntaxKind.StructDeclaration: + case SyntaxKind.RecordStructDeclaration: + { + // Inside a type, default is private + if (accessibility != Accessibility.Private) + return false; + } + + break; + + default: + return false; // Unknown parent kind, don't do anything } return true; diff --git a/src/Analyzers/CSharp/Analyzers/AddAccessibilityModifiers/CSharpAddAccessibilityModifiersDiagnosticAnalyzer.cs b/src/Analyzers/CSharp/Analyzers/AddAccessibilityModifiers/CSharpAddAccessibilityModifiersDiagnosticAnalyzer.cs index 2c427e2d3adca..eb5667c5d0b6f 100644 --- a/src/Analyzers/CSharp/Analyzers/AddAccessibilityModifiers/CSharpAddAccessibilityModifiersDiagnosticAnalyzer.cs +++ b/src/Analyzers/CSharp/Analyzers/AddAccessibilityModifiers/CSharpAddAccessibilityModifiersDiagnosticAnalyzer.cs @@ -43,23 +43,13 @@ private void ProcessMemberDeclaration( // If we have a class or struct, recurse inwards. if (member.IsKind(SyntaxKind.ClassDeclaration, out TypeDeclarationSyntax? typeDeclaration) || member.IsKind(SyntaxKind.StructDeclaration, out typeDeclaration) || + member.IsKind(SyntaxKind.InterfaceDeclaration, out typeDeclaration) || member.IsKind(SyntaxKind.RecordDeclaration, out typeDeclaration) || member.IsKind(SyntaxKind.RecordStructDeclaration, out typeDeclaration)) { ProcessMembers(context, option, typeDeclaration.Members); } -#if false - // Add this once we have the language version for C# that supports accessibility - // modifiers on interface methods. - if (option.Value == AccessibilityModifiersRequired.Always && - member.IsKind(SyntaxKind.InterfaceDeclaration, out typeDeclaration)) - { - // Only recurse into an interface if the user wants accessibility modifiers on - ProcessTypeDeclaration(context, generator, option, typeDeclaration); - } -#endif - if (!CSharpAddAccessibilityModifiers.Instance.ShouldUpdateAccessibilityModifier(CSharpAccessibilityFacts.Instance, member, option.Value, out var name)) return; diff --git a/src/Analyzers/CSharp/Tests/AddAccessibilityModifiers/AddAccessibilityModifiersTests.cs b/src/Analyzers/CSharp/Tests/AddAccessibilityModifiers/AddAccessibilityModifiersTests.cs index 29134c5de0c6b..717a97b665764 100644 --- a/src/Analyzers/CSharp/Tests/AddAccessibilityModifiers/AddAccessibilityModifiersTests.cs +++ b/src/Analyzers/CSharp/Tests/AddAccessibilityModifiers/AddAccessibilityModifiersTests.cs @@ -611,5 +611,106 @@ internal class C : I await test.RunAsync(); } + + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsAddAccessibilityModifiers)] + [WorkItem(47448, "https://github.com/dotnet/roslyn/issues/47448")] + public async Task TestModifierOnInterfaceMethods_01() + { + var source = @"public interface I +{ + void M(); +}"; + + var test = new VerifyCS.Test + { + TestCode = source, + FixedCode = source, + Options = + { + { CodeStyleOptions2.RequireAccessibilityModifiers, AccessibilityModifiersRequired.ForNonInterfaceMembers }, + } + }; + + await test.RunAsync(); + } + + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsAddAccessibilityModifiers)] + [WorkItem(47448, "https://github.com/dotnet/roslyn/issues/47448")] + public async Task TestModifierOnInterfaceMethods_02() + { + var source = @"public interface I +{ + void [|M|](); +}"; + var expected = @"public interface I +{ + public void M(); +}"; + + var test = new VerifyCS.Test + { + TestCode = source, + FixedCode = expected, + Options = + { + { CodeStyleOptions2.RequireAccessibilityModifiers, AccessibilityModifiersRequired.Always }, + } + }; + + await test.RunAsync(); + } + + [Theory, Trait(Traits.Feature, Traits.Features.CodeActionsAddAccessibilityModifiers)] + [WorkItem(47448, "https://github.com/dotnet/roslyn/issues/47448")] + [InlineData((int)AccessibilityModifiersRequired.ForNonInterfaceMembers)] + [InlineData((int)AccessibilityModifiersRequired.OmitIfDefault)] + public async Task TestModifierOnInterfaceMethods_03(int modifier) + { + var source = @"public interface I +{ + public void [|M|](); +}"; + var expected = @"public interface I +{ + void M(); +}"; + var test = new VerifyCS.Test + { + TestCode = source, + FixedCode = expected, + Options = + { + { CodeStyleOptions2.RequireAccessibilityModifiers, (AccessibilityModifiersRequired)modifier }, + } + }; + + await test.RunAsync(); + } + + [Theory, Trait(Traits.Feature, Traits.Features.CodeActionsAddAccessibilityModifiers)] + [WorkItem(47448, "https://github.com/dotnet/roslyn/issues/47448")] + [InlineData((int)AccessibilityModifiersRequired.Always)] + [InlineData((int)AccessibilityModifiersRequired.ForNonInterfaceMembers)] + [InlineData((int)AccessibilityModifiersRequired.OmitIfDefault)] + public async Task TestModifierOnInterfaceMethods_04(int modifier) + { + var source = @"public interface I +{ + protected void M(); +}"; + + var test = new VerifyCS.Test + { + TestCode = source, + FixedCode = source, + ReferenceAssemblies = Testing.ReferenceAssemblies.Net.Net60, + Options = + { + { CodeStyleOptions2.RequireAccessibilityModifiers, (AccessibilityModifiersRequired)modifier }, + } + }; + + await test.RunAsync(); + } } } diff --git a/src/Analyzers/Core/CodeFixes/AddAccessibilityModifiers/AddAccessibilityModifiersHelpers.cs b/src/Analyzers/Core/CodeFixes/AddAccessibilityModifiers/AddAccessibilityModifiersHelpers.cs index 30e3f839b4d06..935d26af2a8f9 100644 --- a/src/Analyzers/Core/CodeFixes/AddAccessibilityModifiers/AddAccessibilityModifiersHelpers.cs +++ b/src/Analyzers/Core/CodeFixes/AddAccessibilityModifiers/AddAccessibilityModifiersHelpers.cs @@ -49,6 +49,9 @@ private static Accessibility GetPreferredAccessibility(ISymbol symbol) // that's not legal. And these are reasonable default values for them. if (symbol is IMethodSymbol or IPropertySymbol or IEventSymbol) { + if (symbol.ContainingType.IsInterfaceType()) + return Accessibility.Public; + if (symbol.IsAbstract) return Accessibility.Protected;