#3057. Add more for-statement tests #3110
Conversation
|
Oh, I thought the flow analysis was always clear as day! ;-D |
There was a problem hiding this comment.
This is tricky! ;-)
I've tried to comment on a number of cases where a test has a description which claims to be about a specific formula in the spec (in particular, the conservative join), but it doesn't really seem to test that formula. It's more natural to say that it is testing some other thing (e.g., that after(B) contains a specific setting of 'assigned' for a variable, or 'captured', etc.)
| test1(int? n) { | ||
| if (n != null) { // n promoted to `int` | ||
| for (; () { | ||
| late int? i = (n = x); // n demoted to `int?` |
There was a problem hiding this comment.
I don't think write-captured variables care about which type we're assigning to them: They can't be promoted at any location where the write-capture has occurred (that is: the function object exists, no matter whether we "know" that it has or hasn't been called). So x could be a plain int, and it would still prevent all promotion of n.
I think this implies that the test would be slightly stronger if x were actually of type int.
There was a problem hiding this comment.
Also, the flow analysis makes no attempt to prove that n = x is dead code because we never read the value of i. All we need is the fact that n = x occurs in the function literal, no matter where.
There was a problem hiding this comment.
Thanx! x replaced by 42.
| /// - Let `before(U) = merge(after(S), continue(S))`. | ||
| /// - Let `after(N) = inheritTested(join(false(C), unsplit(break(S))), after(U))` | ||
| /// | ||
| /// @description Checks that `before(U) = merge(after(S), continue(S))`. Test |
There was a problem hiding this comment.
This is going to merge the flow models which is after(S) plus all the flow models from locations where S contains a statement of the form continue; or continue L; which is targeting this for statement. But then we should probably have at least one test that contains continue; or continue L;.
One thing which could be tested is that a variable which may be assigned before a continue; in the body is "not definitely unassigned" when it is used in U.
This test seems to be testing something which is quite different, and I'm actually not quite sure how I would describe it...
There was a problem hiding this comment.
OK, looking at this one more time I think I get it: The break; is needed because this test will actually run (so it should not enter an infinite loop). There is no continue; statement , but this is a case that needs to work, just like the case where we do have a continue; statement.
| test1(int? n) { | ||
| if (n != null) { // n promoted to `int` | ||
| for (; () { | ||
| late int? i = (n = x); // n demoted to `int?` |
There was a problem hiding this comment.
Thanx! x replaced by 42.
| test1(int? n) { | ||
| if (n != null) { // n promoted to `int` | ||
| for (; () { | ||
| late int? i = (n = x); // n demoted to `int?` |
| /// - Let `before(U) = merge(after(S), continue(S))`. | ||
| /// - Let `after(N) = inheritTested(join(false(C), unsplit(break(S))), after(U))` | ||
| /// | ||
| /// @description Checks that `before(U) = merge(after(S), continue(S))`. Test |
There was a problem hiding this comment.
OK, looking at this one more time I think I get it: The break; is needed because this test will actually run (so it should not enter an infinite loop). There is no continue; statement , but this is a case that needs to work, just like the case where we do have a continue; statement.
| try { | ||
| for (;; i) { // Possibly assigned. https://github.com/dart-lang/sdk/issues/42232#issuecomment-690681385 | ||
| continue; | ||
| i = 42; |
There was a problem hiding this comment.
This is a good one because it shows that the analysis is somewhat lossy: We will actually never execute line 23, but the analysis still thinks that it might have been executed when we reach U.
Of course, developers would be expected to modify their code such that there is no dead code, which means that it isn't a big problem that the analysis can be made less useful by dead code.
2025-03-21 [email protected] dart-lang/co19#3057. Update expectations for `while(false) ...` test (dart-lang/co19#3114) 2025-03-19 [email protected] dart-lang/co19#3057. Add more for-statement tests (dart-lang/co19#3110) 2025-03-17 [email protected] dart-lang/co19#2119. Fix typos in descriptions (dart-lang/co19#3109) 2025-03-17 [email protected] dart-lang/co19#3057. Add `for(;;)` statements tests (dart-lang/co19#3108) 2025-03-14 [email protected] dart-lang/co19#3057. Add while statements tests and update assignments (dart-lang/co19#3107) 2025-03-14 [email protected] dart-lang/co19#3057. Add conditional statements tests (dart-lang/co19#3106) Cq-Include-Trybots: luci.dart.try:analyzer-linux-release-try Change-Id: I7edea518e4d853da39d2f1b78674ab6abeadccb7 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/417180 Reviewed-by: Alexander Thomas <[email protected]> Commit-Queue: Erik Ernst <[email protected]> Reviewed-by: Erik Ernst <[email protected]>
It turns out that flow analysis after
breakstatement can be not intuitive :)