Enable CA1852: Seal internal types

[ladipro]

Context

Types that are not designed to be inherited from should be sealed. This is general defensive goodness, and also has perf implications because it enables some runtime/JIT optimizations. A new analyzer was recently added for this.

Changes Made

Enabled the analyzer and fixed the code with dotnet format.

Testing

Manual review of the changes.

Notes

dotnet format also rectified our namespace imports. I don't believe there's any controversy in that and the change is low risk so I left it in. For the actual sealing:

• It missed one class because it was ifdefed out in Core. I fixed it manually.
• It resulted in a handful of build errors where the compiler didn't like protected members in sealed types. I fixed those manually.
• The analyzer doesn't understand InternalsVisibleTo, by design. This is not causing any issues for us now as we don't derive from internal types in other assemblies. Our InternalsVisibleTo all point to our own assemblies built in this repo so this is a non-breaking change (unless hacks, reflection, etc. but the bar is certainly not that high).
• I am not including changes to files under src/Deprecated in this PR.

[vlada-shubina]

I am not including changes to files under src/Deprecated in this PR.

We can possibly exclude the analyzers for those classes using globbing in editorconfig.
Similar to what we are doing in templating for external source code:

https://github.com/dotnet/templating/blob/abf67821fa26adc56016612747ccd31eed23a592/.editorconfig#L645-L648

[rainersigwald]

I'd prefer two commits: one for dotnet format to do the usings, then add the sealed rule and rerun. But I won't block on it.

[ladipro]

I'd prefer two commits: one for dotnet format to do the usings, then add the sealed rule and rerun. But I won't block on it.

That's very reasonable and easy to do. I have force-pushed an update with 2 commits. The overall diff is identical to the initial commit.

[ladipro]

I am not including changes to files under src/Deprecated in this PR.

We can possibly exclude the analyzers for those classes using globbing in editorconfig. Similar to what we are doing in templating for external source code:

https://github.com/dotnet/templating/blob/abf67821fa26adc56016612747ccd31eed23a592/.editorconfig#L645-L648

This is already kind of working via the ProjectIsDeprecated prop. Without it build would be failing in the Deprecated directory unless we catch up with all the severity=warning rules there. It's just that dotnet format doesn't respect it and makes fixes there, which may be a bug. I also see other dotnet format issues such as not honoring DOTNET_ROOT and always running MSBuild from a globally installed SDK. I'm adding it to the list of things to follow up on later.

[vlada-shubina]

I am not including changes to files under src/Deprecated in this PR.

We can possibly exclude the analyzers for those classes using globbing in editorconfig. Similar to what we are doing in templating for external source code:

https://github.com/dotnet/templating/blob/abf67821fa26adc56016612747ccd31eed23a592/.editorconfig#L645-L648

This is already kind of working via the ProjectIsDeprecated prop. Without it build would be failing in the Deprecated directory unless we catch up with all the severity=warning rules there. It's just that dotnet format doesn't respect it and makes fixes there, which may be a bug. I also see other dotnet format issues such as not honoring DOTNET_ROOT and always running MSBuild from a globally installed SDK. I'm adding it to the list of things to follow up on later.

VS auto-fixes are not respecting it either (but it's also easy to revert those changes).

[JanKrivanek]

Thanks!

Out of scope and stupid question (given by lack of detailed knowledge of analyzers and dotnet format) - should running of dotnet format guarantee that ones changes are properly adhering to the preconfigured rules (.editorconfig)? Or at least those that can be automatically fixed?

If that's the case should dotnet format be strongly suggested as a pre-commit step (alonmg with e.g. build.cmd -test)? Or is there any downside (e.g. any possibly unwanted automatic reformatting)?

I'm just thinking whether we should add this step (and other specific steps - like the build.cmd -test) to our contributing guide

[ladipro]

Out of scope and stupid question (given by lack of detailed knowledge of analyzers and dotnet format) - should running of dotnet format guarantee that ones changes are properly adhering to the preconfigured rules (.editorconfig)? Or at least those that can be automatically fixed?

If that's the case should dotnet format be strongly suggested as a pre-commit step (alonmg with e.g. build.cmd -test)? Or is there any downside (e.g. any possibly unwanted automatic reformatting)?

I'm just thinking whether we should add this step (and other specific steps - like the build.cmd -test) to our contributing guide

Apologies for not responding earlier. My understanding is that we allow rules to be configured as "suggestion", meaning that we don't enforce them, we just slightly prefer them to be followed. If dotnet format attempts to fix everything, suggestions included, it would result in unnecessary and unwanted churn. We enforce only rules that generate warnings and those should be checked by regular build so they are already part of our contribution acceptance criteria.