-
Notifications
You must be signed in to change notification settings - Fork 214
Final definite assignment #1091
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Very good, actually. ;)
@@ -433,7 +468,7 @@ Definitions: | |||
- Else if `S` is `X extends R` then let `T1` = `X & T` | |||
- Else If `S` is `X & R` then let `T1` = `X & T` | |||
- Else `x` is not promotable (shouldn't happen since we checked above) | |||
- Let `VM2 = VariableModel(declared, T1::promoted, T1::tested, assigned, | |||
- Let `VM2 = VariableModel(declared, T1::promoted, S::tested, assigned, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a deliberate change? It seems wrong--we want to add T1
to the "tested" set, since it's (effectively) the type we're testing against. S
is the old type, so adding it to "tested" doesn't gain us anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, S
was wrong. It's not clear to me that T1
is right though, is it? Or should it be T
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. The current implementation uses T
, so let's change it to that. I'm happy to do that as a separate PR if you want (since at this point it's unrelated to this change), or you can go ahead and do it now--your call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since I backed out the rest of the changes to this doc, how about you add this change to one of your updates?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, will do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed initialization based promotion, restored |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love it. I know we went through a ton of churn to get here, but I feel pretty confident that we landed on the right answer in the end.
Tests out for review here: https://dart-review.googlesource.com/c/sdk/+/157881 |
New tests for the definite assignment related changes specified in dart-lang/language#1091 . Change-Id: I119e36c1bddca97f7021c9015b0f2eee5cb80969 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/157881 Reviewed-by: Erik Ernst <[email protected]> Reviewed-by: Paul Berry <[email protected]>
Tests for the initializer promotion related behavior out for review here: https://dart-review.googlesource.com/c/sdk/+/163663 |
Final variant/proposal for definite assignment based inference.
This variant requires explicit initialization on all branches, but allows adjustment for Null.