-
Notifications
You must be signed in to change notification settings - Fork 198
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
Using if (foo?.bar == somethingNotNull)
should promote foo
#1224
Comments
The only value comparisons which promote are Here you are deriving that because So, working as intended. |
Why isn't that expected to happen? The above is a contrived case, but patterns like this are common enough: if (foo?.isDelicious() == true) {
print(foo.describeDeliciousness());
} |
I find it slightly surprising that this does not work. Promotion of a local would happen if you manually desugared if (foo?.something != null) ... to if (foo != null && foo.something != null) ... so why should you have to do that yourself? Not promoting With |
I'm not surprised we don't promote because I have memorized which operations can promote, and this isn't one of them. If we consider Generally, I really, really don't want to reason about code from syntactic equivalences. It gives surprising behavior (like We also don't promote https://nullsafety.dartpad.dev/5a783a27daed945ead642728a80229c6 |
Respectfully whether you personally want to reason about it isn't really the point of this issue - at least me and Stephen (likely more) consider this valid. I suspect this pattern is very common across both external and Google packages and unless we intentionally want users to hand desugar to the longer-form code we should consider improving this. I'm happy to wait for other responses, thanks! |
My intuition is that this feature would make users less confused than they would be without it. I'd be inclined to push on it, especially if we think we can do it relatively quickly. If we think it will take too long we should consider whether it will be breaking to release it later. @leafpetersen |
Totally! I could also see at minimum adding some information to our migration guides (/cc @munificent) that highlight "these are cases you'd think would promote, but don't, here are some tracking issues for future versions of Dart". Otherwise I think users are going to think they did something wrong. |
Maybe I'm just not sure what the point of this issue is. The described behavior is correct according to the specification, so all tools are working as intended and required. That makes it neither a bug, nor a tool enhancement issue, except perhaps to give better messages when encountering the pattern. (Read: I can't triage the issue because I don't know what action is requested.) If we want to change the specified behavior, we should move the issue to the language repository. |
if (foo?.bar == somethingNotNull)
does not promote foo
if (foo?.bar == somethingNotNull)
should promote foo
cc @stereotype441 Am I correct that this is one of the cases that would be handled if we explicitly track the "nonNull" sets? |
@leafpetersen Yes, I believe it would, and I would actually really enjoy the mental exercise of making it work. But it would be quite a pandora's box to open, because the formulation we had with If I were going to tackle this idea I would play with As a backup plan, I might consider using |
I'm not sure what the general behavior you're proposing is. There are plenty of other cases where a void main() {
String? foo = 'Foo';
if (foo?.toUpperCase() == null) { ... }
if (foo?.toUpperCase() is Object?) { ... }
if (foo?.toUpperCase().isEmpty ?? true) { ... }
if (foo?.toUpperCase() == foo?.toUpperCase()) { ... }
if ([foo?.toUpperCase()].isNotEmpty) { ... }
if (Future.value(foo?.toUpperCase()) is Future) { ... }
} What are the rules you have in mind for when |
The general principle that I think @matanlurey and @rakudrama are going from is that:
One way to see this is:
The specific instance of
That said, I'm really quite surprised that this is considered something that "obviously should work". You guys are better at reasoning about nullability than I am... I had to think about that one for a bit. :) That said, I do see the reasonableness of the I'm skeptical about pushing on this right now, we really need to get this feature out the door. If we do tackle this (now or later), I think it would likely be on a somewhat ad hoc basis. Presumably we want to notice that one of the arguments of the equality was "obviously" a non-nullable literal, and use that to gather some promotion information while traversing the other argument. |
Not sure who you're asking, but speaking for myself, whatever we do my number 1 requirement is that it has to be sound, and in all your examples there, it wouldn't be sound to promote because the The thing Leaf and I were talking about with So, for the example of |
If we track more inferences through null-aware accesses, I guess it would mean: For
For
We then have to generalize that to arbitrary chains, including extension methods, but I do believe it's reasonably simple (if the selector is non-nullable it preserves the exact null-ness, if it's nullable, it preserves non-nullness, if it's Would it also make something like We would probably also want |
Yeah, that's pretty much what I was thinking, but explained much more clearly. Thank you 😃. One minor correction: flow analysis never reasons about when a variable's value is
Yes, except with the caveat mentioned above, so the only reasoning it would do is that if the entire result is non-null, then
Incidentally, this improvement could be done independently of whether or not we fix
Ditto for this. Currently flow analysis doesn't pay attention to calls to |
We could probably do that for We could exploit the knowledge that In the end we actually just came up with a few cases (which may be taken as a hint that it's a viable proposal to add these "missing" cases, because there aren't so many): Let |
True, we cannot generally infer anything about the type of |
Here are two patterns that I think may be common: if (x?.hasSomeField == true) {
doSomething(x.someField);
} if (x?.hasSomeField ?? false) {
doSomething(x.someField);
} The latter is recommended in Effective Dart so IMO it's the more important case to make work. After migration it would be nicer to see |
Yes, that was my point. :) All of my examples were structurally similar to what @matanlurey was requesting but should clearly not promote. I was trying to understand what the boundary line was that put @matanlurey's example on the "promotes" side but left all of mine on the "does not promote" side. It sounds like there is a boundary based on flow analysis, though I have to admit it feels pretty subtle to me. Though maybe not any more subtle than the flow analysis we already do. I like that the cases where we could make the flow analysis smarter are expressions that do clearly have an operator related to null:
+1. Or even the simpler (though likely much less common): test(bool? b) {
if (b ?? false) {
b + 1;
}
} |
I guess the generalized rule could be that Also, we can treat Together, those would handle Not sure how to handle |
It's very specific, but also explicitly suggested in "Effective Dart" so it may be worth handling explicitly. |
@stereotype441 How much work is this (either as a general feature, or to hit just the |
If we want to recognize variable ?. selectors ?? literalBoolean and then the branch for I wish we could recognize any constant boolean, but with We can potentially also recognize the expansion |
Probably a few days' work. Not sure when I can get to it though--let's talk priority in our meeting in a few minutes 😃 |
@natebosch found another example that I think would be addressed by this fix: String? current
while((current = next()) != null && current.isNotEmpty) { // Error, should be current!.isNotEmpty |
This last one makes good sense. An assignment to a local variable is guaranteed to evaluate to the current value of that variable, so testing the value can be used to infer something about the variable. If we introduce "binding assignments" or "local variable declarations" like We could technically allow |
…nfo. Previously, we used a single class hierarchy, ExpressionInfo, to store all the information that flow analysis needs to know about a variable, including: 1. What is known about the program state if the expression evaluates to true/false 2. Whether the expression is a `null` literal 3. Whether the expression is a reference to a variable. However, in order to address dart-lang/language#1274 (Infer non-nullability from local boolean variables), we'll need #3 to be tracked orthogonally from #1, so that when a local boolean is referred to later, we can track information of type #1 and #3 simultaneously. However, it makes sense to keep #1 and #2 in the same data structure, because future work is planned to represent them in a more uniform way, as part of addressing dart-lang/language#1224 (Using `if (foo?.bar == somethingNotNull)` should promote `foo`). Change-Id: I432f6e2e80543bb1d565b49403180c520eef66a5 Bug: dart-lang/language#1274 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/175008 Reviewed-by: Johnni Winther <johnniwinther@google.com> Commit-Queue: Paul Berry <paulberry@google.com>
…motion info." This reverts commit fd2a6c6. Reason for revert: Broke pkg/dds/test/sse_smoke_test Original change's description: > Flow analysis: Track expression variables separately from promotion info. > > Previously, we used a single class hierarchy, ExpressionInfo, to store > all the information that flow analysis needs to know about a variable, > including: > > 1. What is known about the program state if the expression evaluates > to true/false > > 2. Whether the expression is a `null` literal > > 3. Whether the expression is a reference to a variable. > > However, in order to address > dart-lang/language#1274 (Infer > non-nullability from local boolean variables), we'll need #3 to be > tracked orthogonally from #1, so that when a local boolean is referred > to later, we can track information of type #1 and #3 simultaneously. > > However, it makes sense to keep #1 and #2 in the same data structure, > because future work is planned to represent them in a more uniform > way, as part of addressing > dart-lang/language#1224 (Using `if (foo?.bar > == somethingNotNull)` should promote `foo`). > > Change-Id: I432f6e2e80543bb1d565b49403180c520eef66a5 > Bug: dart-lang/language#1274 > Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/175008 > Reviewed-by: Johnni Winther <johnniwinther@google.com> > Commit-Queue: Paul Berry <paulberry@google.com> TBR=paulberry@google.com,scheglov@google.com,johnniwinther@google.com Change-Id: I70b4adaf13f412a42a8128b9c7b9583b4171158e No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: dart-lang/language#1274 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/175321 Reviewed-by: Paul Berry <paulberry@google.com> Commit-Queue: Paul Berry <paulberry@google.com>
…motion info." This is a reland of fd2a6c6 Original change's description: > Flow analysis: Track expression variables separately from promotion info. > > Previously, we used a single class hierarchy, ExpressionInfo, to store > all the information that flow analysis needs to know about a variable, > including: > > 1. What is known about the program state if the expression evaluates > to true/false > > 2. Whether the expression is a `null` literal > > 3. Whether the expression is a reference to a variable. > > However, in order to address > dart-lang/language#1274 (Infer > non-nullability from local boolean variables), we'll need #3 to be > tracked orthogonally from #1, so that when a local boolean is referred > to later, we can track information of type #1 and #3 simultaneously. > > However, it makes sense to keep #1 and #2 in the same data structure, > because future work is planned to represent them in a more uniform > way, as part of addressing > dart-lang/language#1224 (Using `if (foo?.bar > == somethingNotNull)` should promote `foo`). > > Change-Id: I432f6e2e80543bb1d565b49403180c520eef66a5 > Bug: dart-lang/language#1274 > Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/175008 > Reviewed-by: Johnni Winther <johnniwinther@google.com> > Commit-Queue: Paul Berry <paulberry@google.com> Bug: dart-lang/language#1274 Change-Id: I002adbde782887def50dc80ab6673411b321c341 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/175362 Reviewed-by: Konstantin Shcheglov <scheglov@google.com> Reviewed-by: Johnni Winther <johnniwinther@google.com> Commit-Queue: Paul Berry <paulberry@google.com>
I've started to work on this using the idea of tracking The rules I'm considering would look something like this:
But I'm running into troublesome interactions with unsound null checking. Consider this mixed-version program: // Opted out
int f() => null;
// Opted in
int g() => f();
void test(int? x, Object? y, bool b) {
if (x?.remainder(y as int) == g()) { // (1)
if (b) { // (2)
print(x.isEven); // (3)
} else {
print(y.isEven); // (4)
}
}
} Using the above rules, what would happen during flow analysis is:
Which means that both Now consider what happens at runtime. If If But if I'll continue thinking about this and see if there's a way to get the functionality we want without the unsoundness escalation. My guess is that it will be possible, but it will involve something different than tracking the |
Another request for a feature covered by this discussion occurred here: dart-lang/sdk#45015 void main() {
final today = DateTime.now();
DateTime? userDate;
DateTime firstDate = (userDate?.isBefore(today) ?? false) ? userDate : today;
} |
will this be in 2.15? |
@jodinathan Sorry, no. The feature set for 2.15 has already been decided upon, and it doesn't include a fix for this. |
I just came across this as well (cf #2356). After reading through the comments, I sense there is an intent here to eventually fix this, but it is not entirely clear if we've reached an agreement on that. Could you clarify if that's the case? |
I think we agree that there are possibilities around some of the more common expression forms, but we do not have a concrete design for what to do to which expressions, which means no current concrete plan to actually do something. |
Another surprise (to me):
The text was updated successfully, but these errors were encountered: