Skip to content
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

[cfe] Private field of a mixin application is not promoted #53436

Closed
sgrekhov opened this issue Sep 6, 2023 · 0 comments
Closed

[cfe] Private field of a mixin application is not promoted #53436

sgrekhov opened this issue Sep 6, 2023 · 0 comments
Assignees
Labels
area-front-end Use area-front-end for front end / CFE / kernel format related issues.

Comments

@sgrekhov
Copy link
Contributor

sgrekhov commented Sep 6, 2023

The following test works in analyzer and fails in CFE

mixin Mixin {
  final int? _x = 42;

  void test() {
    if (_x != null) {
      _x.isOdd; // Ok
    }
  }
}

base mixin BaseMixin {
  final int? _x = 42;

  void test() {
    if (_x != null) {
      _x.isOdd; // Ok
    }
  }
}

class WithMixin = Object with Mixin;

base class WithBaseMixin = Object with BaseMixin;

main() {
  WithMixin _withMixin = WithMixin();
  if (_withMixin._x != null) {
    _withMixin._x.isEven; // CFE. Error: Property 'isEven' cannot be accessed on 'int?' because it is potentially null.
  }
  _withMixin.test();

  WithBaseMixin _withBaseMixin = WithBaseMixin();
  if (_withBaseMixin._x != null) {
    _withBaseMixin._x.isEven; // CFE. Error: Property 'isEven' cannot be accessed on 'int?' because it is potentially null.
  }
  _withBaseMixin.test();
}

cc @stereotype441

Tested on the edge SDK (Sept 6, 2023) on Linux x64

@sgrekhov sgrekhov added the area-front-end Use area-front-end for front end / CFE / kernel format related issues. label Sep 6, 2023
@stereotype441 stereotype441 self-assigned this Sep 6, 2023
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 <eernst@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
Reviewed-by: Chloe Stefantsova <cstefantsova@google.com>
Reviewed-by: Lasse Nielsen <lrn@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-front-end Use area-front-end for front end / CFE / kernel format related issues.
Projects
None yet
Development

No branches or pull requests

2 participants