[breaking change] Allow private field promotion for abstract getters if there are no conflicting declarations.

[stereotype441]

Change intent

Change the rules for which property gets are eligible for type promotion so that a reference to an abstract getter is considered promotable, provided that there are no other declarations with the same name in the library that would prevent promotability.

For example, this code currently doesn't type promote; with the change it would:

class A {
  int? get _field;
}
class B extends A {
  final int? _field;
  B(this._field);
}
test(A a) {
  if (a._field != null) {
    print(a._field * 2); // Currently an error; will be allowed
  }
}

Justification

The current rules for private field promotion require the implementation to figure out which declaration each property access refers to. If a property access refers to a private final field declaration, and there are no declarations with the same name in the library that aren't safe to promote, then the property access is considered promotable. But if it refers to an abstract getter declaration (as in the above example), then it's considered not promotable. There are two problems with this.

First, and probably most interesting to end users, it unnecessarily restricts type promotion. In the example above, there's no reason a._field wouldn't be safe to promote; we simply decided not to. That seems unfortunate.

Second, and probably more of interest to those of us who maintain the language, the language specification doesn't consider property accesses to refer to declarations directly; they refer to members of interfaces. Those members sometimes arise from getter declarations; sometimes they arise from field declarations; and sometimes they arise through a different process entirely. So the notion of "which declaration a property get refers to" isn't always well defined. For example, consider this code:

abstract class A {
  abstract final int? _field;
}
abstract class B {
  int? get _field;
}
abstract class C implements A, B {}
class D implements C {
  final int? _field;
  D(this._field);
}
test(C c) {
  if (c._field != null) {
    int i = c._field; // OK? ERROR?
  }
}

The reason it's unclear whether c._field should be eligible for type promotion is because it refers to a getter in class C called ._field. But class C doesn't contain a declaration called _field. Instead, this getter arose from merging the interfaces of classes A and B, one of which contains a promotable member called _field, and one of which contains a non-promotable member called _field.

In practice, the implementation makes a choice of whether to allow type promotion, but it does so in an arbitrary and unreliable way. For this example, the code is accepted as written (c._field promotes), but swapping the order of A and B in class C's implements clause (changing it to abstract C implements B, A) leads to a compile error.

With the change, the behavior will be well-defined, and c._field will promote regardless of the order in which C lists its interfaces.

For further discussion see dart-lang/language#3328 (comment).

Expected impact

The primary effect of this change is to make certain property accesses promotable that previously weren't, so for most purposes it should be non-breaking. However, it is technically a breaking change, because changes to type promotion can lead to different types being inferred in situations where the type is implicit, and those different types can have different behaviors. For example the following code would break:

test(C c) {
  if (c._i != null) {
    var x = [c._i]; // Currently inferred as `<int?>[c._i]`; will change to `<int>[c._i]`.
    x.add(null); // Currently ok; will become an error
  }
}

It's also possible that the improved type promotion could lead to some lints such as unnecessary_non_null_assertion, invalid_null_aware_operator, or unnecessary_cast. If the user has a continuous integration setup which treats these lints as errors, then that could lead to a build failure when upgrading to a version of Dart containing the breaking change.

Mitigation

If any code is broken due to different types being inferred, it should be easy to fix by adding explicit types. For example, in the code above, [c._i] could be changed to <int?>[c._i].

If a continuous integration setup is broken due to lints being treated as errors, then there are two possibilities. Either:

• Fix the lint when upgrading the version of Dart
• Add // ignore: comments everywhere that's affected, then upgrade the version of Dart, then fix the lints and remove the // ignore: comments.

[itsjustkevin]

@vsmenon @grouma @Hixie for a breaking change request.

[Hixie]

Can noSuchMethod no longer override privates like this? That was the reason we had previously not implemented this.

[stereotype441]

Can noSuchMethod no longer override privates like this? That was the reason we had previously not implemented this.

Correct. That was a breaking change we made last year (#49687) and in point of fact it was necessary in order for field promotion to be sound at all.

[grouma]

@leonsenft or @mattrberry - does this change impact the ACX injection system at all?

[leonsenft]

@leonsenft or @mattrberry - does this change impact the ACX injection system at all?

I don't think it has any impact.

[vsmenon]

@stereotype441 - is the pattern (at the top of your first comment) common? Just curious.

[Hixie]

LGTM

[stereotype441]

@stereotype441 - is the pattern (at the top of your first comment) common? Just curious.

@vsmenon Sorry for the slow reply. I'm not 100% certain how common this pattern is, but I ran the change experimentally through google3 and found no breakages.