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

improvement(semantic/cfg): rework error path. #3519

Merged
merged 1 commit into from
Jun 13, 2024

Conversation

rzvxa
Copy link
Contributor

@rzvxa rzvxa commented Jun 3, 2024

This PR aims to provide a more accurate error/finalization flow, I've nuked the old error path approach and rewrote it with more versatility in mind.

We used to visit the finalizer block twice and create 2 sets of AstNodes/Basic Blocks for them, This was there to differentiate between the error path finalizer and success path one. There is no longer a need for having 2 separate sets of nodes to do this differentiation.

As for the error path now we have 2 kinds of them, Everything is attached to an error block - even if it is not in a try-catch statement - this results in a lot of extra edges to keep track of these "Implicit" error blocks but I believe in future it can help us to track cross block error paths, For example, we can dynamically attach and cache the implicit error block of a function to its call site error path (either implicit or explicit).

Copy link

graphite-app bot commented Jun 3, 2024

Your org has enabled the Graphite merge queue for merging into main

Add the label “merge” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “hotfix” to add to the merge queue as a hot fix.

You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link.

Copy link

codspeed-hq bot commented Jun 3, 2024

CodSpeed Performance Report

Merging #3519 will not alter performance

Comparing 06-03-improvement_semantic_cfg_rework_error_path (9b30971) with main (59666e0)

Summary

✅ 22 untouched benchmarks

@github-actions github-actions bot added the A-ast Area - AST label Jun 3, 2024
@rzvxa rzvxa force-pushed the 05-30-improvement_linter_react_use_new_cfg_for_detection_conditional_nodes branch from 9aa8f48 to 94bc8d8 Compare June 5, 2024 19:14
@rzvxa rzvxa force-pushed the 06-03-improvement_semantic_cfg_rework_error_path branch from baa85e5 to 86be84b Compare June 5, 2024 19:14
@rzvxa rzvxa force-pushed the 05-30-improvement_linter_react_use_new_cfg_for_detection_conditional_nodes branch from 94bc8d8 to 3c67f42 Compare June 5, 2024 19:30
@rzvxa rzvxa force-pushed the 06-03-improvement_semantic_cfg_rework_error_path branch 2 times, most recently from 78d9b27 to 20d3e34 Compare June 5, 2024 19:47
@rzvxa rzvxa force-pushed the 05-30-improvement_linter_react_use_new_cfg_for_detection_conditional_nodes branch from 572e94a to e56665c Compare June 6, 2024 06:04
@rzvxa rzvxa force-pushed the 06-03-improvement_semantic_cfg_rework_error_path branch from 20d3e34 to 10fcc29 Compare June 6, 2024 06:04
@rzvxa rzvxa force-pushed the 05-30-improvement_linter_react_use_new_cfg_for_detection_conditional_nodes branch from e56665c to 33826cf Compare June 6, 2024 06:13
@rzvxa rzvxa force-pushed the 06-03-improvement_semantic_cfg_rework_error_path branch from 10fcc29 to 2ed1bb7 Compare June 6, 2024 06:13
@rzvxa rzvxa force-pushed the 05-30-improvement_linter_react_use_new_cfg_for_detection_conditional_nodes branch from 33826cf to 93a23d7 Compare June 6, 2024 06:16
@rzvxa rzvxa force-pushed the 06-03-improvement_semantic_cfg_rework_error_path branch from 2ed1bb7 to ce302b4 Compare June 6, 2024 06:16
@Boshen Boshen force-pushed the 05-30-improvement_linter_react_use_new_cfg_for_detection_conditional_nodes branch from 93a23d7 to 60f89d1 Compare June 6, 2024 07:58
@Boshen Boshen changed the base branch from 05-30-improvement_linter_react_use_new_cfg_for_detection_conditional_nodes to main June 6, 2024 08:04
@rzvxa rzvxa force-pushed the 06-03-improvement_semantic_cfg_rework_error_path branch from ce302b4 to 462af6d Compare June 6, 2024 12:52
@rzvxa rzvxa marked this pull request as ready for review June 6, 2024 13:12
@rzvxa rzvxa force-pushed the 06-03-improvement_semantic_cfg_rework_error_path branch from 462af6d to 9a2dc57 Compare June 6, 2024 15:23
@rzvxa rzvxa marked this pull request as draft June 6, 2024 15:48
@rzvxa rzvxa marked this pull request as ready for review June 12, 2024 23:01
@rzvxa
Copy link
Contributor Author

rzvxa commented Jun 12, 2024

@Boshen This stack is ready for review.

There is one thing I want you to consider carefully and give feedback on it. I've added a lot of "Implicit" error edges, Basically every basic block has an error edge even when it's not in a try block, And every function (+Program) has a basic block to harness these errors similar to a catch block. It results in extra memory usage and doesn't have a real impact on our current workflow I've mostly added them for forward compatibility if we want to handle cross-module/cross-function error unwinding.

So regarding your concern around this subject (#3641) we may want to make these edges optional or remove them altogether until we find a real use case. I added them when I wasn't aware that memory usage is an issue in the semantics.

@rzvxa rzvxa force-pushed the 06-03-improvement_semantic_cfg_rework_error_path branch from 0b13125 to b6a762d Compare June 12, 2024 23:08
Copy link

graphite-app bot commented Jun 13, 2024

Merge activity

Boshen pushed a commit that referenced this pull request Jun 13, 2024
This PR aims to provide a more accurate error/finalization flow, I've nuked the old error path approach and rewrote it with more versatility in mind.

We used to visit the finalizer block twice and create 2 sets of AstNodes/Basic Blocks for them, This was there to differentiate between the error path finalizer and success path one. There is no longer a need for having 2 separate sets of nodes to do this differentiation.

As for the error path now we have 2 kinds of them, Everything is attached to an error block - even if it is not in a try-catch statement - this results in a lot of extra edges to keep track of these "Implicit" error blocks but I believe in future it can help us to track cross block error paths, For example, we can dynamically attach and cache the implicit error block of a function to its call site error path (either implicit or explicit).
@Boshen Boshen force-pushed the 06-03-improvement_semantic_cfg_rework_error_path branch from b6a762d to 29808c4 Compare June 13, 2024 07:30
This PR aims to provide a more accurate error/finalization flow, I've nuked the old error path approach and rewrote it with more versatility in mind.

We used to visit the finalizer block twice and create 2 sets of AstNodes/Basic Blocks for them, This was there to differentiate between the error path finalizer and success path one. There is no longer a need for having 2 separate sets of nodes to do this differentiation.

As for the error path now we have 2 kinds of them, Everything is attached to an error block - even if it is not in a try-catch statement - this results in a lot of extra edges to keep track of these "Implicit" error blocks but I believe in future it can help us to track cross block error paths, For example, we can dynamically attach and cache the implicit error block of a function to its call site error path (either implicit or explicit).
@Boshen Boshen force-pushed the 06-03-improvement_semantic_cfg_rework_error_path branch from 29808c4 to 9b30971 Compare June 13, 2024 07:36
@graphite-app graphite-app bot merged commit 9b30971 into main Jun 13, 2024
22 checks passed
@graphite-app graphite-app bot deleted the 06-03-improvement_semantic_cfg_rework_error_path branch June 13, 2024 07:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ast Area - AST A-linter Area - Linter A-semantic Area - Semantic
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant