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

[Analyzer] Missing static error on an indirect implementation of a base class. #52160

Closed
leafpetersen opened this issue Apr 24, 2023 · 7 comments
Assignees
Labels
area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. P1 A high priority bug; for example, a single project is unusable or has many test failures
Milestone

Comments

@leafpetersen
Copy link
Member

The static analysis section of the class modifiers spec specifies that the error around implementing a base class from another library extends through the transitive closure of the super-declarations:

    A declaration implements another declaration, and the other
    declaration itself, or any of its super-declarations,
    are marked `base` or `final` and are not from the first declaration's
    library _(with some exceptions for platform libraries)_.

    _(You can only implement an interface if *all* `base` or `final`
    superdeclarations are inside your own library. Or if you're in
    a pre-feature library and all `base` or `final` superdeclarations
    are in platform libraries.)_

    More formally:
    A declaration *C* in library *L* has a declared interface *P*,
    and *P* has any superdeclaration *S*, from a library *K*,
    which is marked `base` or `final` _(including *S* being *P* itself)_,
    and neither:
    * *K* and *L* is the same library, mor
    * *K* is a platform library and *L* is a pre-feature library.

The analyzer does not seem to report this error, the following code produces no error when tested at current HEAD:

// lib.dart
base class C {}

// test.dart
import "lib.dart";
base class Base extends C {}
base class Nefarious implements Base  {}

We seem to currently have no test coverage for this error (and the related errors for final, and for the mixin and mixin class cases).

@munificent is working on adding tests, @dart-lang/language-team please help expedite reviews and landing here.

cc @kallentu @srawlins @bwilkerson @scheglov @itsjustkevin

Tentatively marking this as a blocker for stable, let's plan on a cherry pick if at all possible. If the fix looks like it will be hard to land in that time frame, we can discuss alternatives and possibly aim for the first patch release.

@leafpetersen leafpetersen added the area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. label Apr 24, 2023
@leafpetersen leafpetersen added this to the Dart 3 stable milestone Apr 24, 2023
@sgrekhov
Copy link
Contributor

Duplicate of #52056 ?

@scheglov
Copy link
Contributor

I see that #52056 is assigned to @kallentu, so it looks to me that she is working on fixing it in the analyzer as well.
Correct me if this is not so.

@stereotype441
Copy link
Member

Kallen's on vacation this week so given that this is a blocker for stable, we probably shouldn't wait for her return 😃

@scheglov scheglov self-assigned this Apr 25, 2023
@scheglov scheglov added the P1 A high priority bug; for example, a single project is unusable or has many test failures label Apr 25, 2023
@scheglov
Copy link
Contributor

@leafpetersen
Copy link
Member Author

cc @goderbauer @zanderso as a heads up - this is a missing static error in both CFE and analyzer, there's a chance that we will need to do some fixup in flutter to roll this.

@zanderso
Copy link
Member

cc @jason-simmons the engine sheriff
cc @brianquinlan who I think is the VM gardener today.

copybara-service bot pushed a commit that referenced this issue Apr 25, 2023
…lementing 'base' via 'extends'.

Bug: #52160
Change-Id: I47a72021a002663d04fd8ea14c11a012917e8a8c
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/298320
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Konstantin Shcheglov <scheglov@google.com>
@leafpetersen
Copy link
Member Author

Confirmed that this fixes the pending tests here: https://dart-review.googlesource.com/c/sdk/+/298040 .

copybara-service bot pushed a commit that referenced this issue Apr 26, 2023
…for implementing 'base' via 'extends'.

Bug: #52160
Change-Id: I47a72021a002663d04fd8ea14c11a012917e8a8c
Cherry-pick: https://dart-review.googlesource.com/c/sdk/+/298320
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/298201
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Reviewed-by: Samuel Rawlins <srawlins@google.com>
Commit-Queue: Konstantin Shcheglov <scheglov@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. P1 A high priority bug; for example, a single project is unusable or has many test failures
Projects
None yet
Development

No branches or pull requests

5 participants