Enable promotion for private final fields and fields on non-escaping private classes

[leafpetersen]

Field accesses in general are not subject to promotion even for final fields, since it is in general not possible to know that the field is not overridden with a getter without seeing the whole program. In a limited set of cases, with a small set of changes to the language, library level analysis could show that certain field accesses are safe to promote.

This was discussed previously here and here.

Level 0: Allow property accesses to final, private fields on this to promote.

We could allow a property access on this to a final private field which is not overridden in its declaration library to promote if we made the following change to the language.

Disallow mixins from inducing concrete overrides of private members from other libraries.

In current Dart (as of 2.16) a mixin can be used to induce an override of a private member from another library. The following code has an error in the analyzer, but not the CFE, and prints 0 followed by 1.

// lib1.dart
class A {
  final int _privateField = 3;
  void testThisCall() => print(_privateField);
}

class B {
  int _backingStore = 0;
  int get _privateField => _backingStore++;
}

import "lib1.dart";

class C extends A with B {
}
void main() {
  new C()..testThisCall()..testThisCall();
}

A slightly more elaborate example runs with no errors in both the CFE and the analyzer, and again prints 0, 1:

// mixin0.dart
import "mixin1.dart";

class A {
  final int _privateField = 3;
  void testThisCall() => print(_privateField);
}

class B {
  int _backingStore = 0;
  int get _privateField => _backingStore++;
}

class C extends D with B {}

// mixin1.dart
import "mixin0.dart";

class D extends A {}

void main() {
  new C()..testThisCall()..testThisCall();
}

To enable Level 0, we must therefore remove this loophole. I believe it is sufficient to enforce that it is uniformly an error to cause an override of a private field (concrete or abstract) in any library outside of the library in which the private field is declared. This is a breaking change, but likely to be non-breaking in practice.

Level 1: Allow property accesses to final fields (private or not) on private non-escaping classes to promote.

A class is non-escaping if it is private, and it is never implemented, mixed in, or extended by a public class either directly or transitively, nor given a public name via a typedef.

All overrides of the members of a non-escaping class can be observed locally, and an access to a non-overridden field could be allowed to be promoted, whether on this or on a different instance, and whether the field name is private or public.

Level 2: Allow property accesses to final, private fields on instances other than this to promote.

For Level 0, it is sufficient to ensure that all potential overrides are visible. Since promotion is restricted to accesses on this, it is not necessary to ensure that there are no other implementations of the field. For Level 2 promotion, we must also ensure that all non-throwing implementations of a private field are visible. This entails the following steps (at the least, are they sufficient?).

Don't delegate private names to noSuchMethod.

In current Dart (as of 2.16), noSuchMethod can be used to provide a concrete implementation of a private name from a different library.

// lib1.dart
class A {
  final int _privateField = 3;
}

void testOtherCall(A a) => print(a._privateField);

// main.dart
import "lib1.dart";

class E implements A {
  int _count = 0;
  dynamic noSuchMethod(_) => _count++;
}

void main() {
  var e = new E();
  testOtherCall(e);
  testOtherCall(e);
}

This code prints 0, 1. The implicit method forwarder in E delegates calls to _privateField to noSuchMethod which can provide an implementation. To avoid this, I propose that forwarding stubs for private members from other libraries always throw, rather than delegating to noSuchMethod. This is a breaking change, probably unlikely to be significant, but it is possible that there may be uses of this misfeature (e.g. mockito?).

Forbid private members from being implemented by unrelated private members outside of their defining library.

For accesses on instances other than this, it is not sufficient to prevent unexpected overrides of private members - we must also prevent unexpected implementations of private members. For example, the following is valid in current Dart (2.16):

// lib1.dart
class A {
  final int _privateField = 3;
}
class B {
  int _backingStore = 0;
  int get _privateField => _backingStore++;
}

void testOtherCall(A a) => print(a._privateField);

// main.dart
import "lib1.dart";

class D extends B implements A {}

void main() {
  var d = new D();
  testOtherCall(d);
  testOtherCall(d);
}

This code prints 0, 1, since D brings together two previously unrelated private members to provide an implementation of A which uses the concrete private member from B.

To prevent this, there are a few different options.

• 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.

All of these are technically breaking. I suspect they are unlikely to be very breaking in practice, but this is something we would want to validate with corpus analysis.

Discussion

Level 0 and Level 1 seem like clear wins to me, with fairly minimal cost. I don't see much down side to pursuing them, even if they don't fully solve the issue. Level 1 requires generalizing promotion to paths (e.g. allowing a.b to be promoted), and we would need to define exactly what paths we promote and under what circumstances it's valid to do so, but I'm not too concerned about this (a simple story might be that a promotable path is either a promotable property access on this, a promotable local variable, or a promotable property access on a promotable path).

Level 2 potentially has more cost, and I'm less confident that I've captured all of the corner cases. Corpus analysis and careful thought would be required. However, it does feel unsatisfying to not support promotion of private fields on non-this instances, so I'm tempted to pursue this further.

cc @mit-mit @lrhn @eernstg @munificent @jakemac53 @natebosch

[lrhn]

Level 0 is probably sound. (I say probably because I can't find any reason it shouldn't be, but it's always possible to miss something. This is like security — you only need one flaw in one place, no matter how obscure, to be unsound.)

Because we only promote instance fields on this, we avoid worries about implicit interfaces and noSuchMethod.

We currently disallow mixins from mixing private fields onto private fields, but not shadowing private fields in general. The CFE apparently fails to implement that restriction. We could extend that to any override of a field, or of any member. That might be breaking, if someone deliberately has a mixin that is intended to override private members. It's probably rare, but not impossible, and I can't see any workaround if we blanket-disallow overriding privates in mixin applications.
On the other hand, it would change the current restriction from just affecting fields (instance variables). I never liked that, it's like it's leaking the implementation of the language, at a point where we wanted to be abstract — a getter is a getter is a getter.

The getter of a final field is currently the only kind of instance getter where we can predict future behavior from current behavior, so if we want to promote any current kind of getter, that's really the only thing we have to work with. (Otherwise we'd need to introduce "stable getters").

This proposal has the advantage that the promotion only works inside the same library, so you can't break anything for anyone other than yourself. That limits the potential issues. We'd probably want to add a warning if a mixin declares a getter with the same name as an instance variable of any class in the same library. Maybe only if the mixin's on type allows that class - but not necessarily, since it's possible for a separate library to extend the field's class with an interface that allows the mixin. So, you should be warned that your mixin can't be applied to that class, and you might want to renamed the mixin's private members. Maybe only for public mixins.

Level 1 is probably sound too. If the class has no non-private subclasses (declared or type-aliased), then all subclasses must exist in the same library, and we can see whether the final field is ever overridden. If not, it's safe to promote accesses.
Again, it only works on this, but that's probably not even necessary, since no-one elsewhere can implement the types either.

Level 2 sounds reasonable too.
I like not forwarding implicit inaccessible private member implementations to noSuchMethod and making them just throw NoSuchMethodError directly. We should do that no matter what!

I'm more worried about making it a compile-time error when private members of another library clash.
Firstly because it leaks implementation details. "These two interfaces are incompatible because of things you can't see" is not a great experience.
Secondly, we have to define when the two declarations are incompatible. That comes back to where the declaration comes from "originally".
If B had implemented A already, we wouldn't complain about writing extends B implements A (other than it being unnecessary). Even though both B and A then declared _privateField, they would both be "the _privateField of A".

This sound more like a step towards declaration based virtual members rather than name based ones. That is, the _privateField of A and of B , as declared here, are different virtual members. If a class implements both, you'd have to say which one you're calling (C# syntax is o.A::_privateField or (o as A)._privateField).
That makes a lot of sense in the C# class/object model, where you already have overloading so the name doesn't determine the method. In Dart, the name has so far uniquely determined the member. Saying whether two things with the same name are "different" declarations or not is an entirely new concept. We can probably do it, but it'd be work done entirely for inaccessible members of other libraries.

And, as you say, we want to recognize "stable" expressions, because we can only allow promotion of a final field on an object reference if that object reference definitely doesn't change between check and use.
(An expression is "library-locally stable" between two occurrences if: it's this, a local variable which cannot change between the two occurrences, a final static private variable, or a "definitely non-overridden" final instance variable member access on a stable expression or on super, where "definitely non-overridden" is the property of final instance fields that we are introducing here at level 0 or 1).

---

All in all, this gives promotion only of certain private fields. That's probably enough for many use-cases.
It does not help with if (o.publicGetter != null) o.publicGetter.method();, which a binding check does (if-variables or similar).

We can also trivially promote private static final variables the same way. A private static final variable is definitely not overridden.

It breaks getter/field symmetry to only promote fields, not getters, but it only does so locally in the same library. If you want to change the field to a getter, you are also in position to change all the promotion sites. If you add something which breaks promotion, you are already editing the library, and will see it immediately.

It only works for some private final fields, depending on the pattern of declarations around it, and if the user can't figure out the border between promotable and non-promotable, then it's going to be a frustrating feature. The rule that people will follow is probably going to be that there can't be any other member declaration with the same name in the same library. That's simple and it works.
(It's actually just another getter declaration, but since getters and methods have different naming conventions, the simpler rules is correct enough.)

---

Maybe we could just do the simple version:

A private final instance variable is locally unique if there is no other instance variable, getter or setter with the same basename declared in the same library.
You can't implement inaccessible private members using noSuchMethod. The implicitly introduced implementations throw instead of calling noSuchMethod.
An expression is locally stable between two occurrences of that same expression (it's the same expression if any unbound identifiers denote the same declaration of the same scope) if the expression is:

• this
• an expression denoting a private static final variable,
• e._member where e is either a locally stable between the two occurrences or e is super, and _member is the name of a locally unique final instance variable. (We can even allow e to have type dynamic!)

[stereotype441]

@leafpetersen:
In the issue description you gave this example:

// mixin0.dart
import "mixin1.dart";

class A {
  final int _privateField = 3;
  void testThisCall() => print(_privateField);
}

class B {
  int _backingStore = 0;
  int get _privateField => _backingStore++;
}

class C extends D with B {}

// mixin1.dart
import "mixin0.dart";

class D extends A {}

void main() {
  new C()..testThisCall()..testThisCall();
}

And then said:

To enable Level 0, we must therefore remove this loophole. I believe it is sufficient to enforce that it is uniformly an error to cause an override of a private field (concrete or abstract) in any library outside of the library in which the private field is declared. This is a breaking change, but likely to be non-breaking in practice.

I don't think this rule removes the loophole, because in the example, the override of a private field is happening in the declaration class C extends D with B {}, and that's the same library as the declaration of the private field.

But I think that's ok, because there's not actually a loophole in this example. In your definition of level 0, you say:

We could allow a property access on this to a final private field which is not overridden in its declaration library to promote if we made the following change to the language.

And therefore, since class C extends D with B {} induces an override of the private field in the same library as its declaration, that means that _privateField won't be subject to type promotion anyhow.

So I think all we have to do is port the analyzer's PRIVATE_COLLISION_IN_MIXIN_APPLICATION error over from the analyzer to the CFE; I don't think it needs to be extended in any way.

[stereotype441]

For level 0, I think we need to add an exception for extension declarations: they can't safely promote private instance fields on this, because inside an extension declaration, this is effectively just another variable, so there's no guarantee that the actual runtime type extends the declared type (it might just implement the interface).

We could support promotion of private instance fields on this at level 2, though.

[munificent]

I like all three levels you have here and I think the minor language changes required to enable them are worth doing.

At some point, I think we should consider level 4: Allow promotion on access to a public field from another library iff:

1. The field is final.
2. The field is not overridden by any other class in the package where it is defined.
3. The field is in a sealed concrete class so cannot be overridden outside of the package.
4. The author of the class has opted into promotability on the file with some kind of annotation.

1 and 2 are the same restrictions as the other levels. 3 requires something like packaged libraries to be able to annotate classes as sealed and non-interface.

4 requires some new annotation or modifier we'd have to define and specify. I think it's important to require this opt-in because otherwise it means changing a field to a getter is a breaking API change. I think "promotability" should be considered part of a class's public API and something the API designer can explicitly control. If we opt all possible final fields in to it by default, I worry authors would get in the habit of "opting out" by preemptively wrapping every field in a getter like they do in Java.

[Levi-Lesches]

4. The author of the class has opted into promotability on the file with some kind of annotation.

Like stable getters (#1518) ?

[munificent]

Yes, like that. I don't know if I am sold on committing to stable getters—to being able to define non-fields that are promotable—but at least being able to control which fields are seems worth doing to me.

[munificent]

... but at least being able to control which fields are seems worth doing to me.

I think all of them are stable (do you know an actual (non-theoretical) counterexample?).

All final fields are stable, yes. The problem is that an API maintainer may not want to commit to that property being a final field henceforth and forever. If we implicitly allow any public final field to promote, then it means changing that field to a getter is a breaking API change. If we don't have some sort of annotation to control whether a public final field allows promotion or not, then API authors have no ability to control that aspect of their public API.

[munificent]

If we implicitly allow any public final field to promote, then it means changing that field to a getter is a breaking API change. I

When you change it to getter, just declare your getter "stable"?

Think about it going in the other direction: I'm an API designer. I'm creating a class that exposes some property. Right now, I can implement it correctly just using a final field. But I don't know if that will always be the case. I want to give my future self the freedom to change that field to a non-stable getter without having it be a breaking change. That means that today I want this final field to not allow promotion.

If we allow public final fields to promote and don't give class authors any way to opt out, they will just pre-emptively wrap every field in a getter (just like you see in Java) in order to not paint their API into a corner.

[leafpetersen]

@tatumizer this discussion is getting very far afield from the issue topic, can you please move discussion of stable getters to the stable getter issue? Thanks!

[eernstg]

I took a look at the different levels in the initial comment, and I think there's a need to make a couple of small adjustments, or at least clarifications.

tl;dr

• For level 0, mixin applications, I believe we need to treat abstract declarations with a non-implemented signature the same as concrete declarations; the same applies for member signatures with no implementation (e.g., from an implements clause).
• For level 0, I believe we need to change noSuchMethod stubs that are forced by privacy such that they will throw rather than forward.
• For level 1, I'm just making a couple of rules explicit. I believe we can do this without language changes.
• For level 2, I agree that we need to make noSuchMethod stubs that are forced by privacy throw rather than forward; and I'd suggest that we introduce implicit stability in order to single out which cases are errors, when an override is caused by a declaration outside the declaring library. We could spell out what it means to be 'unrelated', but I suspect that it's more useful to rely on this special case of stability.

About level 0: Allow property accesses to final, private fields on this to promote:

The proposed language change is to disallow mixins from inducing concrete overrides of private members from other libraries. At the end of the section there is a parenthesis '(concrete or abstract)' that may be aimed at the final instance variable or at the overriding declaration, but let me make it explicit:

It is not quite sufficient to prohibit 'concrete overrides' if that is taken to mean that the mixin application must have a concrete declaration of the given private member. It is enough that the mixin has an abstract declaration with a signature that the inherited concrete declaration does not implement:

// Library ex7lib.dart.

class A {
  final num? _x;
  A(this._x);

  void foo() {
    if (_x != null) print(_x);
  }
}

mixin M {
  int? get _x;
}

// Library ex7.dart.
import 'ex7lib.dart';

var b = false;

class B extends A with M {
  noSuchMethod(Invocation i) => (b = !b)? 10 : null;
  B(int x): super(x);
}

void main() {
  B(1).foo();
}

Note that it's also enough if the mixin has the member signature int? get _x; from one of its superinterfaces.

So we'd need to enhance the error on mixin applications such that it flags situations where the getter of an inaccessible private final instance variable does not implement the corresponding member signature in the interface.

Alternatively, and better, we could change the noSuchMethod forwarders that are forced by privacy to "noSuchMethod throwers": When a noSuchMethod stub is generated for an inaccessible private member (that is, a private member in a different library), it should throw rather than forwarding the call to noSuchMethod. This remedy is already proposed for level 2.

We'd need this anyway, because of the following:

// Library ex8lib.dart.

class A {
  final num? _x;
  A(this._x);

  void foo() {
    if (_x != null) print(_x);
  }
}

abstract class B {
  int? get _x;
}

// Library ex8.dart.
import 'ex8lib.dart';

bool b = false;

class C extends A implements B {
  C(int? n): super(n);
  noSuchMethod(Invocation i) => (b = !b)? 24 : null;
}

void main() {
  C(1).foo();
}

With that, I believe level 0 would be sound.

The CFE actually does/did not implement the noSuchMethod forwarders forced by privacy entirely correctly (cf. dart-lang/sdk#47923), which causes the above two programs to be rejected by the CFE. This means that, luckily, it would not be a breaking change to make those noSuchMethod stubs throw.

About level 1: Allow property accesses to final fields (private or not) on private non-escaping classes to promote.

.. assuming, of course, that the current library does not contain a subtype _S (extends, with, or implements) of said non-escaping class _C, where that getter has an implementation which is not stable (which could be approximated as "anything other than a final instance variable", or it could simply be "any implementation at all which is not the one in _C").

Note that this implementation could be a noSuchMethod forwarder: If _S implements _C and _S has a non-trivial noSuchMethod, then we'd get an implementation which is a noSuchMethod forwarder, and that isn't stable. Also note that this noSuchMethod forwarder would be a regular one (not 'forced by privacy'), so we have no proposals to make that one throw.

Maybe this should be renumbered as level -1, because it can be done without language changes at all?

About level 2: Allow property accesses to final, private fields on instances other than this to promote.

The first remedy is Don't delegate private names to noSuchMethod.

I think it would be conceptually justified, and hopefully essentially non-breaking in practice, to make noSuchMethod stubs throw rather than forward, whenever they are 'forced by privacy'. We could restrict this to the case where those stubs implement the implicitly induced getter of a private final instance variable in another library, but it would be nice if we could make this change for all noSuchMethod stubs that are forced by privacy.

The conceptual model would be that it is supported to obtain an implicit and automatic implementation of any missing member. If it would be possible to write an implementation for that member then the generated implementation will forward to noSuchMethod; otherwise (which is exactly when forced by privacy), it will throw. This means that private members cannot be implemented/overridden to do computations in a different library, so we can trust an invocation of a private method that doesn't throw to be running the implementation in the current library, and not any other implementation. That seems to be a valuable guarantee to have for application logic correctness, which means that we'd want it even in the cases where soundness isn't endangered.

The second remedy is Forbid private members from being implemented by unrelated private members outside of their defining library.

This essentially amounts to a stability check: If we consider the getter of a private final instance variable with no in-library overrides (or only stable ones) to be stable, then the rule is that an implementation that isn't stable is not a correct override.

So, repeating and commenting on the example:

// lib1.dart
class A {
  // Stable: No non-stable overrides exist in lib1.dart.
  final int _privateField = 3;
}
class B {
  int _backingStore = 0;
  // Non-stable, for any definition of stability.
  int get _privateField => _backingStore++;
}

void testOtherCall(A a) => print(a._privateField);

// main.dart
import "lib1.dart";

class D extends B implements A {} // Compile-time error.

void main() {
  var d = new D();
  testOtherCall(d);
  testOtherCall(d);
}

The error arises at the declaration of D because B._privateField is the given implementation, and it is not a correct override of A._privateField (because B._privateField isn't stable).

Again, stability could mean, just to get started, that a getter is stable if and only if it is implicitly induced by a final instance variable, and it isn't overridden by a non-stable getter.

If we use this criterion then we'll get soundness at the smallest possible cost, because we allow for implementations where a final instance variable is implemented by another final instance variable (which is fine!), and we don't have to find suitable rules about what it means to be unrelated.

[leafpetersen]

A note on stability: this implies that changing a private getter or field in a way that makes it non-stable can break unrelated downstream code. This isn't a deep change - it's already the case that adding a private getter or field, or changing the type of one can break unrelated downstream code, but worth noting.

If we made private members unreachable via dynamic dispatch, we could potentially make privacy class (hierarchy?) specific and avoid all of these issues, I think. I wonder how breaking that would be?

[lrhn]

Changes to private members should only affect the library itself, so it's not breaking unrelated code (considering the entire library as related to itself).

If we introduce the rules that a mixin application cannot override an inaccessible member, then adding a private member with the same name as another member in the same library (unless both override the same super-interface members), where at least one is in a mixin-able declaration, is potentially breaking unrelated code. We should probably have a warning for that.

[leafpetersen]

Changes to private members should only affect the library itself, so it's not breaking unrelated code (considering the entire library as related to itself).

No, sorry, I really mean unrelated code, in the same way that is true currently. Right now, in existing Dart, adding a private member to a class is a breaking change, that can break arbitrary downstream code. The (well, a) reason is that some other code in some other library may be using the class as a mixin on another class from the original library with a member with the same private name, and if those members have conflicting types, the mixin application will suddenly cause a type error.

Making stability part of the API in the same way that types are part of the API means that the same breakage can now happen even if the types are compatible. Again, not a sea change, just another instance of the same problem.

And yes, forbidding mixins from inducing overrides makes all such examples errors.

[InMatrix]

It would be really useful to estimate the % of error cases each level proposed here can solve. Maybe we can do some corpus analysis using internal Google code? If, say, 80% of error cases can be covered, it's fine to leave the rest 20% to additional tooling support such as dart-lang/sdk#47588.

[mit-mit]

@munificent didn't you do that analysis?

[munificent]

That sounded familiar, but it took me a while to dig it up. Yes, I did a very rudimentary bit of digging. Here's the old email:

I went through all of the nullable fields in dart_style. Obviously, this is just one codebase with its own possibly idiosyncratic style, but I figured some data is better than none. There are 33 nullable fields spread across 17 classes. I believe none of them are intended to be overridden or implemented.

• 11 of the 33 fields are final.
• 14 of the 22 non-final fields are never assigned null.
• 11 of the 22 non-final fields are never witnessed in the null state outside of their containing class.

Based on this small amount of anecdata:

• If field promotion only applied to some notion of "sealed" fields, that wouldn't be a problem since all of these nullable fields are effectively sealed.

• Only promoting final fields would cover a third of the cases.

• The ability to check whether a late field has been initialized would enable about 2/3 of the non-final fields to use late and become non-nullable.

The dart_style codebase is kind of interesting because it does a lot of lazy initialization. Much of it is sort of a dynamic programming style. I don't know how well the numbers here would correlate to other codebases. We could automatically gather some data like this by looking at the nullability of assignments to nullable fields, but that kind of analysis is a little beyond my expertise. Might be worth talking to @pq and @brianwilkerson if we think it would be useful data to have.

It is fairly easy to scrape a corpus to see how many nullable-annotated fields are final/non-final. I went ahead and did that on 2,000 Pub packages:

-- Nullable fields (3062 total) --
   2268 ( 74.069%): non-final               ===========================
    683 ( 22.306%): final                   =========
     80 (  2.613%): late final              =
     20 (  0.653%): static non-final        =
     10 (  0.327%): late non-final          =
      1 (  0.033%): static const non-final  =