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

Analyzer and CFE disagree on type promotion due to boolean condition variables in pattern assignments #52183

Closed
stereotype441 opened this issue Apr 26, 2023 · 3 comments
Assignees
Labels
area-front-end Use area-front-end for front end / CFE / kernel format related issues.

Comments

@stereotype441
Copy link
Member

Background: Dart has a feature whereby a boolean condition can be stored in a variable and later used to control type promotion, e.g.:

f(int? x) {
  bool b;
  // ...
  b = x != null;
  // ...
  if (b) {
    print(x + 1); // OK; o has been promoted to `int`.
  }
}

When I added flow analysis support for pattern assignments, I tried to make pattern assignments have the same behaviour, e.g.:

f(int? x) {
  bool b;
  // ...
  (b) = x != null; // Note: pattern assignment
  // ...
  if (b) {
    print(x + 1); // OK; o has been promoted to `int`.
  }
}

But it looks like it doesn't work properly. The analyzer accepts this second example; the CFE does not.

Reproduced in c13acbf.

@stereotype441 stereotype441 added the area-front-end Use area-front-end for front end / CFE / kernel format related issues. label Apr 26, 2023
@stereotype441
Copy link
Member Author

I'm investigating now.

@stereotype441
Copy link
Member Author

Found the problem. The CFE generally uses the convention that the expressions passed to flow analysis are lowered expressions (that is, the expressions that remain after the rewrites that occur during type inference), whereas the shared analysis code for patterns follows the convention of passing pre-lowered expressions to flow analysis. Because of this mismatch, flow analysis is unable to keep track of the expression x != null (since it gets lowered by the CFE to !(x == null)), and so the promotion is lost.

I'm preparing a short-term workaround (which I'll upload for code review soon). In the long run I would like to rework the API for flow analysis so that the expressions passed to it are always the pre-lowered expressions. This should make it a lot easier to ensure consistency between the analyzer and the front end, since the analyzer hardly does any lowering at all. I've filed #52189 to track this longer-term work.

@chloestefantsova
Copy link
Contributor

In case it might be helpful, I worked on a similar bug some time ago: https://dart-review.googlesource.com/c/sdk/+/287601

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 <[email protected]>
Commit-Queue: Paul Berry <[email protected]>
copybara-service bot pushed a commit that referenced this issue Jun 16, 2023
In a future CL, I plan to refactor some of the flow analysis API so
that the CFE can supply subexpressions to flow analysis before they
are lowered, rather than after. This should help reduce the risk of
bugs where lowering leads to type promotion being lost
(e.g. #52183).

The refactor I'm planning will be easier if there are no calls to the
flow analysis API within flow analysis itself, so to prepare for it,
this CL moves the body of `FlowAnalysisImpl.functionExpression_begin`
to a private method, and updates both `functionExpression_begin` and
`lateInitializer_begin` (which use the same underlying logic) to call
that private method.

Bug: #52189
Change-Id: I1185418225b8a402670e6eece6599c326fdc234a
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/309440
Commit-Queue: Paul Berry <[email protected]>
Reviewed-by: Johnni Winther <[email protected]>
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

2 participants