Skip to content

BE-581, BE-589: HashQL: Introduce pass::lower/pass::place pipeline facades and fix cached constructor closure bug#8838

Open
indietyp wants to merge 5 commits into
bm/be-585-hashql-split-mir-interpret-diagnostic-apifrom
bm/be-581-hashql-cached-constructor-produces-bare-fnptr-instead-of
Open

BE-581, BE-589: HashQL: Introduce pass::lower/pass::place pipeline facades and fix cached constructor closure bug#8838
indietyp wants to merge 5 commits into
bm/be-585-hashql-split-mir-interpret-diagnostic-apifrom
bm/be-581-hashql-cached-constructor-produces-bare-fnptr-instead-of

Conversation

@indietyp

@indietyp indietyp commented Jun 8, 2026

Copy link
Copy Markdown
Member

🌟 What is the purpose of this PR?

This PR introduces two high-level pipeline functions, pass::lower and pass::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 FnPtr on 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 a closure(FnPtr, ()) aggregate. This resolves the previously failing access-struct-through-opaque interpreter test, which now passes.

The ReifyContext is also extended with a scratch allocator parameter so that the reifier can use scratch memory for temporary allocations (e.g., the Thunks vector), avoiding unnecessary heap allocations.

🔍 What does this change?

  • Introduces pass::lower to run the pre-inline, inline, and post-inline transform passes as a single operation, handling scratch resets and diagnostic collection internally.
  • Introduces pass::place to run size estimation and execution analysis as a single operation, returning the ExecutionAnalysisResidual on success.
  • Adds LowerConfig as a configuration struct for the lowering pipeline, wrapping InlineConfig.
  • Extends ReifyContext with a generic scratch allocator field (S: Allocator) so the reifier can allocate temporary structures (e.g., Thunks::defs) from scratch memory rather than the heap.
  • Fixes the constructor caching bug in rvalue.rs where the cache-hit path returned a bare FnPtr instead of a closure aggregate, causing downstream interpreter failures.
  • Updates mir_reify in the compiletest suite to return the Scratch allocator alongside the root and bodies, so callers that need scratch memory don't have to create a new one.
  • Adds Debug implementations for LinkedGraph, IslandGraph, and ExecutionAnalysisResidual that do not require the allocator type parameter to implement Debug.
  • Adds a regression test (ctor-cached-closure) covering the constructor caching fix.
  • Updates the access-struct-through-opaque test from run: fail to run: pass now that the underlying bug is resolved.

Pre-Merge Checklist 🚀

🚢 Has this modified a publishable library?

This PR:

  • does not modify any publishable blocks or libraries, or modifications do not need publishing

📜 Does this require a change to the docs?

The changes in this PR:

  • are internal and do not require a docs change

🕸️ Does this require a change to the Turbo Graph?

The changes in this PR:

  • do not affect the execution graph

🛡 What tests cover this?

  • New ctor-cached-closure reify UI test covering the constructor caching regression.
  • Updated access-struct-through-opaque interpreter UI test, now expected to pass with correct output.
  • Existing MIR pass and reify UI tests continue to pass with the refactored pipeline.

❓ How to test this?

  1. Run the MIR compiletest suite.
  2. Confirm access-struct-through-opaque passes and produces the expected Opaque(Outer { x: A, y: A }) output.
  3. Confirm ctor-cached-closure passes and produces the expected MIR with closure(FnPtr, ()) on both constructor uses.

indietyp added 2 commits June 8, 2026 12:23
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
Copilot AI review requested due to automatic review settings June 8, 2026 11:41
@cursor

cursor Bot commented Jun 8, 2026

Copy link
Copy Markdown

PR Summary

Medium Risk
Changes MIR reification calling convention and centralizes the optimization/placement pipeline; the ctor fix is correctness-critical but covered by new and updated UI tests.

Overview
Adds pass::lower and pass::place in the MIR crate so lowering (pre-inline → inline → post-inline) and execution placement (size estimation → execution analysis) run as single calls with scratch resets and diagnostic handling, instead of each caller wiring passes by hand. The compiletest Pipeline now uses these facades for transform and prepare.

ReifyContext gains a scratch allocator; mir_reify creates it, passes it through reification, and returns it with the bodies so suites can reuse scratch memory. Reifier/thunk maps are parameterized over allocators where needed.

Fixes MIR reification when a type constructor is reused from cache: the cache-hit path now emits a closure(FnPtr, ()) aggregate like the miss path, instead of a bare FnPtr. That fixes interpreter struct projection on nested newtypes (access-struct-through-opaque now passes) and is covered by ctor-cached-closure.

Smaller follow-ons: custom Debug for LinkedGraph, IslandGraph, and ExecutionAnalysisResidual without requiring the allocator to implement Debug; eval_postgres passes heap into EvalContext; assorted #[inline] on hot paths.

Reviewed by Cursor Bugbot for commit ce89d48. Bugbot is set up for automated code reviews on this repo. Configure here.

@vercel

vercel Bot commented Jun 8, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
hash Ready Ready Preview, Comment Jun 12, 2026 12:41pm
petrinaut Ready Ready Preview Jun 12, 2026 12:41pm
1 Skipped Deployment
Project Deployment Actions Updated (UTC)
hashdotdesign-tokens Ignored Ignored Preview Jun 12, 2026 12:41pm

@codecov

codecov Bot commented Jun 8, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 16.00000% with 63 lines in your changes missing coverage. Please review.
✅ Project coverage is 59.52%. Comparing base (ee4ae89) to head (ce89d48).

Files with missing lines Patch % Lines
libs/@local/hashql/mir/src/pass/mod.rs 0.00% 44 Missing ⚠️
.../hashql/mir/src/pass/execution/island/graph/mod.rs 0.00% 7 Missing ⚠️
libs/@local/hashql/core/src/graph/linked.rs 0.00% 6 Missing ⚠️
libs/@local/hashql/mir/src/pass/execution/mod.rs 0.00% 6 Missing ⚠️
Additional details and impacted files
@@                                   Coverage Diff                                   @@
##           bm/be-585-hashql-split-mir-interpret-diagnostic-api    #8838      +/-   ##
=======================================================================================
+ Coverage                                                59.50%   59.52%   +0.02%     
=======================================================================================
  Files                                                     1347     1347              
  Lines                                                   130804   130990     +186     
  Branches                                                  5879     5883       +4     
=======================================================================================
+ Hits                                                     77838    77978     +140     
- Misses                                                   52063    52105      +42     
- Partials                                                   903      907       +4     
Flag Coverage Δ
rust.hash-graph-api 2.52% <ø> (ø)
rust.hashql-core 79.77% <0.00%> (+0.18%) ⬆️
rust.hashql-eval 75.20% <ø> (ø)
rust.hashql-mir 88.04% <17.39%> (-0.41%) ⬇️
rust.hashql-syntax-jexpr 94.04% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@indietyp indietyp changed the title BE-581: HashQL: Introduce pass::lower/pass::place pipeline facades and fix cached constructor closure bug BE-581, BE-589: HashQL: Introduce pass::lower/pass::place pipeline facades and fix cached constructor closure bug Jun 8, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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::lower and pass::place pipeline facades (plus LowerConfig) 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.

Comment thread libs/@local/hashql/compiletest/src/suite/mir_reify.rs
@codspeed-hq

codspeed-hq Bot commented Jun 8, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

⚡ 2 improved benchmarks
❌ 3 regressed benchmarks
✅ 75 untouched benchmarks

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Benchmark BASE HEAD Efficiency
linear 6.8 µs 7.8 µs -12.67%
diamond 10.9 µs 12.3 µs -11.76%
complex 16.2 µs 18.2 µs -10.6%
bit_matrix/dense/iter_row[64] 170 ns 140.8 ns +20.71%
bit_matrix/dense/iter_row[200] 215 ns 185.8 ns +15.7%

Tip

Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.


Comparing bm/be-581-hashql-cached-constructor-produces-bare-fnptr-instead-of (ce89d48) with bm/be-585-hashql-split-mir-interpret-diagnostic-api (ee4ae89)

Open in CodSpeed

@indietyp indietyp force-pushed the bm/be-581-hashql-cached-constructor-produces-bare-fnptr-instead-of branch from 74f8c73 to a203514 Compare June 8, 2026 11:55
@indietyp indietyp force-pushed the bm/be-581-hashql-cached-constructor-produces-bare-fnptr-instead-of branch from a203514 to eb135ab Compare June 8, 2026 12:22

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Scratch returned by mir_reify and then allocates a new Scratch for PreInline. Since mir_reify now 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,

Comment thread libs/@local/hashql/mir/src/pass/mod.rs
@indietyp indietyp force-pushed the bm/be-581-hashql-cached-constructor-produces-bare-fnptr-instead-of branch from eb135ab to 631682a Compare June 8, 2026 13:05
Copilot AI review requested due to automatic review settings June 9, 2026 07:17
@indietyp indietyp force-pushed the bm/be-581-hashql-cached-constructor-produces-bare-fnptr-instead-of branch from 631682a to b939729 Compare June 9, 2026 07:17
@indietyp indietyp force-pushed the bm/be-585-hashql-split-mir-interpret-diagnostic-api branch from 9ab066d to 5bd5e93 Compare June 9, 2026 07:17

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 36 out of 36 changed files in this pull request and generated no new comments.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 36 out of 36 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

libs/@local/hashql/compiletest/src/pipeline.rs:191

  • ReifyContext now uses scratch for temporary allocations during reify::from_hir (e.g. Thunks::defs). In this staged pipeline, Pipeline::lower doesn’t reset self.scratch after reification, so repeated calls to lower() without subsequently calling transform()/prepare() can cause scratch memory to grow unnecessarily.

Resetting the scratch allocator immediately after from_hir keeps the stage self-contained and matches the intent of scratch allocations being temporary.

        let entry = tri!(hashql_mir::reify::from_hir(node, &mut reify_context));

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/libs Relates to first-party libraries/crates/packages (area) area/tests New or updated tests type/eng > backend Owned by the @backend team

Development

Successfully merging this pull request may close these issues.

2 participants