-
Notifications
You must be signed in to change notification settings - Fork 110
BE-263: HashQL: add body! macro for declarative MIR construction in tests
#8229
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
Conversation
PR SummaryIntroduces a declarative MIR-construction DSL and migrates tests to it.
Written by Cursor Bugbot for commit ed471cd. This will update automatically on new commits. Configure here. |
Merging this PR will not alter performance
Comparing Footnotes
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## bm/be-261-hashql-mir-administrative-reduction-pass #8229 +/- ##
=======================================================================================
+ Coverage 59.24% 87.22% +27.98%
=======================================================================================
Files 1191 69 -1122
Lines 113321 8853 -104468
Branches 4981 252 -4729
=======================================================================================
- Hits 67134 7722 -59412
+ Misses 45411 1034 -44377
+ Partials 776 97 -679 Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
10798e5 to
fd458f2
Compare
4156c74 to
ed471cd
Compare
Merge activity
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review completed. No suggestions at this time.
Comment augment review to trigger a new review at any time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| /// | ||
| /// | Syntax | Description | | ||
| /// | ------ | ----------- | | ||
| /// | `let x;` | `StorageLive(x)` | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The table says let x; / drop x; map to StorageLive(x) / StorageDead(x), but bb! expands these to storage_live($name) / storage_dead($name) which take a Local (while body! binds locals as Place). This makes the documented syntax look like it won’t type-check unless callers use something like x.local.
🤖 Was this useful? React with 👍 or 👎
| (@type $types:ident; Int) => { | ||
| $types.integer() | ||
| }; | ||
| (@type $types:ident; ($($sub:tt),*)) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In body!(@type ...), the tuple arm (@type $types; ($($sub:tt),*)) appears before the struct arm (@type $types; ($($name:ident: $sub:tt),*)), so a struct type like (a: Int, b: Bool) will match the tuple arm first and then fail to expand. This seems to contradict the docs that advertise struct-type support in body!.
🤖 Was this useful? React with 👍 or 👎

🌟 What is the purpose of this PR?
Introduces the body! macro, a declarative DSL for constructing MIR bodies in tests. This replaces the verbose fluent builder API as the primary approach, making tests significantly more readable and concise (~940 net lines removed).
🔍 What does this change?
Pre-Merge Checklist 🚀
🚢 Has this modified a publishable library?
This PR:
📜 Does this require a change to the docs?
The changes in this PR:
🕸️ Does this require a change to the Turbo Graph?
The changes in this PR:
📹 Demo
Before (fluent builder):
After (body! macro):