Allow IDE0250 (and other surface-area-impacting analyzers) to be customizable by visibility

[stephentoub]

Summary

Add support to IDE0250 for respecting dotnet_code_quality.IDE0250.api_surface or the like

Background and Motivation

Many of the dotnet/roslyn-analyzer rules respect an api_surface configuration knob that lets you only raise diagnostics when the target is of a particular visibility. This is valuable in order to be able to separate out public surface area from the rest, as public surface area often requires more validation / process. For example, in dotnet/runtime we have this:

# CA1822: Mark members as static
dotnet_diagnostic.CA1822.severity = warning
dotnet_code_quality.CA1822.api_surface = private, internal

It'd be a breaking change in many cases to make a public instance member static, and even if something isn't breaking it requires more review to validate we want to accept changes to a contract, but we can do so on a whim for anything that's implementation detail.

Proposed Feature

Allow IDE0250 (and other rules that impact public surface area) to opt-out of public surface area diagnostics.

[CyrusNajmabadi]

i like this idea. @mavasani how did you guys accomplish this?

[CyrusNajmabadi]

Also, wrt api_surface, i def want to make it crisp what these modifiers mean. i assume it's not hte declared accessibility of the symbol, but rather its effective-severity. e.g. if i have internal class X { public void M() { } } we would want to offer that M can be static since it's effectively internal, though publicly declared.

[stephentoub]

My hope has been effective visibility. If it's not that, we likely have a lot of debt to clean up in runtime :)

But I believe it is "effective". Here's where I see roslyn-analyzers bottoming out: https://github.com/dotnet/roslyn-analyzers/blob/50f62a926806bc75f294b5f738bc6694e0941361/src/Utilities/Compiler/Extensions/ISymbolExtensions.cs#L180-L225

[CyrusNajmabadi]

Great. Works for me. I just need info from Manish on how this gets threaded through and I think we can definitely take this into account with IDE features. It would likely be good if you could help by letting us know which IDE features you'd use this for (beyond IDE0250) to help us target the fixes. but we can wait on that info once we hear from manish.

[stephentoub]

Also your new IDE0251 :smile: I'll need to go back through the list of ones we don't currently enable to see which others would be most relevant.

[CyrusNajmabadi]

Note: if you don't apply IDE00251 to your public surface area, it makes it less palatable for me to chnage other analyzers to no longer do what they do in the presence of non-readonly members :)

IT's precisely because i hope you'll annotate everything in BCL with readonly on structs/members that i feel more comfortable making the switch for other analyzers to become more conservative :D

[stephentoub]

Eventually it'd be nice if everything relevant is annotated. But changing a method from non-readonly to readonly signs it up to never be able to mutate, and that's not something someone can just do across all public surface area in dotnet/runtime; all such changes need to be reviewed from an API perspective to ensure we're comfortable with the burden. In contrast, anyone can make non-public surface area readable whenever. So it's primarily about staging. We can enable such an analyzer to fix and lock-in the rule for everything non-public and then work to address the public separately. That said, I'm also not sure we'd ever be able to have the analyzer enabled for public surface area permanently; that could lead to needing a lot of suppression, and in general we don't run such signature-impacting rules as part of CI on public surface area. From that perspective, it'd be nice to be able to say e.g. "enable IDE0251 for non-public surface area as a warning, and for public surface area as a suggestion".

[CyrusNajmabadi]

that makes sense. as long as you guys are putting the thought into public APIs you do think shoudl be readonly, then we get to the state i want where we start trusting the presence/absence of the attribute to be the right way to make the call on other features.

Thanks!