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

Flow analysis represents reachability as a single boolean, not a stack #40009

Closed
stereotype441 opened this issue Jan 7, 2020 · 4 comments
Closed

Flow analysis represents reachability as a single boolean, not a stack #40009

stereotype441 opened this issue Jan 7, 2020 · 4 comments
Assignees
@stereotype441
Labels
analyzer-spec Issues with the analyzer's implementation of the language spec area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. area-front-end Use area-front-end for front end / CFE / kernel format related issues. NNBD Issues related to NNBD Release type-enhancement A request for a change that isn't a bug

Comments

@stereotype441
Copy link
Member

The flow analysis pull request (dart-lang/language#763) defines FlowModel.reachable as a stack of booleans; this is necessary in order for promotions to work properly inside of unreachable code blocks, for example:

return;
if (x is! int) return;
x.isEven; // Should be OK

The current implementation defines FlowModel.reachable as a single boolean. I'm not sure if the above case works by some other magic or not, but it seems like it would be best to just switch to the boolean stack defined by the spec proposal.
@stereotype441 stereotype441 added area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. area-front-end Use area-front-end for front end / CFE / kernel format related issues. NNBD Issues related to NNBD Release labels Jan 7, 2020
@stereotype441 stereotype441 mentioned this issue Jan 7, 2020
Merged
@stereotype441
Copy link
Member Author

Note that this will also require implementing and wiring up the split, drop, unsplit, and merge functions documented in that same pull request.

@srawlins srawlins added the analyzer-spec Issues with the analyzer's implementation of the language spec label Jun 16, 2020
@leafpetersen
Copy link
Member

@stereotype441 is this fixed?

@franklinyow franklinyow added the type-enhancement A request for a change that isn't a bug label Sep 3, 2020
@stereotype441
Copy link
Member Author

@leafpetersen the implementation doesn't currently use a stack, but so far I haven't found a concrete scenario where that leads to incorrect behavior. I'm not sure where that leaves us. I would still like to switch to a stack if time allows, so that we can have more confidence in the correctness of the implementation, but it seems like that is lower in priority unless we find a concrete bug.

@stereotype441 stereotype441 self-assigned this Sep 29, 2020
@stereotype441
Copy link
Member Author

After further investigation, it seems that promotion is currently broken inside dead code blocks, so I'm working on this now.

dart-bot pushed a commit that referenced this issue Sep 30, 2020
@stereotype441
Replace flow analysis's setReachable with setUnreachable.
0d35cf7 
The setReachable method took a bool indicating whether the new state
should be considered reachable or not, however outside of tests the
value that was passed in was always `false`.  Change to a
`setUnreachable` method taking no arguments.

This should make it easier to transition to a stack representation for
reachability.

Bug: #40009
Change-Id: I16a35ec3d5f44763a60e42f200a3caa619fac118
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/165142
Reviewed-by: Johnni Winther <johnniwinther@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
dart-bot pushed a commit that referenced this issue Oct 7, 2020
@stereotype441
Replace flow ranalysis reachability bool with a stack.
19995ba 
Currently we don't push or pop anything on the stack, we just
manipulate the single value that's on the stack.  So there's no
functional change.  Logic that pushes and pops the stack will be added
in a follow-up CL.

Bug: #40009
Change-Id: I4deb4cbe06644a2b2c9b5e2c1dc140fb4cd805cf
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/165145
Commit-Queue: Paul Berry <paulberry@google.com>
Reviewed-by: Johnni Winther <johnniwinther@google.com>
dart-bot pushed a commit that referenced this issue Oct 7, 2020
@stereotype441
Ensure that if statements promote properly in unreachable code
ed2f45d 
Bug: #40009
Change-Id: I04a5af558bb70b861d92b5379a8fb84489d5c9f4
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/165402
Commit-Queue: Paul Berry <paulberry@google.com>
Reviewed-by: Johnni Winther <johnniwinther@google.com>
dart-bot pushed a commit that referenced this issue Oct 7, 2020
@stereotype441
Ensure that conditional expressions promote properly in unreachable code
57932cf 
Change-Id: I8680b81b44be789a047238b1e2d008ff240e59c2
Bug: #40009
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/165405
Reviewed-by: Johnni Winther <johnniwinther@google.com>
dart-bot pushed a commit that referenced this issue Oct 7, 2020
@stereotype441
Ensure that do statements promote properly in unreachable code.
c39b164 
Bug: #40009
Change-Id: I2c53e1171bb986784b1f0e18e35556f7d6afcb15
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/166320
Commit-Queue: Paul Berry <paulberry@google.com>
Reviewed-by: Johnni Winther <johnniwinther@google.com>
dart-bot pushed a commit that referenced this issue Oct 7, 2020
@stereotype441
Ensure that for statements promote properly in unreachable code.
7aa9444 
Bug: #40009
Change-Id: I87ea8c8cd943c0c44d795aec084e012b3e2b4496
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/166500
Reviewed-by: Johnni Winther <johnniwinther@google.com>
dart-bot pushed a commit that referenced this issue Oct 7, 2020
@stereotype441
Adjust flow analysis of for each statements for consistency with `f…
c13f4ab 
…or`.

The recent changes to `for` statements were necessary to ensure that
if a `for` statement causes type promotion due to an unreachable
branch without type promotion joining a reachable one with type
promotion, the type promotion would still happen properly even if the
top of the `for` statement was itself unreachable.  There is no need
for a similar change to `for each` statements, because they are simple
enough that this situation cannot arise.  But it's still worth making
the corresponding change for consistency.

Bug: #40009
Change-Id: Idabf7745f04a99152e688bf050a8d80e07b004e2
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/166520
Commit-Queue: Paul Berry <paulberry@google.com>
Reviewed-by: Johnni Winther <johnniwinther@google.com>
dart-bot pushed a commit that referenced this issue Oct 8, 2020
@stereotype441
Ensure that ?? and ?. expressions promote properly in unreachable…
ce7accb 
… code.

Due to #43725 these have to be
updated in a single atomic CL.

Bug: #40009
Change-Id: Ifbd5fdee6379b7cbee5ab135f72a013134b70aa1
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/166601
Reviewed-by: Johnni Winther <johnniwinther@google.com>
dart-bot pushed a commit that referenced this issue Oct 8, 2020
@stereotype441
Ensure that labeled statements promote properly in unreachable code.
17ebbfc 
Bug: #40009
Change-Id: Ice9b50ee6a7e9ffc32635312dc951362ff0da665
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/166540
Reviewed-by: Johnni Winther <johnniwinther@google.com>
dart-bot pushed a commit that referenced this issue Oct 8, 2020
@stereotype441
Ensure that && and || expressions promote properly in unreachable…
2601fa6 
… code.

Bug: #40009
Change-Id: Ib06449624de9320f5229957511ebaaee92c75ec8
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/166606
Reviewed-by: Johnni Winther <johnniwinther@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
dart-bot pushed a commit that referenced this issue Oct 8, 2020
@stereotype441
Ensure that switch statements promote properly in unreachable code.
ef16466 
Bug: #40009
Change-Id: I00a8fcf52bce131874ba6d96680b11e8bc1a549c
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/166640
Reviewed-by: Johnni Winther <johnniwinther@google.com>
dart-bot pushed a commit that referenced this issue Oct 8, 2020
@stereotype441
Ensure that try/catch statements promote properly in unreachable code.
f51fb06 
Bug: #40009
Change-Id: Ie4b8809c6f37ff67891bc874c0c0ec10c70eb9fa
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/166660
Reviewed-by: Johnni Winther <johnniwinther@google.com>
dart-bot pushed a commit that referenced this issue Oct 8, 2020
@stereotype441
Ensure that while statements promote properly in unreachable code.
18644c3 
Bug: #40009
Change-Id: I492b465b08a0f00549e8e69390cead35af211e0b
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/166700
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

@stereotype441 stereotype441

Labels
analyzer-spec Issues with the analyzer's implementation of the language spec area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. area-front-end Use area-front-end for front end / CFE / kernel format related issues. NNBD Issues related to NNBD Release type-enhancement A request for a change that isn't a bug
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@srawlins @stereotype441 @leafpetersen @franklinyow