Merge pull request #23919 from onovotny/default-modifier-style

Omit Default modifier style & code fix

src/EditorFeatures/CSharpTest/AddAccessibilityModifiers/AddAccessibilityModifiersTests.cs

 // 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<OptionKey, object> 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);
+        }
     }
 }

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
+
+
         <Fact, Trait(Traits.Feature, Traits.Features.CodeActionsAddAccessibilityModifiers)>
         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
+
+                <Fact, Trait(Traits.Feature, Traits.Features.CodeActionsAddAccessibilityModifiers)>
+        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

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),

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                       
                     });
             }
         }

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

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
     }
 }

src/Workspaces/Core/Portable/CodeStyle/CodeStyleOptions.cs

@@ -205,6 +205,8 @@ private static CodeStyleOption<AccessibilityModifiersRequired> ParseAccessibilit
                             return new CodeStyleOption<AccessibilityModifiersRequired>(AccessibilityModifiersRequired.Always, notificationOpt);
                         case "for_non_interface_members":
                             return new CodeStyleOption<AccessibilityModifiersRequired>(AccessibilityModifiersRequired.ForNonInterfaceMembers, notificationOpt);
+                        case "omit_if_default":
+                            return new CodeStyleOption<AccessibilityModifiersRequired>(AccessibilityModifiersRequired.OmitIfDefault, notificationOpt);
                     }
                 }
             }

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