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] noSuchMethod forwarder to private member not generated #47923

Open
eernstg opened this issue Dec 14, 2021 · 7 comments
Open

[cfe] noSuchMethod forwarder to private member not generated #47923

eernstg opened this issue Dec 14, 2021 · 7 comments
Assignees
Labels
area-front-end Use area-front-end for front end / CFE / kernel format related issues. P2 A bug or feature request we're likely to work on type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)

Comments

@eernstg
Copy link
Member

eernstg commented Dec 14, 2021

[Edit, May 24 2022: Added implements A such that we avoid using an example which will be an error with dart-lang/language#2020. Updated the CFE error message (it is essentially unchanged) and SHA1 accordingly.]

Consider the following program, which is stored in the files n004lib.dart and n004.dart:

// Library n004lib.dart.
class A { void _m(int i) {}}
abstract class B implements A { void _m(num n); }

// Library n004.dart.
import 'n004lib.dart';
class C extends A implements B {}
void main() => C();

This program is rejected by dart from 94aa2ad with the following error messages:

n004.dart:3:7: Error: The implementation of '_m' in the non-abstract class 'C' does not conform to its interface.
class C extends A implements B {}
      ^
n004lib.dart:2:23: Context: The parameter 'i' of the method 'A._m' has type 'int', which does not match the corresponding type, 'num', in the overridden method, 'B._m'.
Change to a supertype of 'num', or, for a covariant parameter, a subtype.
class A { void _m(int i) {}}
                      ^
n004lib.dart:3:38: Context: This is the overridden method ('_m').
abstract class B implements A { void _m(num n); }
                                     ^

The program should actually be accepted (and a noSuchMethod forwarder for _m should be generated).

There is a rule that prevents noSuchMethod forwarders from being generated in the case where the forwarder would override a non-noSuchMethod-forwarder (that is, a manually written method implementation), but that rule is only valid when the overridden manually written declaration is accessible:

It is a compile-time error if the name $m$ is noSuchMethod forwarded
in a concrete class $C$,
and a superclass of $C$ has an accessible concrete declaration of $m$
which is not a noSuchMethod forwarder.

So the change which is needed here is that the error should not be reported, and the generation of a noSuchMethod forwarder should be allowed to proceed.

@eernstg eernstg added type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) area-front-end Use area-front-end for front end / CFE / kernel format related issues. labels Dec 14, 2021
@johnniwinther johnniwinther self-assigned this Dec 14, 2021
@johnniwinther johnniwinther added the P2 A bug or feature request we're likely to work on label Dec 14, 2021
@stereotype441
Copy link
Member

I'd like to push back on this. I see that the spec does indeed say this code should be accepted, and should produce a noSuchMethod forwarder (which in turn forwards to Object.noSuchMethod, which throws an exception). But is that a beneficial behavior for the user? Generally we prefer wrong code to lead to compile time errors rather than runtime failures, and since we're in that situation now, it feels like it would be a step backward to make the change suggested here.

Maybe it would be better to simply codify the current behavior of the CFE in the spec (and update the analyzer to conform to it). Doing so would be 100% non-breaking for users, since the CFE already rejects the code in question.

Tagging some of the language folks who usually discuss such things: @leafpetersen @lrhn @munificent

@leafpetersen
Copy link
Member

The program should actually be accepted (and a noSuchMethod forwarder for _m should be generated).

I don't understand why there is a noSuchMethod forwarder implied here. @eernstg can you elaborate? This just looks like statically wrong code to me, that should be rejected. What am I missing?

@eernstg
Copy link
Member Author

eernstg commented May 17, 2022

That noSuchMethod forwarder is 'forced by privacy':

Forced by privacy: There exists a direct or indirect superinterface D
of C which is declared in a library L2 different from L, the interface of
D contains a member signature S named m, m is a private name, and
no superclass of C has a concrete member named m accessible to L2 that
correctly overrides S. In this case we also say that S is noSuchMethod
forwarded.

In the given example "C" is C, "D" is B, "S" is void _m(num), "m" is _m in n009lib.dart, and it is true that no superclass of C has a concrete member named _m accessible to n009lib.dart that correctly overrides void _m(num).

One could say that there is little difference between inheriting a method that doesn't work, and not inheriting anything at all, so the situation is quite similar to this one, where we're also generating a noSuchMethod forwarder:

// Library not_n009lib.dart.
class A {}
abstract class B { void _m(num n); }

// Library not_n009.dart.
import 'not_n009lib.dart';
class C extends A implements B {}
void main() => C();

The rationale would be something like "There's no way this class can implement _m, so we will implicitly generate a noSuchMethod forwarder". The same wording fits the original example, where C inherits a method with signature void _m(int), but that's not an implementation of void _m(num).

We have recently discussed the approach to noSuchMethod forwarders (when we discussed ways to enable promotion of instance variables in special cases involving privacy), but I don't remember discussions about turning the situation into a compile-time error. Instead, we considered (and I strongly supported ;-) changing the generated member such that it would throw rather than forwarding to noSuchMethod.

@eernstg
Copy link
Member Author

eernstg commented May 17, 2022

/cc @chloestefantsova

@leafpetersen
Copy link
Member

Just to be sure there's no confusion - please make no code changes around this right now. There's active discussion ongoing about what the best way forward is with this and related examples.

@leafpetersen
Copy link
Member

I have added a proposed resolution for the core questions here in the two issues linked above. TL;DR:

  • We introduce a throwing noSuchMethod stub for private members which don't implement their interface
  • We define the unique interface which concrete methods must implement as described here

I believe that resolves the core issue here.

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. P2 A bug or feature request we're likely to work on type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)
Projects
None yet
Development

No branches or pull requests

4 participants