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

No error in CFE when mixin invokes an abstract super method #47406

Open
sgrekhov opened this issue Oct 7, 2021 · 5 comments
Open

No error in CFE when mixin invokes an abstract super method #47406

sgrekhov opened this issue Oct 7, 2021 · 5 comments
Labels
area-front-end Use area-front-end for front end / CFE / kernel format related issues. front-end-missing-error

Comments

@sgrekhov
Copy link
Contributor

sgrekhov commented Oct 7, 2021

The following code produces an error in Analyzer and no issues in CFE.

abstract class A {
  int get index;
}

mixin M on A {
  int get index => super.index + 1;
}

class AM extends A with M { // Analyzer error here. The class doesn't have a concrete implementation of the super-invoked member 'index'. - mixin_application_no_concrete_super_invoked_member 
  int get index => 42;
}

main() {
  AM am = new AM();
  print(am.index);  // prints 42 in VM
}

cc @eernstg to confirm that analyzer's behavior is right here

Tested on Dart SDK version: 2.15.0-168.0.dev (dev) (Thu Sep 30 12:23:13 2021 -0700) on "windows_x64"

@sgrekhov sgrekhov added area-front-end Use area-front-end for front end / CFE / kernel format related issues. front-end-analyzer-unification Issues where the analyzer and cfe vary (not clearly a bug on one or the other). and removed front-end-analyzer-unification Issues where the analyzer and cfe vary (not clearly a bug on one or the other). labels Oct 7, 2021
@eernstg
Copy link
Member

eernstg commented Oct 7, 2021

Yes, we have this:

It is a compile-time error to apply $M$ to $S$ if $S$ does not implement,
directly or indirectly, all of $T_1$, \ldots, $T_n$.
It is a compile-time error if any of \metavar{members} contains a
super-invocation of a member $m$ \commentary{(for example \code{super.foo},
\code{super + 2}, or \code{super[1] = 2})}, and $S$ does not have a concrete
implementation of $m$ which is a valid override of the member $m$ in
the interface $M_S$.

This means that the existence of the super-method implementation should be checked for each mixin application. The invocation is not checked (that's done once and for all, based on the interface of the enclosing mixin/class of the mixed-in method), and this means that there can be a dynamic error (if the inherited method has a parameter which is covariant-by-declaration).

@eernstg eernstg added type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) front-end-missing-error and removed type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) labels Oct 7, 2021
@johnniwinther
Copy link
Member

This is a known missing check on the CFE part.

The problem with this check is that there is no natural point at which to perform it. The VM and dart2js (in its current state) could perform it as part of the their mixin transformations. DDC, however, doesn't have this point in time, because it compiles modularly using outlines, which don't contain method bodies, so there is no way to know which supercalls to check.

@eernstg
Copy link
Member

eernstg commented Oct 7, 2021

Sounds like the outline of a mixin would need to include the names of members that are invoked in a super-invocation..

@johnniwinther
Copy link
Member

johnniwinther commented Oct 7, 2021

Yes, but the purpose of the outline it to avoid parsing the body for speedy processing. - Can't have your cake and eat it too.

@sigmundch
Copy link
Member

Yes, but the purpose of the outline it to avoid parsing the body for speedy processing. - Can't have your cake and eat it too.

Given that super calls are already using the reserved keyword super, would it be possible to use a naive/diet traversal of the token stream to extract the member names? For example, extracting the identifier in ... super.<identifier> ... regardless of context?

copybara-service bot pushed a commit that referenced this issue Jun 24, 2022
In response to #47406

The error is currently not reported if the mixin declaration is from
an outline dill.

Change-Id: I94a61d6409d0c238614d9f377b5f324153360bc6
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/249184
Reviewed-by: Sigmund Cherem <sigmund@google.com>
Reviewed-by: Jens Johansen <jensj@google.com>
Commit-Queue: Johnni Winther <johnniwinther@google.com>
copybara-service bot pushed a commit that referenced this issue Feb 20, 2024
Issue: #47406
Issue: #48820
Change-Id: I19de399c4670f9866cffceae3bc3ce19201d1ed3
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/352963
Reviewed-by: Johnni Winther <johnniwinther@google.com>
Commit-Queue: Mayank Patke <fishythefish@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. front-end-missing-error
Projects
None yet
Development

No branches or pull requests

4 participants