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

#2420. Add extension types exhaustiveness tests. Enums, trivial cases #2423

Merged
merged 2 commits into from
Dec 12, 2023

Conversation

sgrekhov
Copy link
Contributor

No description provided.

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 asked one question about LanguageFeatures/Extension-types/exhaustiveness_enum_A01_t04.dart, perhaps it's just a typo.

String testStatement1(ET1 e) {
switch (e) {
case E.a:
case E.a:
Copy link
Member

Choose a reason for hiding this comment

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

Presumably this would cause an "unreachable case" warning or something like that. I guess that doesn't cause any trouble with any test run? What's the purpose of having two copies of E.a?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was made by purpose. What if this warning will affect exhaustiveness somehow? Let's be sure that not

Copy link
Member

Choose a reason for hiding this comment

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

We could also simply remove the duplicate case: We're testing that a matched value type for the purpose of exhaustiveness is treated as its erasure, we aren't testing that switch statements have correct analysis of 'unreachable case' for a bunch of enum values (because we would have that test already, somewhere else).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed. Please review

@eernstg
Copy link
Member

eernstg commented Dec 12, 2023

This is good, I do think it's useful to have some tests which are aimed very directly at the situation which is the topic of the test, "the matched value type is an extension type", and keeping everything else simple. We need complicated cases, too, but if we have the simple ones and they are working then we know that we may need to look at different features (or look at feature interactions) if something complex isn't working. If the simple thing doesn't work then we definitely need to look at the feature in focus (here: a matched value type is an extension type).

Another thing came to mind as well: Do we have exhaustiveness tests using extension types as object patterns? Such tests could also test the use of extension types as object patterns in general (so they should be able to have field patterns, like case ET(:final someMember?)). So if we don't have it we should probably have two TODOs.

@sgrekhov
Copy link
Contributor Author

Do we have exhaustiveness tests using extension types as object patterns?
Yes we do. I'll add these tests for extension types later in another PR. Right now I'm adding list pattern tests for extension types. Will do a PR soon

@eernstg
Copy link
Member

eernstg commented Dec 12, 2023

PR soon

Sounds good, thanks!

@sgrekhov sgrekhov requested a review from eernstg December 12, 2023 11:22
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 ec13218 into dart-lang:master Dec 12, 2023
copybara-service bot pushed a commit to dart-lang/sdk that referenced this pull request Dec 19, 2023
2023-12-19 [email protected] Fixes dart-lang/co19#2441. Fix roll failures (dart-lang/co19#2443)
2023-12-18 [email protected] Fixes dart-lang/co19#2435. Fix roll failures (dart-lang/co19#2437)
2023-12-18 [email protected] dart-lang/co19#2420. Add extension types exhaustiveness tests. Variables (dart-lang/co19#2431)
2023-12-15 [email protected] Fixes dart-lang/co19#2430. Update expected errors positions for CFE (dart-lang/co19#2433)
2023-12-15 [email protected] Fixes dart-lang/co19#2432. Remove excessive expected error for CFE (dart-lang/co19#2434)
2023-12-14 [email protected] dart-lang/co19#2420. Add extension types exhaustiveness tests. Maps (dart-lang/co19#2426)
2023-12-14 [email protected] dart-lang/co19#2420. Add extension types exhaustiveness tests. Lists (dart-lang/co19#2424)
2023-12-14 [email protected] dart-lang/co19#2139. Fix wrong failure of Language/Functions/element_type_A02_t06 (dart-lang/co19#2429)
2023-12-13 [email protected] dart-lang/co19#2350. Add more factory constructors tests (dart-lang/co19#2427)
2023-12-13 [email protected] Fixes dart-lang/co19#2415. Update `StreamController.broadcast()` test according to the changed documentation (dart-lang/co19#2425)
2023-12-13 [email protected] dart-lang/co19#2350. Add/update factory constructor tests. Part 4 (dart-lang/co19#2367)
2023-12-12 [email protected] dart-lang/co19#2420. Add extension types exhaustiveness tests. Enums, trivial cases (dart-lang/co19#2423)
2023-12-11 [email protected] dart-lang/co19#2420. Add extension types exhaustiveness tests. Enums (dart-lang/co19#2422)
2023-12-11 [email protected] dart-lang/co19#2386. Rename well-bounded tests. Update descriptions (dart-lang/co19#2405)
2023-12-08 [email protected] dart-lang/co19#2415. Change expectations for Stream.asyncMap() according to the current behavior (dart-lang/co19#2421)

Change-Id: I777eba4f1615c8477a5d2044f295696dfc210b1d
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/342582
Commit-Queue: Erik Ernst <[email protected]>
Reviewed-by: Alexander Thomas <[email protected]>
Reviewed-by: Erik Ernst <[email protected]>
@sgrekhov sgrekhov deleted the co19-2420-2 branch January 21, 2025 09:36
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