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

FlowAnalysis.forwardExpression should be eliminated #52189

Open
stereotype441 opened this issue Apr 26, 2023 · 0 comments
Open

FlowAnalysis.forwardExpression should be eliminated #52189

stereotype441 opened this issue Apr 26, 2023 · 0 comments
Labels
area-fe-analyzer-shared Assigned by engineers; when triaging, prefer either area-front-end or area-analyzer. fe-analyzer-shared-flow-analysis fe-analyzer-shared-technical-debt

Comments

@stereotype441
Copy link
Member

I believe the only reason that FlowAnalysis.forwardExpression exists at all is because of some lack of clarity on my part during the initial implementation of flow analysis. I had not yet become convinced (as I now am) that flow analysis should process the user's code in a form that is as close as possible to the user's source code, not the lowered form produced by the front end during type inference.

Because of this lack of clarity, we would up with a design where in many cases, the expressions that the CFE passes to methods like FlowAnalysis.ifStatement_thenBegin are the lowered expressions. As a result, when lowering happens between two calls to flow analysis methods, FlowAnalysis.forwardExpression must be used to inform flow analysis that the expression has been rewritten into a new form, so that at the time of the latter call, it can match up the new form with the old one.

Things get even more complicated when only some of the code that interfaces with flow analysis uses the convention of passing lowered expressions to flow analysis, and other code passes in pre-lowered expressions (see #52183).

It would be far better to simply always follow the convention that the expressions passed to flow analysis are in their pre-lowered forms. This would make the CFE and the analyzer much more consistent with one another, and also pave the way for a possible future where we don't have to use the same set of types for the pre-lowering and post-lowering representations of the user's code.
@stereotype441 stereotype441 added area-fe-analyzer-shared Assigned by engineers; when triaging, prefer either area-front-end or area-analyzer. fe-analyzer-shared-flow-analysis fe-analyzer-shared-technical-debt labels Apr 26, 2023
This was referenced Apr 26, 2023
Closed
Closed
copybara-service bot pushed a commit that referenced this issue May 10, 2023
@stereotype441
Account for differing flow analysis conventions between CFE and share…
cb0eb28 
…d 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 #52183.
Fixes #52241.

Bug: #52183, #52241.
Change-Id: I2449ce34c54325603bc2730d1660a7cfc7d79aec
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/298840
Reviewed-by: Johnni Winther <johnniwinther@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
copybara-service bot pushed a commit that referenced this issue May 11, 2023
@stereotype441
Remove unused parameter and return value from handleCaseHead.
7968ea8 
These were introduced in
https://dart-review.googlesource.com/c/sdk/+/287601 to allow the
common front end to report back to the shared analysis logic when it
desugars a guard expression, so that flow analysis will properly
account for any promotions made by the guard expression; previously,
if a guard expression got desugared by the front end, promotions
performed by the guard would be lost.

In https://dart-review.googlesource.com/c/sdk/+/298840, we've switched
to a more general technique for ensuring that promotions performed by
the guard will not be lost: now the `dispatchExpression` method uses
`FlowAnalysis.forwardExpression` to compensate for the differences in
the conventions of the CFE and the shared logic, so that the shared
logic no longer needs access to the desugared expression. So the
`head` parameter (and the associated return value) are no longer
needed.

In a future change, I plan to adjust the CFE conventions to match
those of the shared logic, so that `FlowAnalysis.forwardExpression`
won't be needed at all (see
#52189).

Change-Id: I686d1d5b7e4c9db353f583c38792274451f9f8bb
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/302366
Reviewed-by: Johnni Winther <johnniwinther@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
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 <johnniwinther@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
copybara-service bot pushed a commit that referenced this issue Jun 16, 2023
@stereotype441
Flow analysis: clean up public method call to functionExpression_begin.
788a3a9 
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 <paulberry@google.com>
Reviewed-by: Johnni Winther <johnniwinther@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
area-fe-analyzer-shared Assigned by engineers; when triaging, prefer either area-front-end or area-analyzer. fe-analyzer-shared-flow-analysis fe-analyzer-shared-technical-debt
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@stereotype441