diff --git a/src/EditorFeatures/CSharpTest/AddAccessibilityModifiers/AddAccessibilityModifiersTests.cs b/src/EditorFeatures/CSharpTest/AddAccessibilityModifiers/AddAccessibilityModifiersTests.cs index fff3f1c83423fe63ecf6780ea2f7d15e304e6663..ab6701219d7d6945663a78d08c2b1c70f7a98850 100644 --- a/src/EditorFeatures/CSharpTest/AddAccessibilityModifiers/AddAccessibilityModifiersTests.cs +++ b/src/EditorFeatures/CSharpTest/AddAccessibilityModifiers/AddAccessibilityModifiersTests.cs @@ -1,10 +1,13 @@ // Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. +using System.Collections.Generic; using System.Threading.Tasks; using Microsoft.CodeAnalysis.CodeFixes; +using Microsoft.CodeAnalysis.CodeStyle; using Microsoft.CodeAnalysis.CSharp.AddAccessibilityModifiers; using Microsoft.CodeAnalysis.Diagnostics; using Microsoft.CodeAnalysis.Editor.CSharp.UnitTests.Diagnostics; +using Microsoft.CodeAnalysis.Options; using Roslyn.Test.Utilities; using Xunit; @@ -15,6 +18,9 @@ public partial class AddAccessibilityModifiersTests : AbstractCSharpDiagnosticPr internal override (DiagnosticAnalyzer, CodeFixProvider) CreateDiagnosticProviderAndFixer(Workspace workspace) => (new CSharpAddAccessibilityModifiersDiagnosticAnalyzer(), new CSharpAddAccessibilityModifiersCodeFixProvider()); + private IDictionary OmitDefaultModifiers => + OptionsSet(SingleOption(CodeStyleOptions.RequireAccessibilityModifiers, AccessibilityModifiersRequired.OmitIfDefault, NotificationOption.Suggestion)); + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsAddAccessibilityModifiers)] public async Task TestAllConstructs() { @@ -165,5 +171,165 @@ namespace Test internal readonly struct S1 { } }"); } + + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsAddAccessibilityModifiers)] + public async Task TestAllConstructsWithOmit() + { + await TestInRegularAndScriptAsync( +@" +namespace Outer +{ + namespace Inner1.Inner2 + { + internal class {|FixAllInDocument:C|} + { + private class NestedClass { } + + private struct NestedStruct { } + + private int f1; + private int f2, f3; + public int f4; + + private event Action e1, e2; + public event Action e3; + + private event Action e4 { add { } remove { } } + public event Action e5 { add { } remove { } } + event Action I.e6 { add { } remote { } } + + static C() { } + + private C() { } + public C(int i) { } + + ~C() { } + + private void M1() { } + public void M2() { } + void I.M3() { } + partial void M4() { } + + private int P1 { get; } + public int P2 { get; } + int I.P3 { get; } + + private int this[int i] { get; } + public int this[string s] { get; } + int I.this[bool b] { get; } + } + + internal interface I + { + event Action e6; + void M3(); + int P3 { get; } + int this[bool b] { get; } + } + + internal delegate void D(); + + internal enum E + { + EMember + } + } +}", +@" +namespace Outer +{ + namespace Inner1.Inner2 + { + class {|FixAllInDocument:C|} + { + class NestedClass { } + + struct NestedStruct { } + + int f1; + int f2, f3; + public int f4; + + event Action e1, e2; + public event Action e3; + + event Action e4 { add { } remove { } } + public event Action e5 { add { } remove { } } + event Action I.e6 { add { } remote { } } + + static C() { } + + C() { } + public C(int i) { } + + ~C() { } + + void M1() { } + public void M2() { } + void I.M3() { } + partial void M4() { } + + int P1 { get; } + public int P2 { get; } + int I.P3 { get; } + + int this[int i] { get; } + public int this[string s] { get; } + int I.this[bool b] { get; } + } + + interface I + { + event Action e6; + void M3(); + int P3 { get; } + int this[bool b] { get; } + } + + delegate void D(); + + enum E + { + EMember + } + } +}", options: OmitDefaultModifiers); + } + + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsAddAccessibilityModifiers)] + public async Task TestRefStructsWithOmit() + { + await TestInRegularAndScriptAsync(@" +namespace Test +{ + internal ref struct [|S1|] { } +}", @" +namespace Test +{ + ref struct S1 { } +}", options: OmitDefaultModifiers); + } + + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsAddAccessibilityModifiers)] + public async Task TestReadOnlyStructsWithOmit() + { + await TestInRegularAndScriptAsync(@" +namespace Test +{ + internal readonly struct [|S1|] { } +}", @" +namespace Test +{ + readonly struct S1 { } +}", options: OmitDefaultModifiers); + } + + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsAddAccessibilityModifiers)] + public async Task TestClassOutsideNamespace() + { + await TestInRegularAndScriptAsync(@" +internal class [|C1|] { }", @" +class C1 { }", options: OmitDefaultModifiers); + } } } diff --git a/src/EditorFeatures/VisualBasicTest/AddAccessibilityModifiers/AddAccessibilityModifiersTests.vb b/src/EditorFeatures/VisualBasicTest/AddAccessibilityModifiers/AddAccessibilityModifiersTests.vb index 0c6767a60cea93f418db44cd01b8aff5642c1325..fdeb344819a3b7e26cd370630688616bf5a1c543 100644 --- a/src/EditorFeatures/VisualBasicTest/AddAccessibilityModifiers/AddAccessibilityModifiersTests.vb +++ b/src/EditorFeatures/VisualBasicTest/AddAccessibilityModifiers/AddAccessibilityModifiersTests.vb @@ -4,6 +4,8 @@ Imports Microsoft.CodeAnalysis.CodeFixes Imports Microsoft.CodeAnalysis.Diagnostics Imports Microsoft.CodeAnalysis.VisualBasic.AddAccessibilityModifiers Imports Microsoft.CodeAnalysis.Editor.VisualBasic.UnitTests.Diagnostics +Imports Microsoft.CodeAnalysis.Options +Imports Microsoft.CodeAnalysis.CodeStyle Namespace Microsoft.CodeAnalysis.Editor.VisualBasic.UnitTests.AddAccessibilityModifiers Public Class AddAccessibilityModifiersTests @@ -14,6 +16,14 @@ Namespace Microsoft.CodeAnalysis.Editor.VisualBasic.UnitTests.AddAccessibilityMo New VisualBasicAddAccessibilityModifiersCodeFixProvider()) End Function + Private ReadOnly Property OmitDefaultModifiers As IDictionary(Of OptionKey, Object) + Get + Return OptionsSet( + SingleOption(CodeStyleOptions.RequireAccessibilityModifiers, AccessibilityModifiersRequired.OmitIfDefault, NotificationOption.Suggestion)) + End Get + End Property + + Public Async Function TestAllConstructs() As Task Await TestInRegularAndScriptAsync( @@ -95,7 +105,7 @@ namespace N dim f as integer sub M() - end sub() + end sub shared operator &(c1 as S, c2 as S) as integer end operator @@ -105,7 +115,7 @@ namespace N dim f as integer sub M() - end sub() + end sub end module end namespace end namespace", @@ -187,7 +197,7 @@ namespace N Public f as integer Public sub M() - end sub() + end sub Public shared operator &(c1 as S, c2 as S) as integer end operator @@ -197,10 +207,224 @@ namespace N Private f as integer Public sub M() - end sub() + end sub end module end namespace end namespace") End Function + + + Public Async Function TestAllConstructsWithOmit() As Task + Await TestInRegularAndScriptAsync( +" +namespace N + namespace Outer.Inner + Friend class {|FixAllInDocument:C|} + Public class NestedClass + end class + + Public structure NestedStruct + end structure + + Private f1 as integer + Private f2, f3 as integer + Private f4, f5 as integer, f6, f7 as boolean + public f4 as integer + + Private Const foo As long = 3 + private const bar = 4, barbar = 5 + + public Const pfoo As long = 3 + public Const pbar = 4, pbarbar As ULong = 5 + + Private Shared sfoo = 4 + private shared sbar as Long = 5, sbarbar = 0 + + public Shared spfoo = 4 + public Shared spbar = 4, spbarbar as Long = 4 + + Public event e1 as Action + public event e2 as Action + + Public custom event e4 as Action + end event + + shared sub new() + end sub + + Public sub new() + end sub + + public sub new(i as integer) + end sub + + Public sub M1() + end sub + + public sub M2() + end sub + + Public function M3() as integer + end function + + Public function M4() as integer + + public function M5() as integer + end function + + Private partial sub M6() + end sub + + Public property P1 as integer + + Public property P2 as integer + get + end get + end property + + public property P3 as integer + + Public shared operator &(c1 as C, c2 as C) as integer + end operator + end class + + Friend interface I + event e6 as Action + sub M3() + function M4() as integer + property P3 as integer + end interface + + Friend delegate sub D1() + Friend delegate function D2() as integer + + Friend enum E + EMember + end enum + + Friend structure S + Public f as integer + + Public sub M() + end sub + + Public shared operator &(c1 as S, c2 as S) as integer + end operator + end structure + + Friend module M + Private f as integer + + Public sub M() + end sub + end module + end namespace +end namespace", +" +namespace N + namespace Outer.Inner + class C + class NestedClass + end class + + structure NestedStruct + end structure + + Dim f1 as integer + Dim f2, f3 as integer + Dim f4, f5 as integer, f6, f7 as boolean + public f4 as integer + + Const foo As long = 3 + const bar = 4, barbar = 5 + + public Const pfoo As long = 3 + public Const pbar = 4, pbarbar As ULong = 5 + + Shared sfoo = 4 + shared sbar as Long = 5, sbarbar = 0 + + public Shared spfoo = 4 + public Shared spbar = 4, spbarbar as Long = 4 + + event e1 as Action + event e2 as Action + + custom event e4 as Action + end event + + shared sub new() + end sub + + sub new() + end sub + + sub new(i as integer) + end sub + + sub M1() + end sub + + sub M2() + end sub + + function M3() as integer + end function + + function M4() as integer + + function M5() as integer + end function + + Private partial sub M6() + end sub + + property P1 as integer + + property P2 as integer + get + end get + end property + + property P3 as integer + + shared operator &(c1 as C, c2 as C) as integer + end operator + end class + + interface I + event e6 as Action + sub M3() + function M4() as integer + property P3 as integer + end interface + + delegate sub D1() + delegate function D2() as integer + + enum E + EMember + end enum + + structure S + Dim f as integer + + sub M() + end sub + + shared operator &(c1 as S, c2 as S) as integer + end operator + end structure + + module M + Dim f as integer + + sub M() + end sub + end module + end namespace +end namespace", options:=OmitDefaultModifiers) + End Function + End Class End Namespace diff --git a/src/Features/CSharp/Portable/AddAccessibilityModifiers/CSharpAddAccessibilityModifiersDiagnosticAnalyzer.cs b/src/Features/CSharp/Portable/AddAccessibilityModifiers/CSharpAddAccessibilityModifiersDiagnosticAnalyzer.cs index 246252f6da600714467e0a6084204ab63076a693..4563f4b4cbf42614e9ed9a92949a85311318424c 100644 --- a/src/Features/CSharp/Portable/AddAccessibilityModifiers/CSharpAddAccessibilityModifiersDiagnosticAnalyzer.cs +++ b/src/Features/CSharp/Portable/AddAccessibilityModifiers/CSharpAddAccessibilityModifiersDiagnosticAnalyzer.cs @@ -76,14 +76,60 @@ public CSharpAddAccessibilityModifiersDiagnosticAnalyzer() return; } - // If they already have accessibility, no need to report anything. + // This analyzer bases all of its decisions on the accessibility var accessibility = generator.GetAccessibility(member); - if (accessibility != Accessibility.NotApplicable) + + // Omit will flag any accesibility values that exist and are default + // The other options will remove or ignore accessibility + var isOmit = option.Value == AccessibilityModifiersRequired.OmitIfDefault; + + if (isOmit) { - return; + if (accessibility == Accessibility.NotApplicable) + { + return; + } + + var parentKind = member.Parent.Kind(); + switch (parentKind) + { + // Check for default modifiers in namespace and outside of namespace + case SyntaxKind.CompilationUnit: + case SyntaxKind.NamespaceDeclaration: + { + // Default is internal + if (accessibility != Accessibility.Internal) + { + return; + } + } + break; + + case SyntaxKind.ClassDeclaration: + case SyntaxKind.StructDeclaration: + { + // Inside a type, default is private + if (accessibility != Accessibility.Private) + { + return; + } + } + break; + + default: + return; // Unknown parent kind, don't do anything + } + } + else + { + // Mode is always, so we have to flag missing modifiers + if (accessibility != Accessibility.NotApplicable) + { + return; + } } - // Missing accessibility. Report issue to user. + // Have an issue to flag, either add or remove. Report issue to user. var additionalLocations = ImmutableArray.Create(member.GetLocation()); context.ReportDiagnostic(Diagnostic.Create( CreateDescriptorWithSeverity(option.Notification.Value), diff --git a/src/Features/Core/Portable/AddAccessibilityModifiers/AbstractAddAccessibilityModifiersCodeFixProvider.cs b/src/Features/Core/Portable/AddAccessibilityModifiers/AbstractAddAccessibilityModifiersCodeFixProvider.cs index 658788122853f645d342459f42f34dcb5e408035..624d9fa5d9b7524c748c60502a7ec00598d3341d 100644 --- a/src/Features/Core/Portable/AddAccessibilityModifiers/AbstractAddAccessibilityModifiersCodeFixProvider.cs +++ b/src/Features/Core/Portable/AddAccessibilityModifiers/AbstractAddAccessibilityModifiersCodeFixProvider.cs @@ -2,7 +2,6 @@ using System; using System.Collections.Immutable; -using System.Composition; using System.Linq; using System.Threading; using System.Threading.Tasks; @@ -47,11 +46,15 @@ public sealed override Task RegisterCodeFixesAsync(CodeFixContext context) var symbol = semanticModel.GetDeclaredSymbol(declarator, cancellationToken); + // Check to see if we need to add or remove + // If there's a modifier, then we need to remove it, otherwise no modifier, add it. editor.ReplaceNode( declaration, (currentDeclaration, generator) => { - return generator.WithAccessibility(currentDeclaration, symbol.DeclaredAccessibility); + return generator.GetAccessibility(currentDeclaration) == Accessibility.NotApplicable + ? generator.WithAccessibility(currentDeclaration, symbol.DeclaredAccessibility) // No accessibilty was declared, we need to add it + : generator.WithAccessibility(currentDeclaration, Accessibility.NotApplicable); // There was an accessibility, so remove it }); } } diff --git a/src/Features/VisualBasic/Portable/AddAccessibilityModifiers/VisualBasicAddAccessibilityModifiersDiagnosticAnalyzer.vb b/src/Features/VisualBasic/Portable/AddAccessibilityModifiers/VisualBasicAddAccessibilityModifiersDiagnosticAnalyzer.vb index c27afe58dcca566dc4cbb066f01c39e734610d27..b79930dcceddabcec174f37a2ca60837dc674f14 100644 --- a/src/Features/VisualBasic/Portable/AddAccessibilityModifiers/VisualBasicAddAccessibilityModifiersDiagnosticAnalyzer.vb +++ b/src/Features/VisualBasic/Portable/AddAccessibilityModifiers/VisualBasicAddAccessibilityModifiersDiagnosticAnalyzer.vb @@ -55,18 +55,57 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.AddAccessibilityModifiers Return End If - ' If they already have accessibility, no need to report anything. + ' This analyzer bases all of its decisions on the accessibility Dim Accessibility = generator.GetAccessibility(member) - If Accessibility <> Accessibility.NotApplicable Then - Return + + ' Omit will flag any accesibility values that exist and are default + ' The other options will remove or ignore accessibility + Dim isOmit = [option].Value = AccessibilityModifiersRequired.OmitIfDefault + + If isOmit Then + If Accessibility = Accessibility.NotApplicable Then + ' Accessibility modifier already missing. nothing we need to do. + Return + End If + + If Not MatchesDefaultAccessibility(Accessibility, member) Then + ' Explicit accessibility was different than the default accessibility. + ' We have to keep this here. + Return + End If + + Else ' Require all, flag missing modidifers + If Accessibility <> Accessibility.NotApplicable Then + Return + End If End If - ' Missing accessibility. Report issue to user. + ' Have an issue to flag, either add or remove. Report issue to user. Dim additionalLocations = ImmutableArray.Create(member.GetLocation()) context.ReportDiagnostic(Diagnostic.Create( CreateDescriptorWithSeverity([option].Notification.Value), name.GetLocation(), additionalLocations:=additionalLocations)) End Sub + + Private Function MatchesDefaultAccessibility(accessibility As Accessibility, member As StatementSyntax) As Boolean + ' Top level items in a namespace or file + If member.IsParentKind(SyntaxKind.CompilationUnit) OrElse + member.IsParentKind(SyntaxKind.NamespaceBlock) Then + ' default is Friend + Return accessibility = Accessibility.Friend + End If + + ' default for const and field in a class is private + If member.IsParentKind(SyntaxKind.ClassBlock) OrElse + member.IsParentKind(SyntaxKind.ModuleBlock) Then + If member.IsKind(SyntaxKind.FieldDeclaration) Then + Return accessibility = Accessibility.Private + End If + End If + + ' Everything else has a default of public + Return accessibility = Accessibility.Public + End Function End Class End Namespace diff --git a/src/Workspaces/Core/Portable/CodeStyle/AccessibilityModifiersRequired.cs b/src/Workspaces/Core/Portable/CodeStyle/AccessibilityModifiersRequired.cs index 7c11deb5d5a77963b36ad26131435165966613ab..55bfe217960f58f441da5dc90fca138691d5fa6c 100644 --- a/src/Workspaces/Core/Portable/CodeStyle/AccessibilityModifiersRequired.cs +++ b/src/Workspaces/Core/Portable/CodeStyle/AccessibilityModifiersRequired.cs @@ -5,7 +5,10 @@ namespace Microsoft.CodeAnalysis.CodeStyle { internal enum AccessibilityModifiersRequired { + // The rule is not run Never = 0, + + // Accessibility modifiers are added if missing, even if default Always = 1, // Future proofing for when C# adds default interface methods. At that point @@ -13,5 +16,8 @@ internal enum AccessibilityModifiersRequired // want to require them, while some may want to keep the traditional C# style // that public interface members do not need accessibility modifiers. ForNonInterfaceMembers = 2, + + // Remove any accessibility modifier that matches the default + OmitIfDefault = 3 } } diff --git a/src/Workspaces/Core/Portable/CodeStyle/CodeStyleOptions.cs b/src/Workspaces/Core/Portable/CodeStyle/CodeStyleOptions.cs index 411958caafa64a7eed8d1ea8ef02974655ed8098..dda125309e83df8dd6e944102c187bc974ad24fa 100644 --- a/src/Workspaces/Core/Portable/CodeStyle/CodeStyleOptions.cs +++ b/src/Workspaces/Core/Portable/CodeStyle/CodeStyleOptions.cs @@ -205,6 +205,8 @@ private static CodeStyleOption ParseAccessibilit return new CodeStyleOption(AccessibilityModifiersRequired.Always, notificationOpt); case "for_non_interface_members": return new CodeStyleOption(AccessibilityModifiersRequired.ForNonInterfaceMembers, notificationOpt); + case "omit_if_default": + return new CodeStyleOption(AccessibilityModifiersRequired.OmitIfDefault, notificationOpt); } } } diff --git a/src/Workspaces/VisualBasic/Portable/CodeGeneration/VisualBasicSyntaxGenerator.vb b/src/Workspaces/VisualBasic/Portable/CodeGeneration/VisualBasicSyntaxGenerator.vb index 484b5d59eba4519af14152c986bb406b87d92384..88b698dc580cba288e1c926890df3568e176eeb6 100644 --- a/src/Workspaces/VisualBasic/Portable/CodeGeneration/VisualBasicSyntaxGenerator.vb +++ b/src/Workspaces/VisualBasic/Portable/CodeGeneration/VisualBasicSyntaxGenerator.vb @@ -2706,6 +2706,13 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.CodeGeneration End If Dim newTokens = GetModifierList(accessibility, mods, GetDeclarationKind(declaration), isDefault) + 'GetDeclarationKind returns None for Field if the count is > 1 + 'To handle multiple declarations on a field if the Accessibility is NotApplicable, we need to add the Dim + If declaration.Kind = SyntaxKind.FieldDeclaration AndAlso accessibility = Accessibility.NotApplicable AndAlso newTokens.Count = 0 Then + ' Add the Dim + newTokens = newTokens.Add(SyntaxFactory.Token(SyntaxKind.DimKeyword)) + End If + Return WithModifierTokens(declaration, Merge(tokens, newTokens)) End Function