-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
FlowAnalysis.forwardExpression
should be eliminated
#52189
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
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
copybara-service bot
pushed a commit
that referenced
this issue
May 10, 2023
…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 <[email protected]> Commit-Queue: Paul Berry <[email protected]>
copybara-service bot
pushed a commit
that referenced
this issue
May 11, 2023
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 <[email protected]> Commit-Queue: Paul Berry <[email protected]>
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-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
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.
The text was updated successfully, but these errors were encountered: