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.

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

CFE sometimes fails to promote to non-nullable based on null checks in guards #52241

Closed
stereotype441 opened this issue May 2, 2023 · 1 comment
Assignees
Labels
area-front-end Use area-front-end for front end / CFE / kernel format related issues.

Comments

@stereotype441
Copy link
Member

stereotype441 commented May 2, 2023

When running under the VM, the following constructs fail to promote:

var _ = switch (...) {
  int? y when y != null => ... // `y` is not promoted here
  ...
};
if (... case int? y when y != null) {
  // `y` is not promoted here
}
var _ = [if (... case int? y when y != null) ... // `y` is not promoted here
    ]; // And similar for sets and maps

However, trying to do the same thing with a switch statement does promote:

switch (...) {
  case int? y when y != null:
    // `y` is promoted to `int` here
}

This doesn't happen when running the CFE by itself (e.g. when testing in cfe-strong-linux mode). But it does happen if the program is run under the VM.

The analyzer properly promotes in all these circumstances.

I'm working on a fix.

@stereotype441 stereotype441 added the area-front-end Use area-front-end for front end / CFE / kernel format related issues. label May 2, 2023
@stereotype441 stereotype441 self-assigned this May 2, 2023
@stereotype441
Copy link
Member Author

This is happening because of inconsistencies in the CFE and the _fe_analyzer_shared logic about whether the expressions passed to flow analysis should be pre-lowered expressions or lowered expressions. The CFE logic for handling forms like y != null lowers the expression to !(y == null) when compiling for the VM, and uses the convention of passing the lowered expression to flow analysis. The shared logic for switch statements re-reads the guard expression after visiting it (picking up the lowered expression), and uses it in further calls to flow analysis. So it's effectively using the same convention. This is why the promotion works in switch statements. However, the shared logic for switch expressions, if-case statements, and if-case elements doesn't re-read the guard expression after visiting it, so effectively it's using the convention of passing the pre-lowered expression to flow analysis; as a result, promotion doesn't work.

I'm currently working on a fix that changes the shared logic to uniformly use convention of passing the pre-lowered expression to flow analysis, and adds a call to FlowAnalysis.forwardExpression to sync up the conventions when necessary. That should fix this issue in the short term. In the long term, I plan to fix the issue for good by switching everything over to using pre-lowered expressions (see #52189).

copybara-service bot pushed a commit that referenced this issue May 12, 2023
…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 <johnniwinther@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-front-end Use area-front-end for front end / CFE / kernel format related issues.
Projects
None yet
Development

No branches or pull requests

1 participant