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

Current implementation of Analyzer allows noSuchMethod forwarders to be mixed in #33553

Closed
stefantsov opened this issue Jun 21, 2018 · 12 comments

Comments

Projects
None yet
5 participants
@stefantsov
Copy link
Contributor

commented Jun 21, 2018

[Edit eernstg Sep 28 2018: Several issues were discussed in comments below, so the current status may be unclear. Here's the status: dartanalyzer 2.1.0-dev.5.0 has 'No issues!' with the program shown a few lines further down in this text, but a compile-time error must be detected at super.foo().]


As specified in #33380, noSuchMethod forwarders should not be assumed to be provided by mixins. Consider the following code:

class A {
  int foo();

  noSuchMethod(im) => 42;
}

class B extends Object with A {
  noSuchMethod(im) => 87;

  foo() => super.foo();
}

main() {
  print(new B().foo());
}

This code should result in a compile-time error, because B.foo refers to super.foo, but neither of the superclasses of B has member foo. Note that before #33380 the mixin application Object with A was receiving the noSuchMethod forwarder for A.foo as its member and could be reached from B via super.foo.

@stefantsov

This comment has been minimized.

Copy link
Contributor Author

commented Jun 21, 2018

There are two affected tests in tests/language_2:

  • language_2/super_no_such_method4_test;
  • language_2/super_no_such_method5_test.

I've marked them with this GitHub issue number.

@scheglov

This comment has been minimized.

Copy link
Contributor

commented Sep 19, 2018

@lrhn What is "noSuchMethod forwarders"? I don't see this term in the spec.

Why should this be an error? The spec says that S with M, or Object with A in this case, inherits all instance members on the mixin. This includes noSuchMethod, right?

image

So, in this case class B extends Object with A, and has noSuchMethod implementation.

How is this different from the test without mixins?

class A {
  int foo();

  noSuchMethod(im) => 42;
}

class B extends A {
  noSuchMethod(im) => 87;

  foo() => super.foo();
}

main() {
  print(new B().foo());
}

Ah, I see. 16.18.3 Super Invocation, and 16.16 Lookup do not speak about noSuchMethod at all, so I guess its presence or absence does not matter.

The question about difference between the example with mixin, and the example with just classes stands.

image

image

@scheglov scheglov self-assigned this Sep 19, 2018

@scheglov scheglov added the p2-medium label Sep 19, 2018

@kmillikin

This comment has been minimized.

Copy link
Member

commented Sep 20, 2018

No such method forwarders are described by the feature spec https://github.com/dart-lang/sdk/blob/master/docs/language/informal/nosuchmethod-forwarding.md

The spec will be amended (but hasn't been yet) to say that it is a compile-time error to call a super class method unless there is an implementation provided by the super class. This is #33963.

@scheglov

This comment has been minimized.

Copy link
Contributor

commented Sep 24, 2018

I found two tests that look awfully similar, but one (language_2/super_no_such_method1_test) says that it is OK, and another (language_2/super_no_such_method4_test) is a compile-time error.

Why is this? Object with A is a mixin application, and the spec says that it is equivalent to declaring a new synthetic class with members of A as the members of this new synthetic class. So, these two tests should be equivalent, right?

class A {
  int foo();
  noSuchMethod(im) => 42;
}

class B extends A {
  noSuchMethod(im) => 87;

  int foo() => super.foo();
}

main() {
  Expect.equals(87, new B().foo());
}

vs.

class A {
  int foo();

  noSuchMethod(im) => 42;
}

class B extends Object with A {
  noSuchMethod(im) => 87;

  foo() => super.foo(); //# 01: compile-time error
}

main() {
  Expect.equals(87, new B().foo());
}
@scheglov

This comment has been minimized.

Copy link
Contributor

commented Sep 24, 2018

I'm going to assume that language_2/super_no_such_method[1, 2, 3]_test is wrong, and there should be a compile-time error.

https://dart-review.googlesource.com/c/sdk/+/76200

@scheglov scheglov closed this Sep 25, 2018

dart-bot pushed a commit that referenced this issue Sep 25, 2018

Ignore noSuchMethod() for the purpose of searching concrete member im…
…plementations.

R=brianwilkerson@google.com

Bug: #33553
Change-Id: Ie597cb4d39ba16e4503503b59d1e5179c594bad5
Reviewed-on: https://dart-review.googlesource.com/76200
Commit-Queue: Konstantin Shcheglov <scheglov@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
@stefantsov

This comment has been minimized.

Copy link
Contributor Author

commented Sep 25, 2018

I think I have an explanation about how the two test cases in the comment above are different. The idea is that noSuchMethod forwarders are not mixed in, unlike regular methods. It means that in the example 2 super.foo can't be resolved, which results in an error reported at compile time.

@lrhn Please correct me if I'm wrong.

@stefantsov stefantsov reopened this Sep 25, 2018

@scheglov

This comment has been minimized.

Copy link
Contributor

commented Sep 25, 2018

@eernstg @lrhn I'm puzzled. I don't see anything about mixins in the doc about noSuchMethod forwarders. I don't see anything about forwarders in the latest spec. OTOH, the spec is says that if there is no concrete implementation, this is a compile-error. As I can see, the issue referenced above just makes it more obvious that it is a compile-time error, and does not make any exclusions for mixins / noSuchMethod / whatever. Please clarify.

@scheglov scheglov added the needs-info label Sep 25, 2018

@stefantsov

This comment has been minimized.

Copy link
Contributor Author

commented Sep 26, 2018

FWIW, this link may be relevant: #33380.

@eernstg

This comment has been minimized.

Copy link
Member

commented Sep 26, 2018

@scheglov, I get your point.

nosuchmethod-forwarders.md does indeed not address mixins explicitly, but consider mixin application in the light of the language specification section 'Mixin Application':

A mixin application of the form \code{$S$ \WITH{} $M$;} for the name $N$ defines 
a class $C$ with superclass $S$ and name $N$.

A mixin application of the form \code{$S$ \WITH{} $M_1,\ \ldots,\ M_k$;} 
for the name $N$ defines a class $C$ whose superclass is the application of 
the mixin composition (\ref{mixinComposition}) $M_{k-1} * \ldots * M_1$ to $S$ of a
name that is a fresh identifer, and whose name is $N$.
\rationale{The name of the resulting class is necessary because it is part of the 
names of the introduced constructors.}

In both cases above, $C$ declares the same instance members as $M$ (respectively, $M_k$),
and it does not declare any static members.
If any of the instance variables of $M$ (respectively, $M_k$) have initializers,
they are executed in the instance scope of $M$ (respectively, $M_k$)
to initialize the corresponding instance variables of $C$.

This specifies the semantics of mixins in terms of fresh class declarations where the instance members from the class that provided the mixin (or, with the upcoming stand-alone mixin declarations: from the mixin) are declared, as a "copy-down" operation. This issue is concerned with an ambiguous point in that process, namely: Do we copy-down noSuchMethod forwarders as well as the explicitly declared members? Do the forwarders even exist at the time where we perform that copy-down? And the answer that we arrived at, after a bunch of discussions, was "no".

The main issue was probably that a copy-down of noSuchMethod forwarders would allow mixins to capture all their unimplemented methods:

class A {
  String foo() => "I'm the real foo!";
}

class M implements A {
  noSuchMethod(Invocation i) => "Fake!";
  // An instance of `M` will have a forwarder for `String foo()`.
}

class B extends A with M {}

main() => print(B().foo()); // 'Fake!', if we copy-down nSM forwarders.

This is surprising (and not so useful), because the application of M gets to override the existing implementation of foo (A.foo) with a generated forwarder, but that forwarder was generated in M because there was no implementation of foo, as seen from M as a class. That line of reasoning is simply misleading when M is used to extract a mixin, because the answer to "do we have an implementation of foo?" depends on the superclass that it is applied to, for each mixin application.

Next,

it is a compile-time error,

I'm not 100% sure, but 'it' would be this example?:

class A {
  int foo();
  noSuchMethod(im) => 42;
}

class B extends Object with A {
  noSuchMethod(im) => 87;
  foo() => super.foo(); //# 01: compile-time error
}

main() {
  Expect.equals(87, new B().foo());
}

This actually depends on how we answer the question "Is Object with A concrete?"

Assume "no": The superclass of B is Object with A; that class has a non-trivial noSuchMethod but is abstract, so no nSM forwarders are generated and super.foo() is a compile-time error.

Assume "yes": In this case we get a forwarder for int foo() in Object with A, and hence we can use super to invoke it: no error. That forwarder will forward to noSuchMethod in a regular method invocation on this, of course, so we will still get 87, as expected in main. We have said explicitly in nosuchmethod-forwarders.md that it is possible (and of course allowed) to invoke an nSM forwarder in a superinvocation, so super.foo() is OK when we have the forwarder in Object with A.

So the whole thing is controlled by the decision on whether Object with A in the declaration of B is concrete or abstract. It's explicitly specified to be concrete here (and we can make it abstract by adding abstract):

class FreshName = Object with A;

So we would need to decide that a mixin application that occurs in a class declaration declares an abstract class, and then we'd be done and the compile-time error exists in the above example as indicated. I believe that this would simply amount to adding the word abstract in two locations in dartLangSpec.tex, and also that this would be the meaningful thing to do.

@eernstg

This comment has been minimized.

Copy link
Member

commented Sep 26, 2018

Created this CL to make the adjustment where every superclass which is a mixin application is explicitly specified to be an abstract class.

@kmillikin

This comment has been minimized.

Copy link
Member

commented Sep 26, 2018

This matches the implementations. When compiling to Kernel, the superclass representing Object with A is abstract.

@eernstg

This comment has been minimized.

Copy link
Member

commented Sep 26, 2018

Said CL has been landed as e05f1fc, which means that the error indicated as follows here is now clearly the correct behavior:

foo() => super.foo(); //# 01: compile-time error

@scheglov wrote:

So, these two tests should be equivalent, right?

That's actually not quite true, because A is concrete whereas Object with A used as a superclass is abstract (as clarified in the spec just now). That matters because A will have a forwarder for foo, but Object with A doesn't.

@scheglov scheglov removed the needs-info label Oct 1, 2018

@bwilkerson bwilkerson modified the milestones: Dart2.1, Dart2.2 Oct 9, 2018

dart-bot pushed a commit that referenced this issue Nov 6, 2018

Issue 33553. Don't report ABSTRACT_SUPER_MEMBER_REFERENCE if the nomi…
…nal superclass has noSuchMethod().

Bug: #33553
Change-Id: I37ef61ed57d5a479771c7e76a78f7f6fcb99e28b
Reviewed-on: https://dart-review.googlesource.com/c/82947
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Reviewed-by: Paul Berry <paulberry@google.com>
Commit-Queue: Konstantin Shcheglov <scheglov@google.com>

dart-bot pushed a commit that referenced this issue Nov 12, 2018

Issue 33553. For get/set. Don't report ABSTRACT_SUPER_MEMBER_REFERENC…
…E if the nominal superclass has noSuchMethod().

R=brianwilkerson@google.com, paulberry@google.com

Bug: #33553
Change-Id: Ic0e142519a839607560ae0d0e551424a9b32c954
Reviewed-on: https://dart-review.googlesource.com/c/84045
Commit-Queue: Konstantin Shcheglov <scheglov@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>

@scheglov scheglov closed this Nov 12, 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.