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.

Sign up for GitHub

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

Jump to bottom

#2420. Add more exhaustiveness tests #2513

Merged
merged 4 commits into from
Feb 1, 2024
Merged

#2420. Add more exhaustiveness tests #2513

merged 4 commits into from
Feb 1, 2024

Conversation

sgrekhov
Copy link
Contributor

No description provided.

eernstg
eernstg reviewed Jan 30, 2024
View reviewed changes
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.

Lots of comments on this one. ;-)

LanguageFeatures/Extension-types/exhaustiveness_flow_analysis_A01_t01.dart Outdated Show resolved Hide resolved
LanguageFeatures/Extension-types/exhaustiveness_variable_A04_t01.dart Show resolved Hide resolved
LanguageFeatures/Extension-types/static_analysis_extension_types_A33_t03.dart Outdated Show resolved Hide resolved
LanguageFeatures/Patterns/Exhaustiveness/exhaustiveness_sealed_A03_t01.dart Outdated Show resolved Hide resolved
LanguageFeatures/Patterns/Exhaustiveness/exhaustiveness_sealed_A03_t01.dart Show resolved Hide resolved
sgrekhov
sgrekhov commented Jan 31, 2024
View reviewed changes
Copy link
Contributor Author

@sgrekhov sgrekhov left a comment

Choose a reason for hiding this comment

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

Updated. Please take another look

LanguageFeatures/Extension-types/exhaustiveness_flow_analysis_A01_t01.dart Outdated Show resolved Hide resolved
LanguageFeatures/Extension-types/static_analysis_extension_types_A33_t03.dart Outdated Show resolved Hide resolved
LanguageFeatures/Patterns/Exhaustiveness/exhaustiveness_sealed_A03_t01.dart Show resolved Hide resolved
@sgrekhov sgrekhov requested a review from eernstg January 31, 2024 10:09
eernstg
eernstg reviewed Jan 31, 2024
View reviewed changes
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.

This is still really tricky. The difficulties arise because some tests claim that they are about "flow analysis does not use erasure...", and that's a really complex property to test.

The simple case that I can think of is that some expressions of type ET are erased to int for the purpose of checking control flow based on run-time tests/checks, and we just need to make sure that the same expressions aren't erased when we perform normal type checking.

LanguageFeatures/Patterns/Exhaustiveness/exhaustiveness_sealed_A03_t01.dart Show resolved Hide resolved
LanguageFeatures/Patterns/Exhaustiveness/exhaustiveness_sealed_A03_t01.dart Outdated Show resolved Hide resolved
sgrekhov
sgrekhov commented Jan 31, 2024
View reviewed changes
Copy link
Contributor Author

@sgrekhov sgrekhov left a comment

Choose a reason for hiding this comment

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

Partially updated. Github doesn't show me all of the threads on one page therefore I'll comment here the others as well.

  1. exhaustiveness_flow_analysis_A01_t01.dart updated to check that there is no erasure by calling member of an extension type
  2. Yes, we really have tests that check that [extension types] Switch statement is not exhaustive on sealed class sdk#54444 is fixed, for example https://github.com/dart-lang/co19/tree/master/LanguageFeatures/Extension-types/exhaustiveness_variable_A01_t08.dart. So I need to rewrite exhaustiveness_flow_analysis_A02_t01.dart. But I don't quite understand your suggestion to use a not exhaustive type and not exhaustive switch statement. Could you, please, provide an example?
  3. Added exhaustiveness_sealed_A03_t02.dart containing "not possible nut possible" test

Do I forget anything? Please review and advice with p.2.

LanguageFeatures/Patterns/Exhaustiveness/exhaustiveness_sealed_A03_t01.dart Outdated Show resolved Hide resolved
LanguageFeatures/Extension-types/exhaustiveness_flow_analysis_A01_t01.dart Outdated Show resolved Hide resolved
eernstg
eernstg reviewed Jan 31, 2024
View reviewed changes
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 really good! Just a couple of typos left.

LanguageFeatures/Extension-types/exhaustiveness_flow_analysis_A02_t01.dart Outdated Show resolved Hide resolved
LanguageFeatures/Extension-types/exhaustiveness_flow_analysis_A01_t01.dart Outdated Show resolved Hide resolved
LanguageFeatures/Patterns/Exhaustiveness/exhaustiveness_sealed_A03_t01.dart Outdated Show resolved Hide resolved
@sgrekhov
Copy link
Contributor Author

sgrekhov commented Feb 1, 2024

Typos and formatting fixed. I also deleted LanguageFeatures/Extension-types/exhaustiveness_flow_analysis_A02_t01.dart. https://github.com/dart-lang/co19/tree/master/LanguageFeatures/Extension-types/exhaustiveness_variable_A01_t08.dart. does the same checks. Please review

@sgrekhov sgrekhov requested a review from eernstg February 1, 2024 09:08
eernstg
eernstg approved these changes Feb 1, 2024
View reviewed changes
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 f18ecfa into dart-lang:master Feb 1, 2024
2 checks passed
copybara-service bot pushed a commit to dart-lang/sdk that referenced this pull request Feb 5, 2024
@sgrekhov
[co19] Roll co19 to d1ce99fc64d3766ec7dd82362cf80192262822ec
4cc0743 
2024-02-02 sgrekhov22@gmail.com Fix dart-lang/co19#2517. Fix wrong test, add experimental flags (dart-lang/co19#2518)
2024-02-02 sgrekhov22@gmail.com Fix dart-lang/co19#2490. Update incompatible with await tests according to the changed rule (dart-lang/co19#2516)
2024-02-01 sgrekhov22@gmail.com dart-lang/co19#2420. Add more exhaustiveness tests (dart-lang/co19#2513)
2024-01-31 sgrekhov22@gmail.com dart-lang/co19#2446. Add additional test for cast pattern (dart-lang/co19#2514)
2024-01-30 sgrekhov22@gmail.com dart-lang/co19#1399. Add more tests for records (dart-lang/co19#2511)
2024-01-30 sgrekhov22@gmail.com dart-lang/co19#2485. Update map and list constant literals tests. Add parenthesized for records (dart-lang/co19#2512)
2024-01-29 sgrekhov22@gmail.com Fixes dart-lang/co19#2490. Add more await tests for extension types (dart-lang/co19#2500)
2024-01-29 sgrekhov22@gmail.com dart-lang/co19#2119. Fix typo in Subtyping tests description (dart-lang/co19#2510)
2024-01-29 sgrekhov22@gmail.com Fixes dart-lang/co19#2505. Add more tests for `call` member (dart-lang/co19#2506)
2024-01-29 sgrekhov22@gmail.com dart-lang/co19#2420. Add cast-pattern tests for extension types (dart-lang/co19#2458)

Change-Id: I631705a013f9a77910ae0f0be5a9fea01e7c719f
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/350240
Reviewed-by: Erik Ernst <eernst@google.com>
Commit-Queue: Erik Ernst <eernst@google.com>
Reviewed-by: William Hesse <whesse@google.com>
sgrekhov added a commit to sgrekhov/co19 that referenced this pull request Feb 15, 2024
@sgrekhov
dart-lang#2420. Add more exhaustiveness tests (dart-lang#2513)
53128e5 
Add more exhaustiveness tests
sgrekhov added a commit to sgrekhov/co19 that referenced this pull request Feb 15, 2024
@sgrekhov
dart-lang#2420. Add more exhaustiveness tests (dart-lang#2513)
a2d630d 
Add more exhaustiveness tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eernstg eernstg eernstg approved these changes

@chloestefantsova chloestefantsova Awaiting requested review from chloestefantsova

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@sgrekhov @eernstg