sealed class analyzer

[MarkPflug]

I would like to propose the ability to explicitly mark a class as non-sealed. This is the default of course, but more often than not I want to seal my classes and there is currently no way for an analyzer to identify whether a class was intentionally left unsealed or not. To fix this, I would propose a new class modifier virtual to explicitly indicate that the developer intended for the class to be unsealed. This would then make it possible to write an analyzer that requires that a class be explicitly sealed or virtual (or abstract, static, etc).

My opinion is that sealed should have been the default for classes in C# (to match method behavior), and this would allow enforcing that opinion more easily in my own codebases.

[YairHalberstadt]

Why not just use an attribute?

[YairHalberstadt]

I would put it in the project that defines the analyser

[CyrusNajmabadi]

I guess that just feels less elegant than having the language support it natively.

Potentially. but it also works today across all versions of C# with very little effort :)

[MarkPflug]

I would put it in the project that defines the analyser

That doesn't make sense to me, wouldn't that require a runtime dependency on the analyzer library? Analyzers are only used at compile time, and normally wouldn't be a dependency of the output assembly. Maybe by putting a Conditional on it that would make it not apply in the output assembly. I'd have to think about that.

Potentially. but it also works today across all versions of C# with very little effort :)

Of course, but it also seemed like a reasonable enhancement for the language that I thought might be worth considering.

[orthoxerox]

I would put it in the project that contains the code to be annotated and just match it by name in the analyzer.

[MarkPflug]

@DavidArno Any chance your analyzer is available as a nuget package?

[Zodt]

I don't think this kind of juggling with modifiers is a good idea.
This will be a badly breaking change.

[MarkPflug]

It isn't clear to me how this would be a breaking change. virtual is a keyword that was not previously valid as a class modifier. Allowing it would make previously invalid programs valid but wouldn't change the behavior of any privously valid programs.

[supervos]

I agree with @MarkPflug, this can be a default modifier. C# already has default access modifier, this could be the same with a class that can be marked explicitly as sealed/nonsealed (virtual)/abstract where there is currently an implicit/undocumented default.

The documentation https://learn.microsoft.com/en-us/dotnet/csharp/programming-guide/classes-and-structs/abstract-and-sealed-classes-and-class-members and https://learn.microsoft.com/en-us/dotnet/csharp/fundamentals/object-oriented/inheritance do not specifically say that a class by default can be inherited, only what you can do to avoid (sealed) or force (abstract) inheritance.

Having a specific keyword for the default case allows an analyzer, just like the access modifiers have IDE0040 and SA1400.

There is no backwards compatibility issue as this nonsealed modifier is currently a hidden default and this change can be seen like the Nullable Reference Types where you can enable it on project level to allow an extra keyword and doesn't influence projects that don't use this modifier. I don't know the specifics of how the Nullable Reference Types are added to the IL but this can be seen as purely syntactic sugar to help analyzers and programmers who want to think about the effects of sealed/nonsealed/abstract on their code.

[jnm2]

You might be interested in the discussion Stephen Toub started about shipping such an analyzer in the box with the SDK: dotnet/runtime#49944

[DavidArno]

@FaustVX,

Yes, this is the chicken and egg situation: because sealed isn't the default, it's easy to forget to mark it as sealed before release. As a consequence, someone may then go and inherit from a class that you didn't design for inheritance. Luckily for us all, there's a well-established industry standard best practice that comes to our rescue here: semantic versioning. Up the major version, mark it sealed, document a breaking change and release. Alternatively, suck up the fact that you fell victim to the fragile base class problem, suppress the analyzer for that class and live with it.

Either way, having something like CA1852 report on public classes too (except when they contain protected members for example as a logical way to flag that the type is designed for inheritance) would be a huge step forward in C# code quality.

[supervos]

Sorry, I should have been clearer in my comment, the linked discussion is interesting, but it doesn't fully cover the original issue that you can't explicitly state that a public class is made with inheritance in mind. This makes it harder for an analyzer to know if you leave a class unsealed by design or by accident.

@DavidArno has correctly guessed my intent :)

Adding a keyword to indicate nonsealed would bring this modifier to the same level as the access modifiers that also have a default. It would also allow the creation of analyzers just like IDE0040 and SA1400 that allow you to explicitly state your intent.

I think that it would also help the documentation https://learn.microsoft.com/en-us/dotnet/csharp/programming-guide/classes-and-structs/abstract-and-sealed-classes-and-class-members and https://learn.microsoft.com/en-us/dotnet/csharp/fundamentals/object-oriented/inheritance that there is an explicit 3th option besides avoiding (sealed) or forcing (abstract) inheritance.

[HaloFour]

You could use a custom attribute to explicitly indicate that you intended to leave the class unsealed.

[supervos]

This is partially discussed in comment #5631 (comment) on this thread. Where would you place that attribute?

• Do you create a custom class with only a single attribute for this? This causes a dependency to a project that not needed at runtime and that you have to ship together with your library
• Do you place it in the current library and match by name? Then you have an empty attribute class that is replicated over every project and a possibility of spelling mistakes

As far as I know, an attribute is compiled into the type information and will be returned when you request the attributes of a type at runtime. Having a useless entry that is only used for design time seems overkill and can cause issues for libraries that incorrectly handle attributes (direct casting instead of doing a type check, depending on the number of attributes defined, ...)

[HaloFour]

You can use ConditionalAttribute to cause the NonSealedAttribute to not be emitted into the final assembly.

As for where it's declared, you could have the source generator look for it by name rather than by a particular type. That's how the C# compiler works. Given the analyzer would emit an error during compilation if you get the name wrong, I don't see typos being a concern.

[CyrusNajmabadi]

Closing out. As David mentions, this is trivially solvable with an analyzer. If there is a request for more extensive/configurable analyzers, that should go to dotnet/roslyn-analyzers. It's out of scope for the language (which likely won't be changing here)..

[supervos]

Adding a specific option besides sealed and abstract makes it easier for an analyzer that acts just like IDE0040 and SA1400 and don't have to analyze the full code tree to see if there is a virtual method, or if any project in the solution inherits the class.

There are also use cases where you must deliberately leave a class as non sealed because a framework (like Entity Framework) is creating a proxy for your class. Having an analyzer just looking at the presence of virtual methods or other inheritors and without the existence of a keyword to explicitly mark as non sealed would cause false positives.

[thomaseyde]

Is there any reason this cannot be a project setting like <SealedClassByDefault>enable</SealedClassByDefault>?

Then the compiler can make the class sealed if there are no virtual members declared.

[KennethHoff]

IIUC that's how it functions in NativeAoT, so one could argue <PublishAot>true</PublishAot> is exactly that :>

For real though; I would personally like something along the lines of an unsealed keyword and a way to make sealed the default, but at the same time I agree with the C# team on this; We do not want more 'dialects' - places where identical code functions differently (like <Nullable>enabled</Nullable>)