Analyzer should block static extensions on @sealed classes

[srawlins]

As per the static extension spec's comments on sealed classes:

If we introduce sealed classes, we may want to consider whether to allow extensions on sealed classes, since adding members even to a sealed class could still be a breaking change.

One of the reasons for having sealed classes is that it ensures the author can add to the interface without breaking code. If adding a member changes the meaning of code which currently calls an extension member, that reason is eliminated.

Since it's possible to add extensions on superclass (including Object), it would not be sufficient to disallow declaring extensions on a sealed class, you would have to disallow invoking an extension on a sealed class, at least without an explicit override (which would also prevent breaking if a similarly named instance member is added).

Oof. This certainly adds some complexity. We can at least do the first part ("disallow declaring ...") easily.

CC @lrhn @bwilkerson @matanlurey @nex3 (Matan and Natalie are the primary requestors of sealed, I believe, and the Sass package uses @sealed a lot.)

[matanlurey]

I don't think there is any reason to block station extensions on @sealed classes. The goal of @sealed is to block inheritance, which in turn makes evolution or standardization of an object structure impossible. For example if I write:

@sealed class User {
  final String firstName;
  final String lastName;
  User({this.firstName, this.lastName});
}

... and someone wants to write:

extension FullNameUser on User {
  String get fullName => '$firstName $lastName';
}

... I think that not only is fine but a totally cool use of extensions.

[nex3]

I'm surprised that native members take precedence over extension members with the same name. It seems like this is setting all users—not just those with sealed classes—up for backwards-compatibility headaches. Generally speaking, more local names tend to shadow names defined elsewhere (for example, a class defined in my library will shadow an imported class with the same name)—why isn't that the case here as well?

[bwilkerson]

@leafpetersen @lrhn @eernstg For @nex3's question.

[lrhn]

Shadowing of names is based on proximity (lexical scoping, with imports being treated as further away than local declarations). The names of extensions work the same.

Member invocations are not scope based in any way. If you do o.foo, then until now, the run-time type of o, not matter where that is declared, determines which member is invoked.

With extensions, we now have a situation where o.foo can mean more than one thing (instance member on static type of o or extension member declared on static type of o).

There are a number of simple options:

• Always let the extension win.
• Always let the instance member win.
• Always make it an error to have a conflict.

Always making it an error makes it a compile-time error to add either an instance member or an extension member. The other two options make it an error to add the preferred member when the other one already exists.

The reason to let instance members take precedence is to make code more readable. If you see Future f = ...; f.then(...);, then you can trust that you are calling the Future interface's then method. If extension methods could replace instance members, you can never be certain what an invociation actually invokes. Sure, it would be "nice" to mess around with existing classes and replace methods you disagree with, but that's not good for readability or maintainability. Also, there is no way to override the extension member if it takes precedence over the instance member, so we'd have to add that, say f.Future::then(...). Defensive programming would encourage you to use that everywhere. We need an override for extensions anyway because there might be multiple applicable extensions, so there is no need for an extra feature.

Adding an instance member to an existing interface is already a breaking change if anybody implements the interface. It is something you may do anyway because it's necessary, and you want to evolve the class. If you don't want to do that, you can add an extension member instead. In either case, it will break existing code which adds another extension member with the same name to a similar or less specific type. So, we decided that letting instance members take precedence was putting the issues in the same places where you already had to consider them, instead of introducing new issues.

So, talking about a "more local name" is not well-defined for member accesses at all. In most cases, both inteface type and extension will come from a different library. One may think of extensions as "overlays" on the interfaces, and therefore them being "closer" or "shadowing", but that is not the case, and not really a good model. Instance members do not shadow extensions either, the presence of an instance member means that extensions are simply not considered at all, but I guess you could consider instance members as shadowing for invocations - the are closer than the extension because they are on the object itself, whereas extensions are what you do if you look trough the object and find nothing.

[nex3]

The reason to let instance members take precedence is to make code more readable. If you see Future f = ...; f.then(...);, then you can trust that you are calling the Future interface's then method. If extension methods could replace instance members, you can never be certain what an invociation actually invokes.

This is just as true the other way around: if I'm invoking iterable.slice(...), I want to trust that I'll continue to call the slice() extension method that I wrote and tested my code against. If this instance happens to have a method named slice() with subtly (or substantially!) different semantics, that could easily break my code.

It's unlikely that I have an extension method in my scope that I'm unaware of. Yes, it could be possible for a package to suddenly shadow Future.then() where it didn't before, but no one is likely to do so. On the other hand, it's fairly feasible for library code to have an instance passed in that defines any old member without the library author ever laying eyes on that instance's API.

Sure, it would be "nice" to mess around with existing classes and replace methods you disagree with, but that's not good for readability or maintainability. Also, there is no way to override the extension member if it takes precedence over the instance member, so we'd have to add that, say f.Future::then(...). Defensive programming would encourage you to use that everywhere. We need an override for extensions anyway because there might be multiple applicable extensions, so there is no need for an extra feature.

I agree that it would be very bad practice for an extension to knowingly override an existing method on an interface. We have other ways of discouraging that sort of thing, though, such as making it an analysis warning.

Adding an instance member to an existing interface is already a breaking change if anybody implements the interface. It is something you may do anyway because it's necessary, and you want to evolve the class. If you don't want to do that, you can add an extension member instead. In either case, it will break existing code which adds another extension member with the same name to a similar or less specific type. So, we decided that letting instance members take precedence was putting the issues in the same places where you already had to consider them, instead of introducing new issues.

Adding an instance member isn't currently an issue for sealed classes, which is what this issue was originally about. Your proposed extension method semantics would make that a breaking change where it wasn't before.

Even for non-sealed interfaces, the decision comes down to two options:

• Allow instance methods to shadow extension methods, and increase the set of breaking changes caused by adding an instance method.
• Allow extension methods to shadow instance methods, and increase the set of breaking changes caused by adding an extension method.

Which of these options will cause fewer problems in practice? The latter: extension methods always know the interface they're extending and can avoid shadowing existing names, whereas a class doesn't and cannot know the set of all libraries that define extension methods for it.

[lrhn]

These are good points (I don't entirely disagree, even though I may weigh some trade-offs slightly differently).

If we act as if any extension in scope is always deliberately there, then it might be reasonable to consider it a stronger signal than the interface member. The fact that there is no override to avoid the extension member is not an insurmountable problem, you can always choose to not import the extension without an prefix (although that prevents any use of its other members as implicitly invoked extension members too, we can't hide a single extension member).

The question is then whether "extensions are always deliberate" is a reasonable assumption. For example, if dart:core contained an extension, it would likely be included with no deliberation by the user, and it would prevent any other class matched by the extension from trying to implement the same method. I'm not entirely sure that it is a safe assumption. I expect packages to start introducing extensions, and those extensions will then be imported implicitly by anyone importing the package. It's not breaking as such, it'll be a new version of the package, so it should be assumed to introduce new names which can conflict - that can happen every time you upgrade a package.

(Would you consider making a request in the language repo for reconsidering the member selection precedence?)

[eernstg]

@nex3 wrote:

I want to trust that I'll continue to call the slice() extension method that I wrote and tested my code against.

I think we should address this kind of issue in a symmetric manner.

Whether or not the receiver type is @sealed, we can have breakage in both directions: A new version of the package that delivers the receiver class is updated, a new instance method is added, and an extension method invocation could be captured by this new instance method. Or some update, or new dependency, or additional import could add a new extension to the current scope and change an instance method invocation to an extension method invocation. So the priority of extension methods vs. instance methods will never be able to eliminate this kind of breakage, we just have to choose in which direction we want to have the breakage.

So how about this?:

extension methods always know the interface they're extending

That wouldn't actually be true in all cases: A new version of said interface wasn't known to the authors of the extension at the time when it was written. So we can't trust extension authors to take these name clashes into account, even if there's a general agreement to try to do that.

I think the priority that we have today ("instance members always win") is conceptually attractive, because it means that we can read code and trust existing knowledge about a given receiver type (if I know that C has a method foo then I can trust myC.foo() to call the instance method, no matter which and how many extensions we have in scope).

With the opposite priority we would give extension methods the special power that they can override instance methods (which may be cool and convenient in some cases), but it would mean that readers of the code would always need to take extension methods into account in order to understand the effect of a given method invocation. I suspect that the number of wasted developer brain cycles will be bigger if we always need to think "maybe it's an extension method?" (and that's for every method call, that is, every line of code!) as opposed to only having to think so when the given member name is unknown in the static type of the receiver.

However, any of these kinds of breakage could actually be handled using a symmetric non-language approach if our tooling would allow for detecting re-binding events in existing code: "Here is myC.foo(), and it resolved as MyExtension<int>(myC).foo() in commit 38792dd, but in commit 6ffda779 it calls C.foo".

We might still need to edit some code after a package version upgrade, or whatever the trigger is, but the crucial first step is to get notifications about potential bugs of this kind.

I'm not saying that it is easy (what does it mean to be "the same code" in two versions of a library?), but it might be quite usable even with a rather simplistic approach (starting with "this library wasn't changed at all, so every piece of code in here is the same").

Based on this, I think there is no need for special rules about sealed classes.

[leafpetersen]

This issue is by far my biggest concern with the choice to prefer instance members over extension members. One of the main benefits of sealing is to allow non-breaking evolution of classes.

Based on this, I think there is no need for special rules about sealed classes.

I quite disagree with this. I think if we add sealed classes, we will need to say that extensions methods do not apply to them at all. Probably we would then want to add the ability to write extension methods which do take precedence over instance members (perhaps by declaring them with an override modified).

The ship will have sailed for int, String, and bool though - we will already have lots of extensions on those, and so will never be able to add any members to them.

[leafpetersen]

I don't think there is any reason to block station extensions on @sealed classes. The goal of @sealed is to block inheritance, which in turn makes evolution or standardization of an object structure impossible. For example if I write:

@matanlurey Are you not worried that it now becomes a breaking change to add a fullName method to the User class?

[leafpetersen]

I filed an issue in the language repo with a summary of this discussion so far. I'd suggest that we continue further discussion of the instance vs extension member precedence choice there.

[srawlins]

OK I think since it's been resolved that since instance methods take precedence over extension methods, then @sealed classes can have extension methods defined on them. No changes to @sealed or static analysis needed.