[CP] Flow analysis: fix first phase handling of pattern assignments.

[stereotype441]

Commit(s) to merge

2ca7380

Target

Stable

Prepared changelist for beta/stable

https://dart-review.googlesource.com/c/sdk/+/310702

Issue Description

During the first phase of flow analysis, in which flow analysis "looks ahead" to see which variables are potentially assigned in each code block, the analyzer and front end fail to properly account for variables that are assigned by pattern assignment expressions. This "look ahead" information is used by flow analysis to determine the effects of nonlinear control flow (e.g. to figure out which variables to demote at the top of a loop, or to figure out which variables are potentially assigned inside a closure). As a result, some correct code is rejected by the analyzer and compiler, e.g.:

void main() {
  late String s;
  () {
    (s,) = ('',);
  }();
  // `s` is assigned in the closure above, so it is ok to refer to it here.
  // But due to the bug it is reported as "definitely unassigned"
  print(s);
}

Also, some incorrect code is (unsoundly) accepted, e.g.:

int f(int? i) {
  if (i == null) return 0;
  int k = 0;
  // `i` is promoted to non-nullable `int` now
  for (int j = 0; j < 2; j++) {
    // `i` should be demoted at this point, because it's assigned later in the
    // loop, so this statement should be an error. But due to the bug no error
    // is reported.
    k += i;
    // Now assign a nullable value to `i`.
    (i,) = (null,);
  }
  return k;
}

void main() {
  print(f(0));
}

What is the fix

The fix is to call AssignedVariables.write from the analyzer's FlowAnalysisVisitor, and from the CFE's BodyBuilder, to let flow analysis know about variable assignments that occur inside pattern assignment expressions.

Why cherry-pick

If the fix is not cherry-picked, it's possible that an incorrect (and therefore buggy) program might be accepted by the compiler, leading to a crash. The risk of this happening is fairly low today, because the new "patterns" feature hasn't been used very much yet, but will increase over time as the feature becomes more widely used.

The risk is low, since the only code path in the analyzer and front end that's affected by the fix is the code path for dealing with assignments to variables inside pattern assignments, and this is precisely the situation in which the bug occurs.

Risk

low

Issue link(s)

#52745

Extra Info

No response

[itsjustkevin]

@srawlins for review

[stereotype441]

@itsjustkevin it looks like @srawlins is out of the office for the next week. Can we get review from someone who is around? I'd rather not have to wait an extra week to cherry-pick this fix, since it's a soundness issue.

[srawlins]

Sorry I reviewed this last week and forgot to comment: I think this is good for cherry-picking.