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

Fasta: Missing override checks for mixins #34235

Closed
stereotype441 opened this issue Aug 22, 2018 · 7 comments

Comments

@stereotype441
Copy link
Member

commented Aug 22, 2018

Consider the following program:

class Base {
  void foo() {
    print('Base.foo called');
  }
}
class M1 {
  void foo({x}) {
    print('M1.foo called, with x=$x');
  }
}
class BaseWithM1 = Base with M1;
class M2 {
  void foo() {
    print('M2.foo called');
  }
}
class Derived extends BaseWithM1 with M2 {}
main() {
  new Derived().foo();
}

I'm not certain what the behavior of this program should be, but I think it should be either:

  • Undefined due to an error (since the mixin application BaseWithM1 with M2 attempts to override foo({x}) with foo(), and overrides may not drop optional parameters.
  • Prints M2.foo called.

If I run this under the VM, it reports no errors and prints M1.foo called, with x=null. That seems definitely wrong. So even though I'm not certain what the correct behavior should be I'm filing this as a VM bug. Feel free to reclassify if I'm wrong about this.

Note that this is a reduction of a situation arose in practice during development of the analyzer/front_end integration. The actual classes/methods involved were:

  • Base -> Generator
  • M1 -> KernelExpressionGenerator
  • BaseWithM1 -> KernelGenerator
  • M2 -> ContextAwareGenerator
  • Derived -> KernelContextAwareGenerator
  • foo -> buildCompoundAssignment

I'll shortly upload a CL that modifies the parameters of buildCompoundAssignment to work around this issue.

Cc @leafpetersen, @eernstg, and @lrhn to weigh in on what the correct behavior should be.

@eernstg

This comment has been minimized.

Copy link
Member

commented Aug 23, 2018

That program should have a compile-time error, so it's the "Undefined" case that applies, for exactly the reason stated.

With the new mixin declaration and the associated new rules, the error will be detected during mixin application because of the incorrect override relationship from M2.foo to M1.foo.

Also, based on the specification of mixin composition (current spec, 12.2 Mixin Composition), application of a mixin to a superclass is 'equivalent to' a particular ordinary class declaration, which would also ensure that this is a compile-time error because of an override that does not satisfy the associated constraints.

[Edit: PS: It may be helpful to consider the sequence of mixins: Derived = [{}, M2, M1, Base, Object], where {} stands for the class body that occurs in the same line as class Derived. With that, it's easier to see that the relevant override relation is "M2.foo overrides M1.foo".]

@kmillikin

This comment has been minimized.

Copy link
Member

commented Aug 23, 2018

Yeah, it's an error that should be signaled by the front end.

@eernstg

This comment has been minimized.

Copy link
Member

commented Aug 23, 2018

Stray thought: I thought that the common front end would eliminate mixins by rewriting them (pretty much like section 12.2 says) into regular class declarations, so why doesn't that override relation get checked just like all other override relations?

@kmillikin

This comment has been minimized.

Copy link
Member

commented Aug 23, 2018

We don't eliminate mixins that way because it's a whole-program transformation and hostile to separate compilation. (It's ML defunctorization.)

What we do do is we name all the mixin application classes, so the output of Fasta doesn't have anonymous mixin applications. We could check the mixin application classes for invalid overrides, but we don't and that's why I say it's a bug in Fasta.

@dhil

This comment has been minimized.

Copy link
Contributor

commented Aug 23, 2018

By looking at the implementation of checkMethodOverride it seems that all the necessary machinery is in place, it just happen that the machinery is disabled for this particular case. The start of the method reads

bool checkMethodOverride(
    ClassHierarchy hierarchy,
    TypeEnvironment typeEnvironment,
    Procedure declaredMember,
    Procedure interfaceMember) {
  if (declaredMember.enclosingClass != cls) {
    // TODO(ahe): Include these checks as well, but the message needs to
    // explain that [declaredMember] is inherited.
    return false;
  }
(the remainder is omitted)
}

I suppose the "these checks" refer to the subsequent ~140 lines of code. Indeed I have confirmed that by removing the conditional statement fasta detects the error (the program bug.dart contains the example above):

$ fasta compile --strong bug.dart
bug.dart:13:8: Error: The method '_Derived&BaseWithM1&M2::foo' has fewer named arguments than those of overridden method 'M1::foo'.
  void foo() {
       ^
bug.dart:7:8: Context: This is the overriden method ('foo').
  void foo({x}) {
       ^

Thus it seems that we can fairly quickly patch this issue. [EDIT: the standard library fails to compile if one simply removes the conditional. Some more thought is required.] The proper solution would be to hoist out the hard-coded assumptions after the conditional statement in the body of checkMethodOverride, and instead parameterise the body such that we can generate specialised/better error messages.

@dhil dhil self-assigned this Aug 23, 2018

dart-bot pushed a commit that referenced this issue Aug 23, 2018
Work around #34235 in front_end.
Change-Id: I7e5d2e63bfd018d21ff9587f178440da39a5078d
Reviewed-on: https://dart-review.googlesource.com/71232
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
@stereotype441

This comment has been minimized.

Copy link
Member Author

commented Aug 23, 2018

@dhil A possible reason why the standard library failed to compile with the conditional removed is that the front_end contained an erroneous override (making the front end itself unbuildable under the proper override rules). I fixed the invalid override in c95ef87, so you may want to try again.

@stereotype441

This comment has been minimized.

Copy link
Member Author

commented Aug 23, 2018

I've uploaded a CL adding a variant of my example code as a test case to language_2: https://dart-review.googlesource.com/c/sdk/+/71368

dart-bot pushed a commit that referenced this issue Aug 24, 2018
Add a language_2 test reproducing #34235.
Change-Id: Icaf6329729a17b20a591ae6e9b8bf7c0d2e21265
Reviewed-on: https://dart-review.googlesource.com/71368
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Reviewed-by: Daniel Hillerström <hillerstrom@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
dart-bot pushed a commit that referenced this issue Aug 27, 2018
Daniel Hillerström commit-bot@chromium.org
Fixes an illegal override in KernelUnresolvedNameGenerator.
The signature of the overridden method buildCompoundAssignment in
KernelUnresolvedNameGenerator (c.f. kernel_expression_generator.dart)
was illegal, because its (named parameter) arity was four, whilst the
same method in one of its supertypes has arity five.

Furthermore, there was a signature conflict for
buildCompoundAssignment amongst the supertypes of
KernelUnresolvedNameGenerator:

   KernelUnresolvedNameGenerator
   extends KernelGenerator
           |
           = Generator with KernelExpressionGenerator
                            |
                            arity(buildCompoundAssignment) = 5
   with    ErroneousExpressionGenerator, UnresolvedNameGenerator.
           |
           arity(buildCompoundAssignment) = 4

In this CL the conflict is resolved by the "greatest common
denominator" approach, that is, now each implementation of
buildCompoundAssignment has arity five.

This CL is a necessary step towards fixing
#34235

Related change: c95ef87

Change-Id: If1992b8fb2bb994f83557c48fb0ac276b2d1a8d3
Reviewed-on: https://dart-review.googlesource.com/71520
Reviewed-by: Kevin Millikin <kmillikin@google.com>
Commit-Queue: Daniel Hillerström <hillerstrom@google.com>

@dart-bot dart-bot closed this in 03c7184 Aug 30, 2018

dart-bot pushed a commit that referenced this issue Aug 30, 2018
Daniel Hillerström commit-bot@chromium.org
Revert "Enables arity check for overridden methods inherited from a m…
…ixin."

This reverts commit 03c7184.

Reason for revert: Flutter doesn't build with after this CL because the following (kind of) program

  abstract class RenderObject {
  String toString({int minLevel}) => "foo";
  }
  abstract class ContainerRenderObjectMixin extends RenderObject {}
  abstract class RenderBoxContainerDefaultsMixin implements 
  ContainerRenderObjectMixin {}

is wrongfully rejected. In patching this, I stumbled upon what seems to be bug in Fasta where some concrete classes arising from mixed in applications are marked as abstract, which causes the patch to produce false-positives. This needs some more investigation.

Original change's description:
> Enables arity check for overridden methods inherited from a mixin.
> 
> The return type and parameter types checks are still disabled for
> mixins as there is an issue with type parameters being uninstantiated
> in mixin applications which causes the two checks to produce
> false-positives. As a result the SDK fails to compile with the two
> checks enabled because the libraries (such as collections) makes
> extensive use of mixin application with generic types. I've opened a
> separate ticket to track this issue [c.f. #34285].
> 
> Furthermore this CL abstracts the arity check, return type check, and
> method parameter type check in checkMethodOverride to make it easier
> to understand the logic of the method.
> 
> Closes #34235 and closes #32014
> 
> Change-Id: Iae224926c2e99e6e89ccc3c19ec4bc7919ee48a5
> Reviewed-on: https://dart-review.googlesource.com/71781
> Commit-Queue: Daniel Hillerström <hillerstrom@google.com>
> Reviewed-by: Aske Simon Christensen <askesc@google.com>

TBR=askesc@google.com,hillerstrom@google.com

Change-Id: Idee6f53c2d6a17ea8a4103872b1183c01e4d30ce
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://dart-review.googlesource.com/72108
Reviewed-by: Daniel Hillerström <hillerstrom@google.com>
Commit-Queue: Daniel Hillerström <hillerstrom@google.com>

@dhil dhil reopened this Aug 30, 2018

@dhil dhil changed the title VM dispatches to wrong method when mixin implies a dodgy override Fasta: Missing override checks for mixins Sep 19, 2018

@askeksa-google askeksa-google referenced this issue Sep 28, 2018
7 of 7 tasks complete

@dart-bot dart-bot closed this in bb2775b Oct 3, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.