BE-581, BE-589: HashQL: Introduce pass::lower/pass::place pipeline facades and fix cached constructor closure bug#8838
Conversation
chore: add new dependency chore: format feat: error module feat: introduce hashql_eval interner chore: checkpoint feat: checkpoint feat: checkpoint chore: remove old value module feat: checkpoint feat: checkpoint feat: checkpoint feat: checkpoint feat: checkpoint chore: checkpoint feat: move entity query into its own modul fix: query request feat: checkpoint (it compiles!) feat: checkpoint feat: checkpoint feat: checkpoint fix: issue around cached thunking feat: covariance for opaque inners fix: cfgattr serde chore: remove graph module
PR SummaryMedium Risk Overview Reify now takes a Smaller follow-ons: custom Reviewed by Cursor Bugbot for commit b939729. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## bm/be-585-hashql-split-mir-interpret-diagnostic-api #8838 +/- ##
=======================================================================================
- Coverage 59.56% 59.53% -0.04%
=======================================================================================
Files 1347 1347
Lines 130913 130960 +47
Branches 5884 5884
=======================================================================================
- Hits 77982 77961 -21
- Misses 52027 52092 +65
- Partials 904 907 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
pass::lower/pass::place pipeline facades and fix cached constructor closure bugpass::lower/pass::place pipeline facades and fix cached constructor closure bug
There was a problem hiding this comment.
Pull request overview
This PR refactors HashQL’s MIR pipeline API by introducing facade functions for lowering and execution placement, while also fixing a MIR reification bug where cached constructor references were emitted with the wrong calling convention representation (thin FnPtr vs closure aggregate fat pointer). It updates the compiletest harness and UI tests to cover the regression and to reflect the corrected interpreter behavior.
Changes:
- Add
pass::lowerandpass::placepipeline facades (plusLowerConfig) to centralize scratch resets and diagnostic draining. - Fix constructor caching in MIR reification so both first-use and cache-hit paths emit
closure(FnPtr, ())aggregates. - Thread scratch allocator support through reification/compiletest utilities and add/adjust UI regression tests.
Reviewed changes
Copilot reviewed 38 out of 38 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| libs/@local/hashql/mir/tests/ui/reify/ctor-cached-closure.stdout | New expected MIR output asserting cached ctor uses emit closure(FnPtr, ()). |
| libs/@local/hashql/mir/tests/ui/reify/ctor-cached-closure.jsonc | New UI test input for cached-constructor closure aggregate regression. |
| libs/@local/hashql/mir/tests/ui/interpret/access-struct-through-opaque.stdout | New passing interpreter output snapshot for opaque struct access. |
| libs/@local/hashql/mir/tests/ui/interpret/access-struct-through-opaque.stderr | Removes prior ICE stderr expectations now that the bug is fixed. |
| libs/@local/hashql/mir/tests/ui/interpret/access-struct-through-opaque.jsonc | Flips the test directive from run: fail to run: pass. |
| libs/@local/hashql/mir/tests/ui/interpret/access-struct-through-opaque.aux.mir | Updates auxiliary MIR to reflect closure aggregate emission and downstream simplifications. |
| libs/@local/hashql/mir/src/reify/transform.rs | Updates Reifier impl generics to support allocator-parametrized context. |
| libs/@local/hashql/mir/src/reify/terminator.rs | Updates Reifier impl generics to match new allocator parameters. |
| libs/@local/hashql/mir/src/reify/rvalue.rs | Fixes ctor caching path to always build a closure aggregate (fat pointer). |
| libs/@local/hashql/mir/src/reify/mod.rs | Extends ReifyContext with scratch allocator and threads allocator generics through reifier state. |
| libs/@local/hashql/mir/src/reify/current.rs | Adds #[inline] on small From impls. |
| libs/@local/hashql/mir/src/reify/atom.rs | Updates Reifier impl generics to match new allocator parameters. |
| libs/@local/hashql/mir/src/pass/transform/ssa_repair/mod.rs | Adds #[inline] on Default impl. |
| libs/@local/hashql/mir/src/pass/transform/inst_simplify/mod.rs | Adds #[inline] on Default impl. |
| libs/@local/hashql/mir/src/pass/transform/forward_substitution.rs | Adds #[inline] on Default impl. |
| libs/@local/hashql/mir/src/pass/transform/dse/mod.rs | Adds #[inline] on Default impl. |
| libs/@local/hashql/mir/src/pass/transform/copy_propagation/mod.rs | Adds #[inline] on Default impl. |
| libs/@local/hashql/mir/src/pass/transform/cfg_simplify/mod.rs | Adds #[inline] on Default impl. |
| libs/@local/hashql/mir/src/pass/transform/canonicalization.rs | Adds #[inline] on Default impl for config. |
| libs/@local/hashql/mir/src/pass/mod.rs | Introduces LowerConfig, pass::lower, and pass::place facade functions. |
| libs/@local/hashql/mir/src/pass/execution/traversal/mod.rs | Adds #[inline] on From impl. |
| libs/@local/hashql/mir/src/pass/execution/terminator_placement/mod.rs | Adds #[inline] on Default impl. |
| libs/@local/hashql/mir/src/pass/execution/splitting/mod.rs | Adds #[inline] on Default impl. |
| libs/@local/hashql/mir/src/pass/execution/mod.rs | Adds allocator-agnostic Debug for ExecutionAnalysisResidual. |
| libs/@local/hashql/mir/src/pass/execution/island/mod.rs | Adds #[inline] and Default-related tweaks. |
| libs/@local/hashql/mir/src/pass/execution/island/graph/mod.rs | Adds allocator-agnostic Debug for IslandGraph. |
| libs/@local/hashql/mir/src/pass/execution/cost/mod.rs | Adds #[inline] on From impl. |
| libs/@local/hashql/mir/src/pass/analysis/data_dependency/mod.rs | Adds #[inline] on Default impl. |
| libs/@local/hashql/mir/src/interpret/error.rs | Updates diagnostic alias documentation to match Critical default. |
| libs/@local/hashql/mir/src/body/operand.rs | Adds #[inline] on small From impls. |
| libs/@local/hashql/eval/src/orchestrator/error.rs | Updates diagnostic alias documentation to match Critical default. |
| libs/@local/hashql/core/src/graph/linked.rs | Adds allocator-agnostic Debug for LinkedGraph (removes derived Debug). |
| libs/@local/hashql/compiletest/src/suite/mir_reify.rs | Returns Scratch from mir_reify and passes scratch into ReifyContext. |
| libs/@local/hashql/compiletest/src/suite/mir_pass_transform_pre_inline.rs | Updates for new mir_reify return shape. |
| libs/@local/hashql/compiletest/src/suite/mir_pass_transform_cfg_simplify.rs | Reuses Scratch returned by mir_reify instead of creating a new one. |
| libs/@local/hashql/compiletest/src/suite/mir_pass_analysis_data_dependency.rs | Updates for new mir_reify return shape. |
| libs/@local/hashql/compiletest/src/suite/eval_postgres.rs | Minor fix to use heap consistently when passing allocator to eval context. |
| libs/@local/hashql/compiletest/src/pipeline.rs | Switches orchestration to use pass::lower / pass::place facades and threads scratch into reify. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
74f8c73 to
a203514
Compare
a203514 to
eb135ab
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 36 out of 36 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
libs/@local/hashql/compiletest/src/suite/mir_pass_transform_pre_inline.rs:175
- This suite ignores the
Scratchreturned bymir_reifyand then allocates a newScratchforPreInline. Sincemir_reifynow returns a resettable scratch allocator specifically to avoid extra scratch construction, this is doing redundant allocation/work here.
let (root, mut bodies, _) = mir_reify(heap, expr, interner, environment, diagnostics)?;
render.render(
&mut RenderContext {
heap,
eb135ab to
631682a
Compare
631682a to
b939729
Compare
9ab066d to
5bd5e93
Compare

🌟 What is the purpose of this PR?
This PR introduces two high-level pipeline functions,
pass::lowerandpass::place, that encapsulate the MIR lowering and execution placement pipelines respectively. Previously, callers had to manually orchestrate individual passes, manage scratch allocator resets, and collect diagnostics themselves. These new functions consolidate that logic into a single call site, making the pipeline easier to use correctly and harder to misuse.Additionally, this PR fixes a bug in the MIR reifier where a cached constructor reference was emitted as a bare
FnPtron cache hit, while the calling convention requires a fat pointer (closure aggregate). The fix ensures both the first and subsequent uses of a constructor emit aclosure(FnPtr, ())aggregate. This resolves the previously failingaccess-struct-through-opaqueinterpreter test, which now passes.The
ReifyContextis also extended with ascratchallocator parameter so that the reifier can use scratch memory for temporary allocations (e.g., theThunksvector), avoiding unnecessary heap allocations.🔍 What does this change?
pass::lowerto run the pre-inline, inline, and post-inline transform passes as a single operation, handling scratch resets and diagnostic collection internally.pass::placeto run size estimation and execution analysis as a single operation, returning theExecutionAnalysisResidualon success.LowerConfigas a configuration struct for the lowering pipeline, wrappingInlineConfig.ReifyContextwith a genericscratchallocator field (S: Allocator) so the reifier can allocate temporary structures (e.g.,Thunks::defs) from scratch memory rather than the heap.rvalue.rswhere the cache-hit path returned a bareFnPtrinstead of aclosureaggregate, causing downstream interpreter failures.mir_reifyin the compiletest suite to return theScratchallocator alongside the root and bodies, so callers that need scratch memory don't have to create a new one.Debugimplementations forLinkedGraph,IslandGraph, andExecutionAnalysisResidualthat do not require the allocator type parameter to implementDebug.ctor-cached-closure) covering the constructor caching fix.access-struct-through-opaquetest fromrun: failtorun: passnow that the underlying bug is resolved.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:
🛡 What tests cover this?
ctor-cached-closurereify UI test covering the constructor caching regression.access-struct-through-opaqueinterpreter UI test, now expected to pass with correct output.❓ How to test this?
access-struct-through-opaquepasses and produces the expectedOpaque(Outer { x: A, y: A })output.ctor-cached-closurepasses and produces the expected MIR withclosure(FnPtr, ())on both constructor uses.