dotnet / codeformatter

Tool that uses Roslyn to automatically rewrite the source to follow our coding styles
MIT License
1.24k stars 244 forks source link

Adding the internal modifier to nested types does not account for less accessible types it exposes #70

Closed roncain closed 9 years ago

roncain commented 9 years ago

Consider this scenario:

public class MyClass {
   enum MyEnum {}
   struct MyStruct {
     public MyStruct(MyEnum e) {}
}

CodeFormatter will add an explicit 'private' modifier to MyEnum, and an explicit 'internal' to MyStruct. But this now causes an accessibility conflict because MyStruct exposes the private MyEnum. Nested types' accessibility modifiers should not be elevated above the accessibility of the types they expose.

jaredpar commented 9 years ago

Thanks for reporting!

That's a really interesting case we hadn't considered when implementing that rule. Will take a look at getting that fixed.

jaredpar commented 9 years ago

Actually on second thought that is just a bug. MyStruct should be marked private by the code formatter as it is a nested struct and that is the default accessibility for such a construct.

jaredpar commented 9 years ago

@roncain

Do you have a full sample for this that I could look at? I tried putting just that code through the formatter and it produced the expected output.

public class MyClass
{
    private enum MyEnum { }
    private struct MyStruct
    {
        public MyStruct(MyEnum e) { }
    }
}

I've updated this code recently though so it's possible that it's a bug I already fixed.

Either way there is a regression test for this scenario now.

jaredpar commented 9 years ago

Chatting with @roncain this morning I figured out why I could not reproduce this bug. A bad link caused him to use a much older version of the tool where this bug did exist. I was using the latest sources where it had been fixed. The regression test should ensure we don't hit this again.