fix: rewrite typechecker journal to handle nested commits #3375
Conversation
Codecov Report
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more @@ Coverage Diff @@
## master #3375 +/- ##
==========================================
- Coverage 88.85% 88.80% -0.05%
==========================================
Files 84 85 +1
Lines 10673 10706 +33
Branches 2226 2233 +7
==========================================
+ Hits 9483 9507 +24
- Misses 789 798 +9
Partials 401 401
... and 2 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
vyper/ast/metadata.py
Outdated
|
|
||
| @classmethod | ||
| @contextlib.contextmanager | ||
| def speculate(cls): |
There was a problem hiding this comment.
Kind of a weird function name, maybe analyze or validate
There was a problem hiding this comment.
it enters a stateful transaction which could be rolled back, so it would be something like speculate(), or a more vanilla(!) name would be enter() or begin_transaction()
There was a problem hiding this comment.
From the point of view of the compilation, or runtime?
There was a problem hiding this comment.
It's a compile time stateful transaction on the ast. I just realized I still need to write the PR notes.
There was a problem hiding this comment.
The naming doesn't make that totally clear to me, at a quick glance
There was a problem hiding this comment.
i renamed to enter_typechecker_speculation. it's a bit long-winded but it seems descriptive enough to me and invites the reader to take a closer look (which they should! as this is a complicated operation going on.)
this commit fixes a bug which was introduced in 66930fd. when the typechecker enters a nested loop, it can typecheck the inner loop (committing it), and result in invalid state if the outer loop fails to typecheck. this commit implements a checkpointing system in the typechecker state committer so that it can handle nested changes. it also cleans up the data structure used so that there is a single entry point and users of the data structure do not need to think about implementation details. one drawback of this approach is that it can *only* handle changes to the metadata dict. non-idempotent changes to the AST during typechecking should then be restricted to changes to the metadata dict so that they can register automatically with the node metadata journal.
What I did
fix #3374
How I did it
How to verify it
Commit message
Commit message for the final, squashed PR. (Optional, but reviewers will appreciate it! Please see our commit message style guide for what we would ideally like to see in a commit message.)
Description for the changelog
Cute Animal Picture