Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[CP] Account for differing flow analysis conventions between CFE and shared code. #52343

Closed
stereotype441 opened this issue May 10, 2023 · 2 comments
Closed

[CP] Account for differing flow analysis conventions between CFE and shared code. #52343

stereotype441 opened this issue May 10, 2023 · 2 comments
Assignees
@vsmenon @whesse @itsjustkevin @mit-mit @athomas
Labels
area-front-end Use area-front-end for front end / CFE / kernel format related issues. cherry-pick-approved Label for approved cherrypick request cherry-pick-merged Cherry-pick has been merged to the stable or beta branch. cherry-pick-review Issue that need cherry pick triage to approve merge-to-stable

Comments

@stereotype441
Copy link
Member

Commit(s) to merge

cb0eb28

Target

Stable

Prepared changelist for beta/stable

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

Issue Description

When running in the VM, certain expressions that imply type promotions are sometimes rewritten by the compilation pipeline. If those expressions appear in certain contexts involving patterns, the rewrite could cause type promotion to be lost. This happens because of differences in flow analysis conventions between the front end and the shared implementation of patterns: the front end assumes that the rewritten expressions will be passed to flow analysis, whereas the shared logic assumes the original expressions will be passed to flow analysis.

For example, this code is accepted by the analyzer but not the VM, because the guard clause j != null is internally rewritten to !(j == null), causing the promotion of j to non-nullable to be lost:

int f(int i, int? j) => switch (i) {
  0 when j != null => j,
  _ => i
};

What is the fix

We account for the difference in conventions by adding a call to FlowAnalysis.forwardExpression to the front end's implementation of dispatchExpression. This ensures that when transitioning between front end logic and shared logic, flow analysis will be informed how to match up the rewritten expressions to their original counterparts.

Why cherry-pick

This change fixes type promotion to be consistent between the analyzer and the VM. Without this fix, there could be customer frustration, because a type might appear to promote properly in the IDE, but then fail to compile when run under the VM.

The fix is confined to the new shared analysis infrastructure and the dispatchExpression method, which are only used for patterns and switch statements. Since switch statements didn't support any type promotion prior to Dart 3.0, this means that no user code written prior to Dart 3.0 should be affected. Also, since the change brings the VM behaviour into alignment with the analyzer, it shouldn't break any user code that is already accepted by the analyzer. So I believe the overall risk of this change is low.

Risk

low

Issue link(s)

#52241, #52183

Extra Info

No response
@stereotype441 stereotype441 added the cherry-pick-review Issue that need cherry pick triage to approve label May 10, 2023
@itsjustkevin
Copy link
Contributor

@johnniwinther for review!

@johnniwinther
Copy link
Member

LGTM

@itsjustkevin itsjustkevin added merge-to-stable cherry-pick-approved Label for approved cherrypick request labels May 12, 2023
copybara-service bot pushed a commit that referenced this issue May 12, 2023
@stereotype441
[stable] Account for differing flow analysis conventions between CFE …
c6b0b74 
…and shared code.

With one exception (noted below), the shared analysis logic uses the
convention that the expressions passed to flow analysis are the
original (pre-lowered) expressions, whereas the expressions passed to
flow analysis by the CFE are the lowered expressions. This difference
caused two problems:

- If a boolean expression appeared on the right hand side of a pattern
  assignment or pattern variable declaration, and it was lowered, the
  flow analysis implications of that boolean expression would be lost.

- If a boolean expression appeared in a `when` clause, and it was
  lowered, the flow analysis implications of that boolean expression
  would be lost. Exception: for switch statements, the shared analysis
  logic has been passing lowered expressions to flow analysis, so this
  problem didn't occur for `when` clauses in switch statements.

Notably, when compiling for the VM, the CFE lowers expressions like
`x != null` to something more like `!(x == null)`.

Fortunately, the first of these two situations shouldn't cause
problems very often, since typically if the right hand side of an
assignment or variable declaration is a boolean expression, there is
no need for the left hand side to involve patterns.

As for the second of these two situations, it's also not too likely to
cause problems, since typically null checks occur inside patterns
rather than in `when` clauses.

As a short term fix, we remove the exception noted above, and we
account for the difference in conventions by adding a call to
`FlowAnalysis.forwardExpression` to the CFE's implementation of
`dispatchExpression`, so that when transitioning between CFE logic and
shared logic, flow analysis will be informed how to match up the
lowered expressions to their pre-lowered counterparts.

Longer term, I would like to switch everything to the convention of
using the original (pre-lowered) expressions; this will bring the
analyzer and CFE into better alignment with each other and should
eliminate a class of subtle bugs. This long term goal is tracked in
#52189.

Fixes: #52343

Bug: #52183, #52241.
Change-Id: I662ac0127b25e92588100dd19737bf04a9979481
Cherry-pick: https://dart-review.googlesource.com/c/sdk/+/298840
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/302642
Reviewed-by: Johnni Winther <[email protected]>
Commit-Queue: Paul Berry <[email protected]>
@itsjustkevin itsjustkevin added the cherry-pick-merged Cherry-pick has been merged to the stable or beta branch. label May 15, 2023
@lrhn lrhn added the area-front-end Use area-front-end for front end / CFE / kernel format related issues. label May 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@vsmenon vsmenon

@whesse whesse

@itsjustkevin itsjustkevin

@mit-mit mit-mit

@athomas athomas

Labels
area-front-end Use area-front-end for front end / CFE / kernel format related issues. cherry-pick-approved Label for approved cherrypick request cherry-pick-merged Cherry-pick has been merged to the stable or beta branch. cherry-pick-review Issue that need cherry pick triage to approve merge-to-stable
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
8 participants
@stereotype441 @vsmenon @lrhn @whesse @itsjustkevin @johnniwinther @mit-mit @athomas