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: Implement missing override checks #32613

Closed
alexmarkov opened this issue Mar 20, 2018 · 11 comments
Closed

CFE: Implement missing override checks #32613

alexmarkov opened this issue Mar 20, 2018 · 11 comments
Assignees
Labels
area-front-end Use area-front-end for front end / CFE / kernel format related issues. customer-vm front-end-missing-error P2 A bug or feature request we're likely to work on
Milestone

Comments

@alexmarkov
Copy link
Contributor

CFE doesn't fully implement all checks related to overriding/inheriting members yet.
This includes reporting the following errors:

  • When method is "overridden" with getter/field and vice versa.
  • When static member conflicts with inherited instance member with the same name.

Currently, some of these errors are reported by VM. However, we would like to add tree shaking kernel transformation which removes conflicting members and VM no longer has a chance to report these errors. This issue will be used to silence failing tests for override checks in status files.

/cc @kmillikin @stefantsov @mraleph

@alexmarkov alexmarkov added area-front-end Use area-front-end for front end / CFE / kernel format related issues. customer-vm labels Mar 20, 2018
dart-bot pushed a commit that referenced this issue May 8, 2018
Detect the following conflicts:

  - declaring a method and inheriting a setter or getter with the same
    name

  - declaring a getter or setter and inheriting a method with the same
    name

  - inheriting both a method and a getter or setter (or both) with the
    same name

Declaring a method and a getter with the same name is already
detected.  Declaring a method and a setter with the same name is not
yet detected, because it should be done in the same way as
method/getter declaration conflicts.

Bug: #32613
Change-Id: I2d168894453d7032372388faa0872d3fc7aa9ef7
Reviewed-on: https://dart-review.googlesource.com/53803
Commit-Queue: Kevin Millikin <kmillikin@google.com>
Reviewed-by: Dmitry Stefantsov <dmitryas@google.com>
Reviewed-by: Aske Simon Christensen <askesc@google.com>
@kmillikin
Copy link

As of 8f6c6c2 it is now an error to declare an instance method and inherit an instance getter or setter with the same name, or to declare an instance getter or setter and inherit an instance method with the same name, or to inherit both an instance method and an instance getter or setter with the same name.

We don't catch the cases of static members conflicting with inherited instance members. While we do catch duplicated declarations of methods and getters in the same class, we do not catch duplicated declarations of a method and a setter with the same name (presumably we should, which is related to issue #33077).

Note that there's an unfortunate interaction where we will signal an error for any conflicting inherited members, and also signal an error that they are not both implemented if the class is not abstract. Since you can't actually fix the second problem by implementing both of them, we should find a way to suppress that error.

@kmillikin kmillikin added the P2 A bug or feature request we're likely to work on label May 9, 2018
@kmillikin kmillikin added this to the Dart2Stable milestone May 9, 2018
@eernstg
Copy link
Member

eernstg commented May 28, 2018

Please note that the specification of member declaration conflict rules are being updated, cf. #33235. I believe that the behavior requested here matches the updated specification, but it may be useful to consult the table here in order to clarify corner cases.

@mit-mit
Copy link
Member

mit-mit commented Jul 9, 2018

@lrhn @leafpetersen @eernstg can you help us understand how critical it is to keep this in 2.0 stable?

@leafpetersen
Copy link
Member

I'm not 100% clear where the implementation is on this. It sounds like most (but not all) of this is implemented in the CFE? Probably relatively rare that this breaks something, but it will be a breaking change to fix. I'm ok with leaving it as an open bug against Dart 2, but we will need to fix it at some point.

@jensjoha jensjoha self-assigned this Jul 10, 2018
@jensjoha
Copy link
Contributor

From the tables here: #33235 (comment)
It seems that

has instance setter `n=`      has instance method `n`       2450 or 2451

is not handled, i.e. it should give an error, but doesn't.

and that

declares constructor `C.n`    has instance method `n`
declares constructor `C.n`    has instance getter `n`

should be allowed but isn't.

@jensjoha
Copy link
Contributor

Oh, and there's also some tests tagged with 32613 that fails...

@dgrove
Copy link
Contributor

dgrove commented Jul 12, 2018

Any updates on this, @jensjoha ?

@jensjoha
Copy link
Contributor

jensjoha commented Jul 12, 2018

https://dart-review.googlesource.com/c/sdk/+/64500 should make it comply with the tables at #33235 (comment)
https://dart-review.googlesource.com/c/sdk/+/64542 should fix tests tagged with 32613

(neither have currently had the actual code reviewed)

@jensjoha
Copy link
Contributor

dart-bot pushed a commit that referenced this issue Jul 16, 2018
See #32613
and #33235 (comment)

Bug: #32613, #33235, #33237
Change-Id: I0d1432185b6811137e31135ac2c7f58c4de2de6c
Reviewed-on: https://dart-review.googlesource.com/64500
Commit-Queue: Dmitry Stefantsov <dmitryas@google.com>
Reviewed-by: Dmitry Stefantsov <dmitryas@google.com>
Reviewed-by: Erik Ernst <eernst@google.com>
dart-bot pushed a commit that referenced this issue Jul 16, 2018
Bug: #32613

Change-Id: Id69e793afd2c933294299d796a28b66db6b5de17
Reviewed-on: https://dart-review.googlesource.com/64840
Commit-Queue: Dmitry Stefantsov <dmitryas@google.com>
Reviewed-by: Erik Ernst <eernst@google.com>
Reviewed-by: Dmitry Stefantsov <dmitryas@google.com>
Reviewed-by: Peter von der Ahé <ahe@google.com>
@dgrove
Copy link
Contributor

dgrove commented Jul 16, 2018

@stefantsov Can this be closed?

@chloestefantsova
Copy link
Contributor

I believe so.

dart-bot pushed a commit that referenced this issue Jul 18, 2018
Change-Id: Idd473d5105237bcb57c2c051e3a1903c041ec323
Reviewed-on: https://dart-review.googlesource.com/65323
Commit-Queue: Dmitry Stefantsov <dmitryas@google.com>
Reviewed-by: Peter von der Ahé <ahe@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. customer-vm front-end-missing-error P2 A bug or feature request we're likely to work on
Projects
None yet
Development

No branches or pull requests

9 participants