Skip to content

#3057. Add more tests for promotion in switch #3185

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

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, couple of comments!


/// @assertion For now (May, 2025) promotion in switch statement and
/// expressions are not specified yet.
/// TODO (sgrekhov): update when specified
Copy link
Member

Choose a reason for hiding this comment

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

Thanks! Perhaps mention dart-lang/language#3169 for the informal version?

case int _:
print("");
case _:
x = 42;
Copy link
Member

Choose a reason for hiding this comment

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

At this point, x has version x#2 because of the assignment, and we have

Since the version of e is no longer o#1, o is not promoted.

But that would be the type tests implied by pattern matching (because they are still applied to the value of x#1, so we can't allow a pattern match on int _ to say anything about the new value of x).

It should still be the case that int is a type of interest for x, and x should still be promoted via assignment of an expression whose static type is of interest (or it has a unique best supertype that is). So line 20 should promote x to int.

OK!

/// TODO (sgrekhov): update when specified
///
/// @description Checks that pattern match doesn't promote the variable when a
/// case scope is shared abd cases have different types.
Copy link
Member

Choose a reason for hiding this comment

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

Typo:

Suggested change
/// case scope is shared abd cases have different types.
/// case scope is shared and cases have different types.

test1(Object? x) {
switch (x) {
case int v:
case num _:
Copy link
Member

Choose a reason for hiding this comment

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

I'd think there should be two control flow paths to line 19: Matching the pattern in line 17, or matching the pattern in line 18. At the join, the promotion chains should be combined; joinV does this by taking the intersection of the two promotion chains (that is, include exactly the types that occur in both promotion chains, preserving the order), so that would be the empty list here.

OK, so it's not because the cases have different types, it's because these promotion chains happen to have the empty intersection.

So it would be interesting to have a case where the intersection is non-empty (like [Object, num] and [Object, int], or [Object, int] and [num, int]).

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps the logic doesn't actually treat this situation as a full-fledged join, but in that case it would be nice to know it. ;)

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