@jinujoseph or @mavasani should this PR be targeting https://github.com/dotnet/roslyn-analyzers?

@jasonmalinowski this is an .editorconfig code style setting. From the discussion in the linked issue, it sounded like I should just mirror what @CyrusNajmabadi did in another PR. It would need to be here to be available as a default .editorconfig setting for all, right?

@onovotny There's a simultaneous effort to move them, I think.

@jasonmalinowski okay, gotcha.

I'm not entirely sure how to get the default accessibility for a member here:

https://github.com/onovotny/roslyn/blob/3028358c96e7f0b75aefd6416ba679d97e5970de/src/Features/CSharp/Portable/OmitDefaultAccessibilityModifiers/CSharpOmitDefaultAccessibilityModifiersDiagnosticAnalyzer.cs#L86

I have to assume these rules are somewhere that I should be checking? -- class is internal, members in a type private, etc?

Also frequently getting this trying to attach to the GUI worker process:

I have to restart VS and then I can do it once....

I have something working, though not sure if the right way. I am seeing a whitepace issue on the test though:

Microsoft.CodeAnalysis.Editor.CSharp.UnitTests.OmitDefaultAccessibilityModifiers.OmitDefaultAccessibilityModifiersTests.TestAllConstructs FAILED:
	Exception type: 'Xunit.Sdk.EqualException', number: '0', parent: '-1'
	Exception message:
Assert.Equal() Failure
                                   ↓ (pos 506)
Expected: ···    static C() { }\r\n            C() { }\r\n            public C···
Actual:   ···    static C() { }\r\n\r\n            C() { }\r\n            public···
                                   ↑ (pos 506)
	Exception stacktrace

With some feedback, I can continue on the VB.NET side.

Thanks!

My preferred approach is having a tri-state option, like this:

dotnet_default_accessibility_modifiers = allow|require|omit:severity

Since there appears to be a difference of opinions, we should wait for everyone to be back focused on work instead of various family outings, and resolve this in a design meeting in the new year. 😄

@sharwell I agree with you. I thought it should be part of the same option. You can see that there's debate on this #7066 (comment).

I agree this should go to a design meeting and I'm happy to update this as per the decision.

Question to think about: the current option, which is shipping, is dotnet_style_require_accessibility_modifiers. Is there a way to rename that to dotnet_style_default_accessibility_modifiers without breaking people who already are using the current option? I'm not sure if there's a mapping mechanism if you want to rename the setting name.

@onovotny thanks for your patience! We are meeting next week and will have an answer for you by the end of next week on design here...

@kuhlenh Thanks! IMO, I think @sharwell is right and it should be a single rule called dotnet_style_default_accessibility_modifiers. I don't know what kind of remapping mechanisms might exist for backwards compat though, if that's an issue.

I don't know what kind of remapping mechanisms might exist for backwards compat though, if that's an issue.

You can provide arbitrary code to handle this sort of thing. At least a couple of our binary options have become tri-state and then handle the processing accordingly. For example the expression-body option gained 'when on single line' in a backward compatible manner.

@CyrusNajmabadi In that case, or in any other, has the name of the option itself changed or just the available values that can be set for it?

Just the option. However, we could always add the codepath that allows for the name to change as well. :)

@onovotny Design meeting was today. We decided to use the current option (no rename), and to just add a third allowed value:

• true: Currently supported, and meaning does not change
• false: Currently supported, and meaning does not change
• omit_if_default: New value, supported after this is merged. Old versions of Visual Studio will see this as an invalid value, and fall back to the default value false.

This is desirable since it means for all users, the behavior in old versions is unchanged regardless of whether the project adopts the new syntax.

@onovotny It would be painful, and it's totally my fault for leading you down the wrong path, but i think this should all go into a single analyzer/fixer. Basically its the same analyzer fixer that checks your code against the option, and then fixes it accordingly. Right now, having it be two separate ones means basically near 100% duplication, whne all that really had to change is the simple check, and what the fix does.

:(

@CyrusNajmabadi No problem. I'll update my code accordingly. Is there any chance of this hitting 15.6, and if so, what would the deadlines be before it's too late?

@Pilchie I'd like to merge this in 15.7. Can I get an ask mode bar check before doing the rebase it would need?

Ping? :)

Approved for 15.7 - sorry for the delay. Note that it will need to be rebased though (master will be next foundational update, not 15.7)

@onovotny Please let me know if you would like me to do the rebase or if you would like to do it yourself. :)

@sharwell if you don't mind, I'd really appreciate it. Not sure what's diverged w.r.t. the changes in this batch.

@onovotny Nothing diverged, we just want to ship the change earlier than the master branch will ship next. I'll get the rebase done and then we can merge it after the build passes. 😄

@sharwell Thank you!

This will be my first contribution to Roslyn 🎉

windows_release_unit32_prtest failed on DiagnosticDataTests.DiagnosticData_GetText6 (details)

FYI @dotnet/roslyn-infrastructure I'll re-run

test windows_release_unit32_prtest please