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

Precluding methods / setters in extension types #2390

Closed
scheglov opened this issue Nov 21, 2023 · 6 comments
Closed

Precluding methods / setters in extension types #2390

scheglov opened this issue Nov 21, 2023 · 6 comments
Assignees
Labels
bad-test Report tests in need of updates. When closed, the tests should be considered good

Comments

@scheglov
Copy link

https://dart-review.googlesource.com/c/sdk/+/337641 breaks a few co19/ tests.

co19/LanguageFeatures/Extension-types/static_analysis_extension_types_A03_t05
co19/LanguageFeatures/Extension-types/static_analysis_extension_types_A03_t08
co19/LanguageFeatures/Extension-types/static_analysis_extension_types_A30_t02
co19/LanguageFeatures/Extension-types/superinterfaces_of_extension_type_A05_t08

For example

extension type V(int id) {
  external int id2;
}

extension type ET(int id) implements V {
  int id2() => id;
//    ^^^
// [analyzer] unspecified
// [cfe] unspecified
}

Are these tests to be updated?

@sgrekhov sgrekhov self-assigned this Nov 22, 2023
@sgrekhov sgrekhov added the bad-test Report tests in need of updates. When closed, the tests should be considered good label Nov 22, 2023
@sgrekhov
Copy link
Contributor

sgrekhov commented Nov 22, 2023

Are these tests to be updated?

I'm working on this. #2388. Thank you for the list of the failing tests

@sgrekhov
Copy link
Contributor

@scheglov #2392 fixes failing tests except co19/LanguageFeatures/Extension-types/static_analysis_extension_types_A03_t08

// SharedOptions=--enable-experiment=inline-class

extension type I(int id) {
  void set id(int i) {}
}

extension type ET1(int id) {
  void set id(int i) {}
}

extension type ET2(int id) implements I {}

main() {
  ET1 et1 = ET1(1);
  et1.id = 0;

  ET2 et2 = ET2(2);
  et2.id = 0;  // COMPILE_TIME_ERROR.ASSIGNMENT_TO_FINAL 'id' can't be used as a setter because it's final.
}

@eernstg confirmed that this error is wrong. The test should pass according to the new rules

@eernstg
Copy link
Member

eernstg commented Nov 22, 2023

Yes, the intention is that the new rules must make it possible to redeclare a getter as a method (because we generally allow an instance member declaration named n to be redeclared by any kind of instance member declaration named n), and if there is an "inherited" setter that would create a method/setter conflict then we just don't "inherit" it. Similarly for a setter where an otherwise "inherited" method would create a method/setter conflict, we just don't "inherit" the method.

It is still a conflict if two (extension type or non-extension type or a mixture) superinterfaces have a setter and a method with the same basename, and it is not redeclared.

So the new rules are only relevant if the current extension type has a declaration, they don't apply when the conflict occurs exclusively because of declarations in superinterfaces (for that case we just have all the conflicts that we had already). Also, the new rules don't apply unless there is a method declaration somewhere (as long as we're talking about getters and setters there is no conflict about the kinds of declarations, only potentially about the getter return type vs. the setter parameter type).

copybara-service bot pushed a commit to dart-lang/sdk that referenced this issue Nov 24, 2023
2023-11-23 athom@google.com Merge pull request dart-lang/co19#2395 from dart-lang/merge-pre-nnbd
2023-11-23 sgrekhov22@gmail.com Fixes dart-lang/co19#2390. Fix tests according to the new method/setter rules (dart-lang/co19#2392)
2023-11-23 sgrekhov22@gmail.com Fixes dart-lang/co19#2388. Add tests for the new method/setter rules. Update assertions (dart-lang/co19#2393)
2023-11-23 athom@google.com Merge remote-tracking branch 'origin/pre-nnbd' into master
2023-11-22 sgrekhov22@gmail.com Fixes dart-lang/co19#2389. Add additional error expectation for analyzer (dart-lang/co19#2391)
2023-11-21 sgrekhov22@gmail.com dart-lang/co19#2342. Update nullability tests according to the new rules (dart-lang/co19#2385)
2023-11-21 sgrekhov22@gmail.com Fixes dart-lang/co19#2384. Fix not well-bound extension types. Add function-type dynamic test (dart-lang/co19#2387)

Change-Id: Ic3848f6f39fd42b01bfed5feac3d922c6f5a53d5
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/338120
Reviewed-by: Erik Ernst <eernst@google.com>
Reviewed-by: Alexander Thomas <athom@google.com>
@scheglov
Copy link
Author

Ah, of course, only setters and methods.

I'm working on fixing the fix.
Found a failure that I don't understand, co19/LanguageFeatures/Extension-types/static_analysis_extension_types_A30_t02.

extension type I(int id) {
  void set x(String i) {}
}

extension type ET1(int id) {
  void set x(String i) {}
  int get x => 42;
//        ^
// [analyzer] unspecified
// [cfe] unspecified
}

extension type ET2(int id) implements I {
  int get x => 42; // no error, the getter precludes the setter
}

@eernstg Why no error? My current understanding is that only methods preclude setters.

@scheglov scheglov reopened this Nov 27, 2023
sgrekhov added a commit to sgrekhov/co19 that referenced this issue Nov 27, 2023
@sgrekhov
Copy link
Contributor

Looks like a bug in the test. There should be a compile-time error. See #2401

@eernstg
Copy link
Member

eernstg commented Nov 28, 2023

only methods preclude setters.

True, and there's a setter/getter signature conflict. I just landed #2401, so it should be on track now.

copybara-service bot pushed a commit to dart-lang/sdk that referenced this issue Dec 4, 2023
2023-12-01 49699333+dependabot[bot]@users.noreply.github.com Bump actions/setup-java from 3.13.0 to 4.0.0 (dart-lang/co19#2410)
2023-12-01 sgrekhov22@gmail.com dart-lang/co19#2398. Update async tests to avoid false-positive results on web. Language and LanguageFeatures tests (dart-lang/co19#2407)
2023-12-01 sgrekhov22@gmail.com Fixes dart-lang/co19#2408. Fix roll failures (dart-lang/co19#2409)
2023-11-30 sgrekhov22@gmail.com dart-lang/co19#2398. Update asyncStart/End() to correspond SDK version. Replace asyncMultiTest (dart-lang/co19#2406)
2023-11-30 sgrekhov22@gmail.com dart-lang/co19#2398. Remove excessive async. Add explicit `void` (dart-lang/co19#2400)
2023-11-28 sgrekhov22@gmail.com dart-lang/co19#2350. Update existing factory constructor tests. Part 1 (dart-lang/co19#2353)
2023-11-28 sgrekhov22@gmail.com Fixes dart-lang/co19#2390. Add expected error to static_analysis_extension_types_A30_t02.dart (dart-lang/co19#2401)
2023-11-28 sgrekhov22@gmail.com Fixes dart-lang/co19#2399. Update expected errors locations for CFE (dart-lang/co19#2402)
2023-11-24 sgrekhov22@gmail.com dart-lang/co19#2388. Rename and reorder static_analysis_member_invocation_A06_t* tests (dart-lang/co19#2397)

Change-Id: Ie4b51caa12a9a0896c893cc02b099a07ef09fbd7
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/339560
Reviewed-by: Alexander Thomas <athom@google.com>
Reviewed-by: Erik Ernst <eernst@google.com>
Commit-Queue: Erik Ernst <eernst@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bad-test Report tests in need of updates. When closed, the tests should be considered good
Projects
None yet
Development

No branches or pull requests

3 participants