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] No error produced for not-implementing-final-classes through legacy libraries. #52078

Closed
lrhn opened this issue Apr 18, 2023 · 7 comments
Assignees
Labels
analyzer-spec Issues with the analyzer's implementation of the language spec area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. 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

@lrhn
Copy link
Member

lrhn commented Apr 18, 2023

Example:

// old_lib.dart
// @dart=2.19

// MapEntry is `final`, but can be implemented due to SDK exception.
class M implements MapEntry<int, int> {
  int get key => 0;
  int get value => 1;
  String toString() => "Bad";
}

and

// new_lib.dart
// @dart=3.0

import "old_lib.dart";

class N implements M {
  int get key => 0;
  int get value => 1;
  String toString() => "Evil";
}

void main() {
  print(N()); // Evil.
}

This gives no errors, or warnings, in the front end or analyzer.

According to specification, you are not allowed to implement any interface which has any superinterface whose declaration is marked base or final. (Unless you are, yourself, a pre-feature library and all such superinterfaces are in platform libraries.)

Here N is not in a pre-feature library, so it gets no exemptions.
M has a superinterface marked final, so it must not be implemented by N.

(We don't have a rule against N extending M, so its not like we can prevent everything, but we can prevent new implements of a final platform type in 3.0-libraries.)

Filing as area-fe-analyzer-shared. Split this into two issues if it's two different bugs.

@lrhn lrhn added type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) area-fe-analyzer-shared Assigned by engineers; when triaging, prefer either area-front-end or area-analyzer. labels Apr 18, 2023
@lrhn lrhn changed the title Front-end/analyzer does not enforce not-implementing-final-classes through legacy libraries. Front-end/analyzer do not enforce not-implementing-final-classes through legacy libraries. Apr 18, 2023
@lrhn
Copy link
Member Author

lrhn commented Apr 20, 2023

@kallentu

@kallentu
Copy link
Member

kallentu commented Apr 20, 2023

It's not shared code so I'm gonna split this bug.

And this is in the same area as the base implements bug, will look into it

@kallentu kallentu changed the title Front-end/analyzer do not enforce not-implementing-final-classes through legacy libraries. [analyzer] No error produced for not-implementing-final-classes through legacy libraries. Apr 20, 2023
@kallentu kallentu added area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. and removed area-fe-analyzer-shared Assigned by engineers; when triaging, prefer either area-front-end or area-analyzer. labels Apr 20, 2023
@srawlins srawlins added P2 A bug or feature request we're likely to work on analyzer-spec Issues with the analyzer's implementation of the language spec labels Apr 24, 2023
@leafpetersen
Copy link
Member

I think we're probably just going to accept this behavior here. It would be breaking to change this post release, and I don't see it rising to cherry pick level at this point. I'm also not 100% convinced that it's what we want. The problem here is in the legacy library. The Dart 3 code has no ability to control or fix the problem. Eventually, the legacy library will have to do something other than implement MapEntry, and at that point, the Dart 3 code will be correct. Until then, it seems better to just let the code work.

@eernstg
Copy link
Member

eernstg commented May 4, 2023

@leafpetersen, if I'm reading the timezone info correct then your comment here was written before the language team meeting yesterday, and I believe the decision at that meeting was to adjust the tool behavior to fit the current specification (so class N in this comment will be an error).

We do have co19 tests covering this case, but they are only being rolled in right now. For example, https://github.com/dart-lang/co19/blob/master/LanguageFeatures/Class-modifiers/versioning_A03_t02.dart has a few dozen post-feature classes that implement a pre-feature class that implements or extends or mixes in a final platform class.

copybara-service bot pushed a commit that referenced this issue May 4, 2023
… implements a core library base or final class.

Bug: #52078
Change-Id: If97895b9560475f11e8236de87ad5880bdee97c3
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/300880
Reviewed-by: Lasse Nielsen <[email protected]>
Commit-Queue: Kallen Tu <[email protected]>
copybara-service bot pushed a commit that referenced this issue May 8, 2023
… class.

Checks an interface's supertypes for final classes and produces an error if the implementing bypasses a legacy library.

This behaviour should only happen when a post-feature library implements
a pre-feature library declaration that has a final class as a super
declaration.

Similar error to https://dart-review.googlesource.com/c/sdk/+/298320

Bug: #52078
Change-Id: Ie16edb2b231957dad7502fdab3d5faba93bc6773
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/300861
Commit-Queue: Kallen Tu <[email protected]>
Reviewed-by: Konstantin Shcheglov <[email protected]>
@kallentu
Copy link
Member

kallentu commented May 8, 2023

@sgrekhov Just to let you know, the analyzer errors in https://github.com/dart-lang/co19/blob/master/LanguageFeatures/Class-modifiers/versioning_A03_t01.dart should now be added.

Analyzer (not CFE yet) work for this bug is complete.

@kallentu kallentu closed this as completed May 8, 2023
@sgrekhov
Copy link
Contributor

sgrekhov commented May 9, 2023

@kallentu thank you! I checked the tests on the just built SDK on Linux x64. We had 3 failing tests for the case when superinterface chain goes through a pre-feature library

versioning_A03_t01.dart - checks that final class cannot be extended
versioning_A03_t02.dart - checks that final class cannot be implemented
versioning_A03_t04.dart - checks that mixin on a final class cannot be declared

Testing shows me that versioning_A03_t02.dart is fixed and passes now. But other two tests are still failing. I think they should be fixed as well

$ python3 tools/test.py -n analyzer-asserts-linux co19/LanguageFeatures/Class-modifiers/versioning_A03*
No build targets found.
Test configuration:
    analyzer-asserts-linux(architecture: x64, compiler: dart2analyzer, mode: release, runtime: none, system: linux, enable-asserts, use-sdk)
Suites tested: co19
[00:04 |  25% | +    1 | -    0]

FAILED: dart2analyzer-none release_x64 co19/LanguageFeatures/Class-modifiers/versioning_A03_t01
Expected: Pass
Actual: MissingCompileTimeError

--- Command "dart2analyzer" (took 04.000019s):
DART_CONFIGURATION=ReleaseX64 out/ReleaseX64/dart-sdk/bin/dart out/ReleaseX64/gen/dartanalyzer.dart.snapshot -Dtest_runner.configuration=analyzer-asserts-linux --ignore-unrecognized-flags --packages=/home/sgrekhov/Google/dart-sdk/sdk/.dart_tool/package_config.json --format=json /home/sgrekhov/Google/dart-sdk/sdk/tests/co19/src/LanguageFeatures/Class-modifiers/versioning_A03_t01.dart

static error failures:
- Missing expected unspecified error at line 20, column 7, length 34.

- Missing expected unspecified error at line 25, column 7, length 37.

- Missing expected unspecified error at line 30, column 7, length 31.

- Missing expected unspecified error at line 35, column 17, length 43.

- Missing expected unspecified error at line 40, column 17, length 46.

- Missing expected unspecified error at line 45, column 17, length 40.

- Missing expected unspecified error at line 50, column 16, length 42.

- Missing expected unspecified error at line 55, column 16, length 45.

- Missing expected unspecified error at line 60, column 16, length 39.

- Missing expected unspecified error at line 65, column 26, length 51.

- Missing expected unspecified error at line 70, column 26, length 54.

- Missing expected unspecified error at line 75, column 26, length 48.

--- Re-run this test:
python3 tools/test.py -n analyzer-asserts-linux co19/LanguageFeatures/Class-modifiers/versioning_A03_t01
[00:04 |  75% | +    2 | -    1]

FAILED: dart2analyzer-none release_x64 co19/LanguageFeatures/Class-modifiers/versioning_A03_t04
Expected: Pass
Actual: MissingCompileTimeError

--- Command "dart2analyzer" (took 04.000061s):
DART_CONFIGURATION=ReleaseX64 out/ReleaseX64/dart-sdk/bin/dart out/ReleaseX64/gen/dartanalyzer.dart.snapshot -Dtest_runner.configuration=analyzer-asserts-linux --ignore-unrecognized-flags --packages=/home/sgrekhov/Google/dart-sdk/sdk/.dart_tool/package_config.json --format=json /home/sgrekhov/Google/dart-sdk/sdk/tests/co19/src/LanguageFeatures/Class-modifiers/versioning_A03_t04.dart

static error failures:
- Missing expected unspecified error at line 20, column 7, length 29.

- Missing expected unspecified error at line 27, column 7, length 32.

- Missing expected unspecified error at line 34, column 7, length 26.

--- Re-run this test:
python3 tools/test.py -n analyzer-asserts-linux co19/LanguageFeatures/Class-modifiers/versioning_A03_t04
[00:04 | 100% | +    2 | -    2]

=== 2 tests passed, 2 failed ===

@eernstg
Copy link
Member

eernstg commented May 9, 2023

Sounds like we would need to reopen this issue, but I suspect there may be an issue which is a better fit (or a new one should be created, rather than reopening this one):

This issue is about having an implements superinterface edge in new code which causes a compile-time error (because there is a final class in the superinterface graph, and this is still an error even in the case where all paths in the superinterface graph go through a legacy class before they reach that final class).

The failures in LanguageFeatures/Class-modifiers/versioning_A03_t01.dart and ..._t04.dart are caused by the fact that the CFE and the analyzer do not report a compile-time error based on the missing base/final/sealed modifier. That error occurs for extends and on edges in the superinterface graph as well as implements edges. Presumably the failure only arises because the relevant edges in the superinterface graph go through a legacy library.

Is there an open issue about this situation? Otherwise we should probably handle it in a new issue.

Edit: We now have #52321, covering the cases mentioned here.

copybara-service bot pushed a commit that referenced this issue May 22, 2023
…f a core lib declaration through a 2.19 library and a 3.0 library.

Cherry pick for the following behaviour from 3 different CLs.

Error produced when a post-feature library implements
a pre-feature library declaration that has a final/base core library class as a super declaration.

Error produced when a post-feature library subtypes (but not implements)
a pre-feature library declaration that has a final/base core library class as a super declaration.

Fixes: #52406

Bug: #52315, #52115, #52078
Cherry-pick: https://dart-review.googlesource.com/c/sdk/+/302365
Cherry-pick: https://dart-review.googlesource.com/c/sdk/+/301503
Cherry-pick: https://dart-review.googlesource.com/c/sdk/+/300861
Cherry-pick: https://dart-review.googlesource.com/c/sdk/+/301442
Cherry-pick: https://dart-review.googlesource.com/c/sdk/+/300880
Change-Id: I2a192a157d417c1de7656aa624d3b10d9e67626e
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/302851
Commit-Queue: Kallen Tu <[email protected]>
Reviewed-by: Konstantin Shcheglov <[email protected]>
Reviewed-by: Johnni Winther <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
analyzer-spec Issues with the analyzer's implementation of the language spec area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. 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

6 participants