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

#2142. Add Subtyping tests for extension types #2215

Merged
merged 3 commits into from
Aug 18, 2023

Conversation

sgrekhov
Copy link
Contributor

Subtyping tests needs some comments, as usual. It makes sense to review extension_type_* files in the folders:
/Subtyping/test_types/
/Subtyping/static/test_types/
/Subtyping/dynamic/test_types/
All other tests are generated.

Tests in /Subtyping/static/test_types/ test static behaviour. Generated tests will look like

T0 t = t1; // Expect a compile-time error if static type of t1 is not subtype of T0

Tests in /Subtyping/dynamic/test_types/ test runtime behaviour. Generated tests will look like

dynamic forgetType(dynamic d) => d;

T0 t = forgetType(t1); // Expect a run-time error if run-time type of t1 is not subtype of T0

Tests in /Subtyping/test_types/ test both static and runtime behavior. In case of extension types we have to test some cases dynamically or statically only. For example

  extensuin type V(int id) {}
  int i1 = V(42); // Error at compile time
  int i2 = forgetType(V(42)); // No error at run time

So we have to test that extension type V is not assignable to int in static tests only. In dynamic tests we check that forgetType(V(...)) is assignable to int and vice-versa.

Please review

@eernstg
Copy link
Member

eernstg commented Aug 17, 2023

Having looked at the libraries you suggested, I'm not sure about implements relations: Do we have subtype tests confirming that E1 <: E2, E0 <: int, and not E2 <: E1 and not int <: E0 (and similarly for some generic types):

extension type E2(int i) {}
extension type E1(int i) implements E2 {}
extension type E0(int i) implements int {}

Copy link
Member

@eernstg eernstg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! I just have a couple of questions.

@sgrekhov
Copy link
Contributor Author

Added missing cases. Please take another look

@sgrekhov sgrekhov requested a review from eernstg August 18, 2023 09:05
Copy link
Member

@eernstg eernstg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm afraid there's a lot of trailing whitespace. I suppose it can be grepped where it occurs in non-generated files (or is created by the generator)? Otherwise it's good!

@sgrekhov
Copy link
Contributor Author

Updated. Trailing whitespaces removed (yes, it was generator issue and fix affected all tests). But we need to review /test_types/ only. Please note, that one more test type added. /Subtyping/test_types/extension_type_A08.dart

@sgrekhov sgrekhov requested a review from eernstg August 18, 2023 10:52
@eernstg
Copy link
Member

eernstg commented Aug 18, 2023

Please note, that one more test type added. /Subtyping/test_types/extension_type_A08.dart

OK, thanks! It's very helpful to know which parts are actually changed.

However, I'm confused about the changes overall anyway: It used to say (I think) hundred-and-fifty-something files changed; next time it said 253 files changed (I think); but now it says 2840 files changed.

What's going on that changes so many files?

@eernstg
Copy link
Member

eernstg commented Aug 18, 2023

Ah, it's because almost everything had those trailing spaces, and now they have been regenerated without them. OK!

Copy link
Member

@eernstg eernstg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@eernstg eernstg merged commit c44e7e3 into dart-lang:master Aug 18, 2023
copybara-service bot pushed a commit to dart-lang/sdk that referenced this pull request Aug 25, 2023
2023-08-25 sgrekhov22@gmail.com Fixes dart-lang/co19#2236. Expect a secondary error for CFE in syntax_A08_t03.dart (dart-lang/co19#2237)
2023-08-24 sgrekhov22@gmail.com dart-lang/co19#2232. Fix secondary errors in not_a_constant_in_superclass_t02.dart (dart-lang/co19#2233)
2023-08-24 sgrekhov22@gmail.com Fixes dart-lang/co19#2230. Fix roll failures (dart-lang/co19#2231)
2023-08-23 sgrekhov22@gmail.com Fixes dart-lang/co19#2228. Fix syntax error in static_analysis_extension_types_A10_t08.dart (dart-lang/co19#2229)
2023-08-23 sgrekhov22@gmail.com dart-lang/co19#1400. Extension types subtyping tests. Resolve name conflict, add missing tests (dart-lang/co19#2227)
2023-08-23 sgrekhov22@gmail.com dart-lang/co19#1400. Add tests for external members, covariant parameters, nullable supertypes (dart-lang/co19#2225)
2023-08-23 sgrekhov22@gmail.com dart-lang/co19#2217. Add tests for calculating of the least upper bound for extension types (dart-lang/co19#2223)
2023-08-22 sgrekhov22@gmail.com Fixes dart-lang/co19#2224. Remove excessive error expectation for CFE (dart-lang/co19#2226)
2023-08-21 sgrekhov22@gmail.com Fixes dart-lang/co19#2221. Replace inline classes by extension types (dart-lang/co19#2222)
2023-08-21 sgrekhov22@gmail.com Fixes dart-lang/co19#2219. Replace `inline class` by `extension type` in test description (dart-lang/co19#2220)
2023-08-18 sgrekhov22@gmail.com dart-lang/co19#2142. Add Subtyping tests for extension types (dart-lang/co19#2215)
2023-08-18 sgrekhov22@gmail.com Fixes dart-lang/co19#2216. Add test for extension type abstract members (dart-lang/co19#2218)
2023-08-17 sgrekhov22@gmail.com Fixes dart-lang/co19#2213. Fix stack trace comparison (dart-lang/co19#2214)

Change-Id: I009f43878130f934fc6c2eea3368f9763c94e7a5
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/322660
Reviewed-by: Alexander Thomas <athom@google.com>
@sgrekhov sgrekhov deleted the co19-2142-19 branch November 14, 2023 12:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants