search

dart-lang / language

Design of the Dart language
Other
2.68k stars 205 forks source link

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

Closed leafpetersen closed 1 year ago

leafpetersen commented 2 years ago

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.

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 commented 2 years ago

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:

stereotype441 commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago
  1. The author of the class has opted into promotability on the file with some kind of annotation.

Like stable getters (#1518) ?

munificent commented 2 years ago

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 commented 2 years ago

... 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 commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

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

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. https://github.com/dart-lang/sdk/issues/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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 https://github.com/dart-lang/sdk/issues/47588.

mit-mit commented 2 years ago

@munificent didn't you do that analysis?

munificent commented 2 years ago

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.

Based on this small amount of anecdata:

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  =
leafpetersen commented 2 years ago

The following issues are also relevant here:

leafpetersen commented 2 years ago

I filed https://github.com/dart-lang/language/issues/2275 to discuss how to specify a restriction to prevent private members from being overridden outside of their library.

stereotype441 commented 2 years ago

We discussed this at length in the language team meeting today, and decided to shift our approach a bit. Rather than ensuring soundness of promotion in library A by restricting the way classes and mixins can be combined together in library B, we want to simply restrict the notion of what fields in library A are considered "promotable" to just those fields that can be promoted soundly without onerous breaking changes to the language.

The new rule would be: an instance field is promotable only if (a) it is private, (b) it is final, and (c) all other concrete instance getters with the same name in the same library are also final fields.

It might seem a little weird at first that a concrete instance getter (or non-final field) in one class should be able to inhibit promotion of a final field in some other, unrelated class. But we think it's consistent with the fact that in Dart, privacy is library-based rather than class-based.

To make this sound, we only need to make one breaking change to the language: Don't delegate private names to noSuchMethod. This won't be trivial; there are a few uses of mocks in the wild that would be broken by it, and I'm currently doing some research to see how bad the impact will be. But it will close an important privacy hole in the language, so it still seems like a good thing to do. And if people are frustrated by the breaking change, at least we'll be giving them field promotion in exchange, which hopefully will make them happier 😃.

Notably, this approach means that:

As a side note, this approach extends nicely if we ever wind up implementing a notion of "stable getters", because we'll be able to just change the rule from "all other concrete instance getters with the same name in the same library are also final fields" to "all other concrete instance getters with the same name in the same library are either final fields or stable getters".

Credit to @munificent for the new proposal, although it's quite similar to what @lrhn proposed in this comment.

jodinathan commented 2 years ago

will this be shipped in 2.18?

leafpetersen commented 2 years ago

To make this sound, we only need to make one breaking change to the language: Don't delegate private names to noSuchMethod.

I'm not sure if we ever specified whether we mean this globally, or only outside of the defining library. We should nail that down. If you're allowed to do this inside the library, we also need to be sure that there are no nSM interceptors for the private name inside the library.

  • We wouldn't be doing the "level 1" part of Leaf's proposal ("Allow property accesses to final fields (private or not) on private non-escaping classes to promote"). I think this is ok, since the notion of "non-escaping" is non-trivial to define and explain, and if the user wants field promotion on these classes, the workaround is intuitive: just make the field private.

This seems orthogonal to me to the issue we discussed, why would we drop this? Or is there something else I'm missing here? I'm not necessarily opposed to dropping it, but at least it seems worth discussing.

mit-mit commented 2 years ago

will this be shipped in 2.18?

That is unlikely, as we've already branched for that release

stereotype441 commented 2 years ago

To make this sound, we only need to make one breaking change to the language: Don't delegate private names to noSuchMethod.

I'm not sure if we ever specified whether we mean this globally, or only outside of the defining library. We should nail that down. If you're allowed to do this inside the library, we also need to be sure that there are no nSM interceptors for the private name inside the library.

Hmm, good point. I would like us to mean it globally, because that makes for a dramatically simpler rule for what fields are considered promotable. But in my experiments so far, I have been assuming we just mean outside of the defining library, so I don't know how breaking it would be to do it globally, I'd like to measure that in our internal code base before making a decision. Unfortunately, I'm currently hampered in my ability to measure such things internally by https://github.com/dart-lang/sdk/issues/49226, which prevents me from being able to prototype this restriction. Once that issue is fixed, I'll pick up this thread.

stereotype441 commented 2 years ago
  • We wouldn't be doing the "level 1" part of Leaf's proposal ("Allow property accesses to final fields (private or not) on private non-escaping classes to promote"). I think this is ok, since the notion of "non-escaping" is non-trivial to define and explain, and if the user wants field promotion on these classes, the workaround is intuitive: just make the field private.

This seems orthogonal to me to the issue we discussed, why would we drop this? Or is there something else I'm missing here? I'm not necessarily opposed to dropping it, but at least it seems worth discussing.

Sure, let's talk about this in our 1-on-1 meeting on Monday.

eernstg commented 2 years ago

About this design point:

To make this sound, we only need to make one breaking change to the language: Don't delegate private names to noSuchMethod.

We do need to avoid generating noSuchMethod forwarders for private members that are not accessible to the current library, but I think it would be an inconsistent and useless rule to refuse to generate such stubs for private members that are accessible, that is, private members declared in the current library.

After all, a developer could just write a forwarding method explicitly, so it's not like we are protecting ourselves from anything:

class C {
  void _m() {}
}

class FakeC implements C {
  noSuchMethod(Invocation invocation) {
    ... // Whatever.
  }
  void _m() => noSuchMethod(Invocation.method(#_m, []));
}
stereotype441 commented 2 years ago

About this design point:

To make this sound, we only need to make one breaking change to the language: Don't delegate private names to noSuchMethod.

We do need to avoid generating noSuchMethod forwarders for private members that are not accessible to the current library, but I think it would be an inconsistent and useless rule to refuse to generate such stubs for private members that are accessible, that is, private members declared in the current library.

After all, a developer could just write a forwarding method explicitly, so it's not like we are protecting ourselves from anything:

class C {
  void _m() {}
}

class FakeC implements C {
  noSuchMethod(Invocation invocation) {
    ... // Whatever.
  }
  void _m() => noSuchMethod(Invocation.method(#_m, []));
}

That's true. The counterpoint to that, though, is that if we allow a non-throwing implicit noSuchMethod forwarder to be generated for a private member declared in the current library, then in order to preserve soundness, the rule for what fields are promotable must take that noSuchMethod forwarder into account. E.g.:

class C {
  final int? i;  // Seems like it should be promotable, right?
  C(...) : i = ...;
}
// Nope!  Because FakeC overrides `i` with an implicit getter that's not stable.
class FakeC implements C {
  noSuchMethod(Invocation invocation) => randomBool() ? 1 : null;
}

So yes, the advantage of allowing non-throwing implicit noSuchMethod forwarders to be generated for private members is that it makes sense in terms of privacy, but the disadvantage is that those generated forwarders now inhibit field promotion, and that means more potential for user confusion, and more work for us as language implementers.

I'm not really trying to argue one way or another here (like I said earlier, I'd like to measure how breaking it would be in our internal code base before making a decision); I just want to make sure we're clear about the pros and cons.

stereotype441 commented 2 years ago
  • We wouldn't be doing the "level 1" part of Leaf's proposal ("Allow property accesses to final fields (private or not) on private non-escaping classes to promote"). I think this is ok, since the notion of "non-escaping" is non-trivial to define and explain, and if the user wants field promotion on these classes, the workaround is intuitive: just make the field private.

This seems orthogonal to me to the issue we discussed, why would we drop this? Or is there something else I'm missing here? I'm not necessarily opposed to dropping it, but at least it seems worth discussing.

Sure, let's talk about this in our 1-on-1 meeting on Monday.

We decided in today's language meeting to go ahead and drop level 1, at least for the time being. If, at some future point, we decide to add it back in, we probably would do so as part of a larger effort to enable promotion for a wide variety of public fields using some kind of notion of "stable getters".

rubenferreira97 commented 1 year ago

I wanted to check in and see if anyone has an update on this issue. I understand that the development team has been quite busy with Dart 3 lately, and I appreciate your hard work on this major update. However, since this issue has been in the Language Funnel - Being implemented for quite some time now, I'm curious to know when we can expect it to be released.

stereotype441 commented 1 year ago

@rubenferreira97 I'm back to actively working on this. I hope to be able to release it as part of Dart 3.1.

mit-mit commented 1 year ago

@stereotype441 i noted that you landed inference-update-2 -- can you give an update here on what works now, and what remaining work you have planned?

stereotype441 commented 1 year ago

@mit-mit

@stereotype441 i noted that you landed inference-update-2 -- can you give an update here on what works now, and what remaining work you have planned?

Sure! What works is: any "get" of a property can be promoted if all the following conditions hold:

So for example:

class C {
  final int? _i;
  C(this._i);
}
f(C c) {
  if (c._i != null) {
    print(c._i + 1); // OK; `c._i` known to be non-null
  }
}

Bare identifiers that refer to a property of this are also promoted, e.g.:

class C {
  final int? _i;
  C(this._i);
  m() {
    if (_i != null) {
      print(_i + 1); // OK; `_i` known to be non-null
    }
  }
}

But there are a few known issues I'm still working through:

(Edit September 13, 2023: promotion is inhibited by the presence of an implicit noSuchMethod forwarding getter with the same name as the field the user is trying to promote; it is not inhibited by the presence of a noSuchMethod forwarder for a method.)

lrhn commented 1 year ago

Any plans for promoting static final fields in the same library? It's not particularly useful, since those rarely have unknown values. Might be useful for late final static fields.

rubenferreira97 commented 1 year ago

Since we are discussing non-null promotion:

Is this example eligible to promotion? (Sorry if this is not about field promotion, If requested I may open a new issue)

class A {
  A(this.aInt);
  final int aInt;
}

A? _a;

void main() {
  _a = A(1);
  // In this context is it safe to promote _a to A from here?
  _a.aInt; // Error: The property 'aInt' can't be unconditionally accessed because the receiver can be 'null'.
  notNull(_a); //Error: The argument type 'A?' can't be assigned to the parameter type 'A'.
}

void notNull(A a) {}

I don't know if this example was already posted, github indexing is bad :/.

lrhn commented 1 year ago

@rubenferreira97 This is not field promotion as such, and unlikely to be allowed to promote, since the variable is neither local nor final.

mit-mit commented 1 year ago

This is now working in the main channel: https://gist.github.com/mit-mit/16504b68f22f03ca51fb359509275aa5

stereotype441 commented 1 year ago

This is now working in the main channel: https://gist.github.com/mit-mit/16504b68f22f03ca51fb359509275aa5

That's right! Note that to get field promotion you have to set your Dart language version to 3.2, either through a // @dart=3.2 comment at the top of the file, or by using a pubspec SDK constraint of 3.2. If you do that, of course, your Dart code will only work with the (as yet unreleased) 3.2 version of Dart, so if you're maintaining a package published on https://pub.dev/, you'll want to wait 😃.

There's only one task remaining to do before the stable release that has customer impact: the "why not promoted" logic for explaining why a field can't be promoted needs to be updated (https://github.com/dart-lang/sdk/issues/53102). This means that as of today, if you try to promote a field and you can't for some reason (e.g. because it's not a private final field, or because there's a non-promotable field elsewhere in the same library with the same name), the error message you see won't be very helpful. It will just say "'fieldName' refers to a property so it couldn't be promoted. See http://dart.dev/go/non-promo-property", instead of explaining why that particular field couldn't be promoted.

Other than that, everything should work just fine; please let me know (either by replying here or filing a separate issue) if you try it out and run into any difficulties.

Note: in an earlier comment I mentioned two other issues that I hoped to address before calling this feature done:

Unfortunately there was not time to address those two issues before the Beta-2 deadline for Dart 3.2, and since Beta-2 is our normal cutoff for adding new language features, those two issues will have to wait until at least Dart 3.3 to be addressed.

mateusfccp commented 1 year ago

@stereotype441 Has the three levels been implemented (0, 1 and 2)?

stereotype441 commented 1 year ago

@stereotype441 Has the three levels been implemented (0, 1 and 2)?

Not precisely. In July of 2022 we had a discussion about this feature and decided to go with an approach that would be simpler to implement and require fewer breaking changes to the language. You can see the full details here: https://github.com/dart-lang/language/issues/2020#issuecomment-1190546861

The short story is, a field is promotable if it is private and final, and:

For practical purposes, this gives you almost exactly what you would have expected from levels 0 and 2 of the original proposal, but not level 1.

stereotype441 commented 1 year ago

Closing this issue now, since the feature is complete, and the remaining work to do is tracked in other issues.