Skip to content

#3057. Fix tests for literals and the type-check #3116

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
Merged
merged 3 commits into from
Mar 31, 2025
Merged

#3057. Fix tests for literals and the type-check #3116

merged 3 commits into from
Mar 31, 2025

Conversation

sgrekhov
Copy link
Contributor

No description provided.

eernstg
eernstg reviewed Mar 26, 2025
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.

I think we'll have to coordinate with Paul about these changes because they depend on changes which are discussed in dart-lang/language#3100, but only accepted by the language team last week, and not yet specified (or, I think, implemented).

@eernstg
Copy link
Member

eernstg commented Mar 26, 2025

@stereotype441, these changes would presumably be valid when the spec and implementation changes discussed in dart-lang/language#3100 have been performed. What's the right ordering/scheduling of these changes and the associated tests?

@sgrekhov sgrekhov added needs-info Additional information needed from the issue author status-blocked Blocked from making progress by another (referenced) issue and removed needs-info Additional information needed from the issue author labels Mar 26, 2025
@stereotype441
Copy link
Member

stereotype441 commented Mar 26, 2025 •
edited
Loading

@eernstg said:

@stereotype441, these changes would presumably be valid when the spec and implementation changes discussed in dart-lang/language#3100 have been performed. What's the right ordering/scheduling of these changes and the associated tests?

I think the changes are significant enough that they should be language-versioned, which means that we should handle them the same way we handle other language versioned features. That means something like:

  • Create an experimental flag for the feature. I can do this later today; if there are no objections, let's call it sound-flow-analysis.
  • Include the line // SharedOptions=--enable-experiment=sound-flow-analysis in any co19 tests that exercise the new behavior.
  • Include the line // @dart=3.7 in any co19 tests that exercise the old behavior (3.7 is the latest stable release version, so it unambiguously selects the old behavior).

I'll be including similar comments in tests that I write in tests/language for the new feature.

Note that the new tests will initially fail, because I haven't implemented the new functionality yet. That's fine. We can ignore the failures for now, and when I get around to implementing the new functionality, I'll search for pre-existing failures to make sure they all start passing.

Edit: I'm hoping to ship the new behavior in Dart 3.9.

@stereotype441
Copy link
Member

I think the changes are significant enough that they should be language-versioned, which means that we should handle them the same way we handle other language versioned features. That means something like:

  • Create an experimental flag for the feature. I can do this later today; if there are no objections, let's call it sound-flow-analysis.

FYI, the new experimental flag has landed: dart-lang/sdk@82420ba

@sgrekhov
Copy link
Contributor Author

Thank you!

@sgrekhov sgrekhov removed the status-blocked Blocked from making progress by another (referenced) issue label Mar 31, 2025
@sgrekhov
Copy link
Contributor Author

Experimental flag and the language version added. Please review.

@sgrekhov sgrekhov requested a review from eernstg March 31, 2025 13:27
eernstg
eernstg reviewed Mar 31, 2025
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 good!

I'm suggesting an adjustment of some comments (because the current wording gives readers too much justification for saying "that's not true!").

@sgrekhov
Copy link
Contributor Author

Updated. PTAL

@sgrekhov sgrekhov requested a review from eernstg March 31, 2025 16:53
eernstg
eernstg approved these changes Mar 31, 2025
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, thanks!

@eernstg eernstg merged commit 46d5763 into dart-lang:master Mar 31, 2025
2 checks passed
copybara-service bot pushed a commit to dart-lang/sdk that referenced this pull request Apr 4, 2025
@sgrekhov
[co19] Roll co19 to 2c7f9a6a379cfc49b3e9019d1f616d7ec9edd766
51616a6 
2025-04-03 [email protected] dart-lang/co19#3057. Add pattern assignment cases to for-in tests (dart-lang/co19#3130)
2025-04-02 [email protected] dart-lang/co19#3057. Add pattern assignment cases to while-loop tests (dart-lang/co19#3127)
2025-04-02 [email protected] dart-lang/co19#3057. Add pattern assignment cases to do-while tests (dart-lang/co19#3124)
2025-04-02 [email protected] dart-lang/co19#3122. Fix typo in type_inference_A07_t01.dart (dart-lang/co19#3125)
2025-04-01 [email protected] dart-lang/co19#3122. Fix errors in dot shorthands tests. (dart-lang/co19#3123)
2025-04-01 [email protected] dart-lang/co19#3057. Add switch statement tests (dart-lang/co19#3121)
2025-03-31 [email protected] dart-lang/co19#3057. Fix tests for literals and the type-check (dart-lang/co19#3116)

Cq-Include-Trybots: luci.dart.try:analyzer-linux-release-try
Change-Id: Ieb1f5fb641976186bb0f5715c64e020c2ba097ec
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/420440
Reviewed-by: Alexander Thomas <[email protected]>
Reviewed-by: Erik Ernst <[email protected]>
Commit-Queue: Alexander Thomas <[email protected]>
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.
3 participants
@sgrekhov @eernstg @stereotype441