linter
linter copied to clipboard
dart-lang
proposal: `Prefer extending this class over implementing it` backed by annotation on the class
prefer_extends_over_implements
Description
Implementing this class is unsafe because public methods may be added to the class in the future. Extending the class is safe.
Details
Methods can't be safely added to an abstract class or a mixin that has been implemented rather than extended. Abstract classes that exist over long periods of time and many versions, like Flutter's TextInputClient may cause problems for developers if they are implemented rather than extended.
An annotation like @preferExtends would be needed to mark abstract classes like this.
Kind
Helps preserve forwards compatibility.
Good Examples
abstract class FrobComm {
void doSomething();
}
class MyFrobComm extends FrobComm {
@override void doSomething() { ... } // actually do something
}
Years later...
abstract class FrobComm {
void doSomething();
void doSomethingElse() { } // stub implementation
}
MyFrobComm still compiles and MyFrobComm.doSomething() callers still DTRT. Note of course that considerable thought is required by the abstract class developer to ensure that existing code will not break after doSomethingElse() is added.
Bad Examples
class MyFrobComm implements FrobComm {
@override void doSomething() { ... } // actually do something
}
Now MyFrobComm fails to compile when FrobComm.doSomethingElse (with a stub implementation) is added years later.
Discussion
Flutter has about 250 public abstract classes and it would likely be helpful to discourage developers from using implements with at least some of them.
Assuming we don't add language support for this fairly soon, we'd probably want to implement this by creating an annotation that could be added to class declarations indicating that they shouldn't be implemented.
@munificent I believe the language team has also had discussions about visibility / usability modifiers. It would be good to use that work to inform any set of annotations we might add.
The main class capabilities that we've discussed are (1) creating an instance (currently disabled using abstract), (2) creating a subclass (currently always enabled using extends), (3) creating a subtype (currently always enabled using implements), (4) applying a class as a mixin (currently enabled using with, for a restricted set of classes).
Some constraints can be (partially) enforced by accident: For instance, if a class C has at least one generative constructor (including redirecting ones), and all of them are private, then no class outside the declaring library can be a subclass of C.
In any case, the purpose of preventing implements could be as stated in this issue, that is, to ensure that newly added instance members will be a breaking change in fewer cases (name clashes are still breaking, but 'not implemented' errors won't happen). However, it could also be used to ensure that the invocation of a specific method will call a specific implementation (probably aided by @mustCallSuper), such that the actions taken there are known to happen.
So it's probably a good idea to abstain from naming one specific purpose, and just refer to the actual constraint, e.g., using @noExtends or @noImplements as in https://github.com/dart-lang/sdk/issues/46331.
The language team has had discussions about using negative mechanisms (like abstract, which takes away the ability to create an instance of a given class) or a positive mechanism (like open class in Kotlin, which enables subclasses).
I don't think it is crucial that any metadata based mechanisms like the one proposed here is exactly like an upcoming language mechanism. Of course, when @foo is changed to aFancyNewKeyword, it doesn't matter that the new keyword isn't actually spelled foo. But it is probably not even crucial that the metadata based approach and the language mechanism are both negative, or that they are both positive. In contrast, it could be a good opportunity to try out the practical value of this choice between negative and positive mechanisms. For instance, @allowsWith could enable mixing in a given class (which is able to do that, anyway), and @noImplements on C could prevent implements T whenever T is a subtype of C, etc. The point would be that @allowsWith would be rare and @noImplements would be rare, so it's useful to make the former positive and the latter negative, because we'd want to avoid having these annotations all over the place. But if we conclude that it's still better to change the polarity of a particular capability then it's still doable using tool support.
I recommend against this proposal. I think the recommendation to extend an abstract class, rather than implement an interface, simultaneously avoids the root problem, and encourages harmful behavior.
First, in the average case, I believe that the root problem that this proposal aims to solve is not the decision to implement an interface, but rather the framework's decision to add more behaviors to an existing interface. Adding behaviors to existing interfaces is a clear violation of the open-closed principle. While it may not always be possible to conform to the open-closed principle, I would strongly discourage the official Flutter/Dart orgs from publicly incentivizing developers to violate this principle in perpetuity.
Second, if one asks "why" an interface keeps expanding, it's likely that one will discover repeated violations of the single responsibility principle. The interface is likely adding responsibilities that belong within different roles. Furthermore, as the API expands along multiple dimensions, it's likely that many of these methods are left empty. After all, this proposal is based specifically on the idea that it's OK to leave methods stubbed, doing nothing. In practice, this means that many apps have implementations that don't do the expected things when those objects are passed to various methods. For example, imagine a new stub called fly() is added to an abstract class called Vehicle. Good news, it didn't break any apps. Instead, that empty stub secretly appeared in everyones' apps undetected. But now, somewhere, there's a method that takes in a Vehicle and that methods calls fly(). Unsurprisingly, that method actually expects the Vehicle to fly(). But it doesn't. In fact, the developer who implemented Vehicle never even noticed that fly() was added to the interface. What we see here is essentially a violation of the Liskov substitution principle.
We're now looking at likely violations of the "SOL" in "SOLID" principles.
Beyond the violations of software engineering principles, there's also a very concrete and pragmatic problem. You can only extend one class. This fact is probably the greatest area of frustration for all developers who use classical inheritance. Fortunately, Dart offers great alternatives with interfaces and mixins. But this proposal aims to eliminate half of those options.
It's interesting that TextInputClient is raised as an example. TextInputClient is probably always implemented by a State object, making it impossible to extend the abstract class. Furthermore, both TextInputClient and TextInputConfiguration are great examples where the root problem are the APIs, themselves, not the way in which external developers are using them. Those APIs are inherently monolithic and harmful to the expansion of Flutter's text capabilities. Those APIs should be fundamentally redesigned. I've spoken with @justinmc, @LongCatIsLooong, and @Renzo-Olivares about specific issues with those APIs. If TextInputClient were not so inherently unstable, it wouldn't pose an issue worthy of the recommended lint.
Asking developers to become less fragile to an unstable underlying interface does not help solve the problem of an unstable interface. In fact, I believe that this lint helps reduce a pain that developers should feel. It should be painful to work with monolithic, mutating interfaces. That pain should result in a redesign of those interfaces to be compositional, with explicit design towards future behavior expansion.
I just wanted to add here that an added benefit with being able to force a client to extend your class rather than implement it is that you are also able to maintain some internal assertions.
For instance, defining a PositiveDuration class which essentially extends Duration and adds an assertion in the constructor so that the duration is indeed positive. (Of course even extending it you could break that assumption but in that case it would really be intentional.)
I think the ability to have this kind of added safety is the main reason I would like to see some form of this lint or preferably annotation approach as described above.
I think being able to specify the intent of a class with more analyser support seems quite invaluable. I'm thinking here of for instance classes like ListBase, which are more or less written to be extended rather than implemented.
PositiveDuration is another great example of a poor software engineering decision. A Duration can be positive or negative. If you prevent negative values, you're not a Duration. That's not a legitimate sub-type. This is a violation of the Liskov Substitution Principle. This is what I'm talking about. Look at how quickly and easily this tool becomes an aid to make poor decisions.
What you want, if you need to enforce a positive duration, is a factory method, or some other instance creation tool. Not a subclass.
Subclassing is a WAY overused programming tool. It's also one of the least understood programming tools. Please do not make it easier to propagate bad decisions around subclassing.
If a class wants clients to conform to a special contract, put those details in the Dart Docs. Preconditions are a common documentation detail. If a developer is going to extend your class, then I think it's reasonable to expect that developer to read the docs. And, again, subclassing is hugely restrictive and easy to get wrong, I don't think many developers should be operating under the pretext that clients need to subclass their creations. Let's teach developers how to build more effective APIs, rather than incentivize common structural mistakes.