Skip to content

#1540. Tests error expectations updated #1544

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

Merged
merged 2 commits into from
Nov 14, 2022

Conversation

sgrekhov
Copy link
Contributor

No description provided.

@@ -22,6 +22,7 @@ class C {
static set foo(String s) {
// ^
// [analyzer] unspecified
// [cfe] unspecified
Copy link
Member

Choose a reason for hiding this comment

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

The indentation is inconsistent here.

@eernstg
Copy link
Member

eernstg commented Nov 14, 2022

Hi, @sgrekhov! Could you add just a few words about the nature of these changes? I'm not quite sure what to look for when I'm reviewing.

@sgrekhov
Copy link
Contributor Author

Hi, @sgrekhov! Could you add just a few words about the nature of these changes? I'm not quite sure what to look for when I'm reviewing.

This PR fixes #1540. Change https://dart-review.googlesource.com/c/sdk/+/268946 was landed, CFE chander reporting an errors so I'm changing the tests accordingly

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 do not understand why the code changed from having try/catch to not having it. However, the test was always expecting a compile-time error, so there's no need for a try/catch construct (unless there is some subtle effect associated with control flow analysis, but I can't see how that could be the case in these tests). So it's fine that the try/catch is gone, but it would be nice to hear just a couple of words about this topic. ;-)

The next thing as that, apparently, the CFE used to report an error on call sites for a name-clashed declaration, and that's no longer the case.

So, LGTM!

@eernstg eernstg merged commit cbc9f9c into dart-lang:pre-nnbd Nov 14, 2022
@sgrekhov
Copy link
Contributor Author

These try/catch preserved here since the times of Dart 1.0. AFAIR when Dart was not strongly typed, the code which produced a static errors in the analyser, could be run on VM and produced a runtime error there. So, these try/catch were for VM meaning "runtime error is Ok here"

@eernstg
Copy link
Member

eernstg commented Nov 14, 2022

OK, thanks!

copybara-service bot pushed a commit to dart-lang/sdk that referenced this pull request Dec 1, 2022
2022-12-01 [email protected] Fixes dart-lang/co19#1565. FallThroughError tests removed (dart-lang/co19#1567)
2022-11-14 [email protected] dart-lang/co19#1540. Tests error expectations updated (dart-lang/co19#1544)

Change-Id: I4f4904e3c1e002230f4c47c5236fe0f93b18fc8b
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/273220
Reviewed-by: Erik Ernst <[email protected]>
Commit-Queue: Michael Thomsen <[email protected]>
Reviewed-by: Alexander Thomas <[email protected]>
@sgrekhov sgrekhov deleted the co19_2-1540 branch December 26, 2022 15:25
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.

3 participants