Roslyn: "Add accessibility modifiers" C# formatting rule breaks code

[vsfeedback]

We do not use "private" and "internal" everywhere because they are default modifiers and we don't want to see defaults in our code. That rule adds them, making lots of diffs for no reason. This rule should not be opt-out, it should be opt-in.

Another problem is methods with "extern" modifier. This rule removes "extern", breaking our interop code. Please don't do that.

_This issue has been moved from https://developercommunity.visualstudio.com/content/problem/293507/add-accessibility-modifiers-c-formatting-rule-brea.html
VSTS ticketId: 648040_
_These are the original issue comments:_

Jinu Joseph [MSFT] on 8/5/2018, 00:53 AM (22 days ago):

Thank you for your feedback! For us to investigate this further, could you please provide code snippet which leads to this issue, specific example will help us to investigate better. We look forward to hearing from you!

Roy Scheerens on 8/5/2018, 03:28 AM (22 days ago):

I have this problem as well, and it seems to happen with every extern function/method that doesn't have an access modifier. If I enable the additional code cleanup, and the 'add accessibility modifiers' option, it will remove extern and replace it with private.

using System;
using System.Runtime.InteropServices;

namespace ConsoleApp8
{
  class Program
  {
    [DllImport("User32.dll", CharSet = CharSet.Unicode)]
     static extern int MessageBox(IntPtr h, string m, string c, int type);

    static void Main(string[] args)
    {
      MessageBox(IntPtr.Zero, "hello world", "My Message Box", 0);
    }
  }
}

Jinu Joseph [MSFT] on 8/5/2018, 10:52 PM (21 days ago):

thanks much for the example, we will investigate the issue

_These are the original issue solutions:_
(no solutions)

[sharwell]

The syntax generator used by this code fix should definitely not be removing the extern modifier. 🐛

[sharwell]

We do not use "private" and "internal" everywhere because they are default modifiers and we don't want to see defaults in our code. That rule adds them, making lots of diffs for no reason. This rule should not be opt-out, it should be opt-in.

Many of our code style rules default to the majority use case, while allowing alternative configurations. This is one of those rules; you can specify the preferred behavior in .editorconfig under the dotnet_style_require_accessibility_modifiers rule:

dotnet_style_require_accessibility_modifiers = omit_if_default:warning

[d-dizhevsky]

@sharwell I would like to take this or at least try to fix. Is this the right place to start from?
https://github.com/dotnet/roslyn/blob/6193ddfc950a7f6b2d8a7509d113711c99c0539f/src/Features/Core/Portable/AddAccessibilityModifiers/AbstractAddAccessibilityModifiersCodeFixProvider.cs#L51

[sharwell]

@d-dizhevsky Yes, and it's specifically the implementation of WithAccessibility (called within that expression) which is causing problems. You would want to add tests for both the code fix reported here and for the CSharpSyntaxGenerator type where WithAccessibility is defined.

• https://github.com/dotnet/roslyn/blob/master/src/EditorFeatures/CSharpTest/AddAccessibilityModifiers/AddAccessibilityModifiersTests.cs
• https://github.com/dotnet/roslyn/blob/master/src/Workspaces/CSharpTest/CodeGeneration/SyntaxGeneratorTests.cs

[jinujoseph]

also reported at link

[FrankHileman]

Is there a converse rule, to remove the noise from the redundant modifiers, private and internal?

[sharwell]

@FrankHileman Yes, see https://github.com/dotnet/roslyn/issues/29561#issuecomment-416660897