How do we define the restriction preventing unrelated private members from being combined?

[leafpetersen]

In Dart 2, we intended to enforce the property that a private member from a library could never be overridden outside of the library (see discussion in #2020 for examples how this can happen). The specified error around mixins was never fully implemented in the CFE (but was implemented in the analyzer). A similar loophole using extension and implements was never closed.

For private field promotion (see #2020) we propose to close both of those loopholes. The discussion in #2020 is not precise about how this is to be done. This issue is to clarify and resolve this issue.

In #2020 I proposed three possible resolutions:

• We might choose to disallow a class to implement a private member from a different library with the same name from two different classes entirely.
• We might choose to disallow a class to implement a private member from a different library with the same name from two different non-subtype related classes.
• We might choose to say that any time a class implements a private member from a different library with the same name from two different classes (or two different non-subtype related classes) we generate a stub which throws.

The first of these I believe works, but is quite draconian.

I believe we are not in favor of the third, since it seems to generate surprising runtime failures.

The second of these I believe does not work as phrased because of the following example:
//lib1.dart

class A {
  void _foo () {}
}

class B implements A {
  void _foo() {}
}

// lib2.dart

import "lib1.dart"
class C extends A implements B {}

This example is legal by the above criteria (since A and B are in a subtype relation) but allows B._foo to be overridden outside of its defining library, which breaks the private variable promotion proposal.

This might be fixable by restricting the direction of the subtype check, but it feels possibly overly restrictive. Consider:

//lib1.dart

class A {
  void _foo () {}
}

class B implements A {
  void _foo() {}
}

// lib2.dart

import "lib1.dart"
class C implements A, B {} // Legal, since A and B are in a subtype relation

class D extends B {} // Legal

class E extends D implements C {} // Error?

Here the superclass of E is not in a subtype relationship with C, but this code seems reasonable.

Defining this more directly on the members themselves may be more precise. A variant of a proposal from @stereotype441 would look something like the following:

If a class C in library L inherits a concrete member m which is private, and not accessible in L, and m is not a synthetic noSuchMethod thrower, then the concrete inherited declaration of m must already be an override of every other declaration of m in the super-interface graph of C, or it is an error.

• I exclude noSuchMethod throwers here, since we already allow new noSuchMethod throwing implementations to be added
• We probably should ignore compiler generated stubs when we talk about "the concrete inherited declaration of m"

This is a very non-local property to check, which is unfortunate. I don't see a clear approach that can be checked based solely on the super-interfaces however.

An alternative proposal from @eernstg is to track only "promotabilty" in the interface.

We mark private getters as promotable or not in their library, and make it an error for a concrete getter which is not promotable to override or implement a promotable signature.

This has much nice behavior WRT to local analysis, but doesn't solve the more general problem of wanting to forbid private members from being overridden in general. We could choose to use this approach for promotability, while also maintaining (and implementing in the CFE) the existing error around mixins. This would leave the ability to introduce new implementations of private members via extension + implements for non-promotable members.

cc @eernstg @lrhn @munificent @natebosch @jakemac53 @stereotype441 @johnniwinther @chloestefantsova @scheglov

[lrhn]

We don't generally want to prevent overriding private members, only overrides that are not intended by the author.

Or, more precisely, we worry about a concrete member being the implementation of an interface signature that the author of the concrete member did not intend it to be an implementation of.

Would this be a good time to introduce override as a language feature, rather than the annotation @override, and only allow overriding if the overriding member is marked with override.

Even without that, we could check that a concrete private member m named n of a class/mixin C does not become the implementation of a method signature s named n of an interface F unless

• C has F as superinterface, or
• there exists an interface G s.t. both C and F has G as superinterface, G declares a member signature s' with the name n and s and s' are equivalent (mutual subtypes).

In those cases, the signature s of F occurs in a superinterface of C, either itself or something s is intended to be the same as.

The two types need not be subtype related, but they do need to be subtype related to a common ancestor, and not modify the signature between the ancestor and the signature (the implementation may be more accepting).

(Making this an error will give the client library no recurse and the author of the private members no warning. We should warn, likely with a recommended lint, a library author if they have public classes and interfaces that can be combined into such a private member conflict error. They can then rename the private members, or ensure that they have a common superinterface, if combining is intended.)

[stereotype441]

Ok, so we have a bunch of ideas on the table right now:

1. (@leafpetersen's first bullet) Disallow a class to implement a private member from a different library with the same name from two different classes entirely.
2. (@leafpetersen's second bullet) Disallow a class to implement a private member from a different library with the same name from two different non-subtype related classes.
3. (@leafpetersen's variant of a proposal from me) If a class C in library L inherits a concrete member m which is private, and not accessible in L, and m is not a synthetic noSuchMethod thrower, then the concrete inherited declaration of m must already be an override of every other declaration of m in the super-interface graph of C, or it is an error.
4. (@eernstg's alternative proposal) We mark private getters as promotable or not in their library, and make it an error for a concrete getter which is not promotable to override or implement a promotable signature.
5. (@lrhn's suggestion) Check that a concrete private member m named n of a class/mixin C does not become the implementation of a method signature s named n of an interface F unless (a) C has F as superinterface, or (b) there exists an interface G s.t. both C and F has G as superinterface, G declares a member signature s' with the name n and s and s' are equivalent (mutual subtypes).

(And of course, variants of these proposals where instead of issuing a compile time error we generate a stub which throws. Whether we prohibit these classes entirely or generate stubs that throw is orthogonal to the discussion below.)

I think it's helpful to consider, for each of these proposals, what the corresponding rule has to be for promotability of private fields. I want the rule for promotability to look something like this:

• A private final field f in class C of library L is promotable provided that there is no class D in L which implements C but contains (or inherits) a concrete getter with the same name as f which is not a final field.

(Note: it's tempting to say that the class D only gets in the way of promotion if it's concrete, but this doesn't work because even if D is abstract, some other library could contain a concrete class that extends it.)

I believe this definition of promotability works with ideas 1, 3, and 4.

As Leaf explained above, it doesn't work with idea 2 because there could be a class in some other library which implements C but extends some supertype of C whose corresponding definition of f is not a final field. I won't try to come up with a rule for promotability for idea 2 because I don't think we're going to go in this direction.

As for idea 5, consider:

//lib1.dart
abstract class G {
  int? get _x;
}
class C implements G {
  int? get _x => ...;
}
class F implements G {
  final int? _x;
  F(this._x);
}
test(F f) {
  if(f._x != null) {
    print(f._x + 1); // UNSOUND!
  }
}

Under this idea, promotion of F._x is unsound because some other library might do this:

//lib2.dart
class D extends C implements F {} // Allowed because of the common interface class G

So for idea 5, we would have to tweak the rule for promotability to be something like this:

• A private final field f in class C of library L is promotable provided that there is no class D in L which implements either C or a class containing a getter that f overrides, but contains (or inherits) a concrete getter with the same name as f which is not a final field.

I think this would work. It's a bit of a mouthful but I'm sure someone can help rephrase it in a way that makes it more comprehensible.

Personally, my favorite is still idea 3. I think it strikes a good balance of not being too draconian but still allowing a simple intuitive rule for what's promotable. As for idea 5, I could probably live with it, but since it doesn't allow as much promotion as idea 3 I would like to better understand what the benefit would be.

As Leaf already mentioned, idea 1 is pretty draconian, and idea 2 has a soundness hole, so let's set those two aside. That leaves us just with idea 4. I like that it's simple and highly targeted at the feature we're trying to implement right now, and I like that it opens the door to adding a notion of stable getters to the language in the future. I worry that it's not general enough, though. In my mind, even though field promotion is the motivation for this restriction, the fundamental need is more general: we want to make it possible to discover all the possible dynamic targets of a private member invocation in library L based solely by looking at the classes in L (and possibly their transitive superclasses). Ideas 3 and 5 both address that general need by prohibiting "unintended" overrides (they just differ in what they count as "unintended").

[stereotype441]

In light of the change in how we've decided to approach field promotion (documented in #2020 (comment)), I believe this issue is now moot, so I'm closing it. If we change our minds, or decide to move forward with this issue anyway, we can always reopen it.