Skip to content

Field promotion doesn't work properly for fields declared in mixins #53742

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
stereotype441 opened this issue Oct 12, 2023 · 1 comment
Closed
Assignees
Labels
front-end-expression-compilation legacy-area-front-end Legacy: Use area-dart-model instead. model-features General feature work in the analyzer and CFE.

Comments

@stereotype441
Copy link
Member

The following code is accepted by the analyzer but rejected by the VM:

mixin M {
  final int? _field = 0;
}

class C extends Object with M {
  void test() {
    if (_field != null) {
      _field.isEven;
    }
  }
}

main() {
  C().test();
}

The error message from the VM states:

../../tmp/proj/test.dart:8:14: Error: Property 'isEven' cannot be accessed on 'int?' because it is potentially null.
Try accessing using ?. instead.
      _field.isEven;
             ^^^^^^

The analyzer is correct--the field _field should be promotable.

I'm working on a fix.

@stereotype441 stereotype441 self-assigned this Oct 12, 2023
@johnniwinther johnniwinther added the model-features General feature work in the analyzer and CFE. label Oct 13, 2023
@stereotype441
Copy link
Member Author

A similar problem happens for abstract fields, e.g.:

mixin M {
  abstract final int? _field;
}

abstract class C = Object with M;

test(C c) {
  if (c._field != null) {
      c._field.isEven;
  }
}

main() {}

I'm preparing a CL that addresses both issues.

copybara-service bot pushed a commit that referenced this issue Oct 30, 2023
…ications.

When a field is declared in a mixin, the front end creates a synthetic
getter in the mixin application class that gets the value of the mixed
in field. So if a piece of code accesses the mixed in field through
the mixin application class rather than through the mixin directly,
the resolved member is the synthetic getter rather than a field.

In order to ensure that the field remains promotable even if it is
accessed through the mixin application, the logic in
`OperationsCfe.isPropertyPromotable` needs to be changed so that it
doesn't treat these synthetic getters as non-promotable. The old logic
was essentially this:

1. If the property is not private, it's not promotable.

2. Otherwise, if the property is listed in
   `FieldNonPromotabilityInfo.fieldNameInfo`, it's not
   promotable. (This happens either if the property is not promotable
   for an intrinsic reason, such as being a non-final field or a
   concrete getter, or if it has the same name as a non-promotable
   property elsewhere in the library).

3. Otherwise, if the property is a getter that was lowered from an
   abstract field, it's promotable.

4. Otherwise, if the property is a getter that was lowered from a late
   field, it's promotable.

5. Otherwise, the property isn't promotable. (This was intended to
   cover the case where the property is an abstract getter
   declaration).

(Although conditions 3 and 4 were tested first, since they are more
efficient to test).

It turns out that once conditions 1-2 have been ruled out, the
property must have been declared as a method (which is being torn
off), a private abstract getter, or a (possibly abstract) non-external
private final field. Of these three possibilities, only the last is
promotable. So this can be simplified to:

(conditions 1-2 as above)

3. Otherwise, if the property is a method tear-off, it's not promotable.

4. Otherwise, if the property is an abstract getter, it's not promotable.

5. Otherwise, the property is promotable.

This makes the logic easier to follow, since conditions 1-4 are now
all reasons for non-promotability (rather than a mix of promotability
and non-promotability reasons). It also conveniently addresses the
problem with fields accessed through mixin applications, since they
aren't excluded by any of conditions 1-4.

(We still test conditions 3 and 4 first, since they are more efficient
to test.)

Fixes #53742.
Fixes #53617.
Fixes #53436.

Bug: #53742
Change-Id: If53cb3a42a62c64468cd84fd9586b60b6b10a196
Cherry-pick: https://dart-review.googlesource.com/c/sdk/+/330168
Cherry-pick-request: #53849
Fixes: #53849
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/331750
Reviewed-by: Erik Ernst <[email protected]>
Commit-Queue: Paul Berry <[email protected]>
Reviewed-by: Chloe Stefantsova <[email protected]>
Reviewed-by: Lasse Nielsen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
front-end-expression-compilation legacy-area-front-end Legacy: Use area-dart-model instead. model-features General feature work in the analyzer and CFE.
Projects
None yet
Development

No branches or pull requests

2 participants