I would propose to introduce a new rule that warns on classes that are neither static, abstract, of sealed/final.
Inheritance is one of the pillar of OOP. However, in the real world, most classes are not designed to be properly inheritable.
Properly designing a class to be inheritable is a tricky task. One must anticipate state initialization, state correctness, state mutability and also non-public methods calls from descendants. Anticipating well code re-use is hard and this requires experience. As a consequence there is no chance that a class gets well designed for inheritance by chance.
This is applicable to Java, .NET, PHP, and others.
The tricky part is - of course - how to make clear that a class is open for inheritance:
public /* virtual */ class MyVirtualClass { } // can be compliant, not my preference
[Inheritable]
public class MyInheritableClass { } // Convention based attribute, my preferred option
public sealed class InheritsFrom : WithKnownInheritance { }
public class WithKnownInheritance { } // I'm not sure if this should be sufficient once in the same assembly
public class WithVirtualMethod // Compliant, making a method virtual is a sign of willingly inheritable
{
public virtual void MyVirtualMethod() { }
}
public class WithVirtualProperty // Compliant, making a property virtual is a sign of willingly inheritable
{
public virtual int MyVirtualProperty => 42;
}
What to pick is a debate, and might be language specific, although I would prefer to have a similar solution for all languages.
I can only speak from the Java side of things but this falls in the category of good rules to have as an option but too noisy to be used on all projects.
Say we came up with a good implementation that can raise issues on the right cases, and included it as part of the default quality profile, the noise it would generate would probably be overwhelming for most existing applications. Because the default Java behavior, without sealed or final on classes, is to leave the code open for extension, we would raise issues on code that is completely fine as-is.
If the objective is to enforce some design decision, then my guess is that this should happen outside the sonar analysis, in a phase where the implementation is checked against the specification.
As we actively avoid implementing rules that cannot be integrated into the default quality profile, I would encourage you to look at implementing a Java custom rule that checks for this issue.
I can see why it should not enable this by default, and I understand your reasoning not to put too much effort in rules like those.
However, both for .NET and Java, the default behavior that is open to extension is not seen as being a good thing by a lot of developers.
Furthermore, if nothing in a class hints on the possibility of being overridden (as demonstrated in the examples), it might not a good idea to allow it, hence this proposal.
If you decide not to go forward with this, I’ll implement my own, that is not the problem.
For the .NET, we would like to have such a rule, as it brings performance improvements to the project. It would need to be a non-SonarWay, only for users and projects that are known to be useful (like web apps and console apps).
I personally don’t see how to make this work on class libraries, as they can be referenced even by other projects in the same solution and we can’t detect that (AFAICT).In theory, we could ask to seal at least internal types.
I’ve chosen to allow to convention based decorator as an ‘alternative suppressor’, and to exclude Attributes and Exceptions totally. Obviously it should cover both records and classes.
I’m not sure if such a homegrown name based convention would fit the Sonar philosophy, that’s why I decided to implement it my self.
I’m more than willing to help out, implementing this in the Sonar code base as well.
using System;
using System.ComponentModel;
namespace Noncompliant
{
public class NotAbstractNotSealedClass { } // Noncompliant {{Seal this class or make it explicit inheritable.}}
// ^^^^^^^^^^^^^^^^^^^^^^^^^
public partial class PartialClass { } // Noncompliant
public partial class PartialClass { } // Noncompliant
public class ClassWithExplicitCtor // Noncompliant
{
public ClassWithExplicitCtor(object arg) { }
}
public class NotAbstractWithProtectedBase : AbstractBase { } // Noncompliant
public abstract class AbstractBase
{
protected object Property { get; set; }
}
public class NotAbstractWithVirtualBase : VirtualBase { } // Noncompliant
public class WithProtectedOverride : VirtualBase // Noncompliant
{
protected override object Property { get; set; }
}
public abstract class VirtualBase
{
protected virtual object Property { get; set; }
}
}
namespace Compliant
{
public class SomeAttribute : Attribute { } // Compliant {{Attributes are ignored.}}
public class SomeExceptoin : Exception { } // Compliant {{Exceptions are ignored.}}
[Inheritable]
public class WithAttribute { } // Compliant {{Decorated with Inheritable attribute.}}
[SupportsMocking]
public class WithDerivedAttribute { } // Compliant {{Decorated with derived Inheritable attribute.}}
public abstract class AbstractClass { } // Compliant {{Abstract classes should be inherited.}}
public sealed class SealedClass { } // Compliant {{Sealed classes can not be inherited..}}
public static class StaticClass { } // Compliant {{Static classes can not be inherited.}}
public class WithProtectedCtor // Compliant {{Designed for inheritance.}}
{
protected WithProtectedCtor() { }
}
public class WithProtectedProperty // Compliant {{Designed for inheritance.}}
{
protected object Property { get; set; }
}
public class WithProtectedField // Compliant {{Designed for inheritance.}}
{
protected object field;
}
public class WithProtectedMethod // Compliant {{Designed for inheritance.}}
{
protected object Method() => new();
}
public class WithVirtualProperty // Compliant {{Designed for inheritance.}}
{
public virtual object Property { get; set; }
}
public class WithVirtualMethod // Compliant {{Designed for inheritance.}}
{
public virtual object Method() => new();
}
[Obsolete]
public class ObsoleteClass { } // Compliant {{Obsolete classes are ignored.}}
public partial class PartialSealedClass { } // Compliant
public sealed partial class PartialSealedClass { } // Compliant
public class InheritableAttribute : Attribute { }
public sealed class SupportsMockingAttribute : InheritableAttribute { }
}
We can’t take any assumptions about naming conventions, unfortunately. We would need some systematic solution to the noise problem.
AFAICT, we can’t predict that a class will be used by a referenced project in the same solution. So while the rule will be valuable, it’s not technically possible to implement it on the .NET side
@Pavel_Mikula Nope, unfortunately not. I would prefer to have the rule I implemented combined with Sonar, but given the Sonar approach that is not a good fit.
It would be really nice to know - within the Roslyn analyzer context - that the assembly you’re analyzing is the last one to analyze/compile. That would allow some of these kind of solutions to be built.
Anyway, for those who have no problems depending on a naming convention, can use my rule.
