Skip to content

BE-71: HashQL: Consolidate AST special-form expansion stages into one#8870

Open
indietyp wants to merge 55 commits into
bm/be-481-hashql-lower-null-to-constantunit-during-reificationfrom
bm/be-71-hashql-consider-merging-ast-stages
Open

BE-71: HashQL: Consolidate AST special-form expansion stages into one#8870
indietyp wants to merge 55 commits into
bm/be-481-hashql-lower-null-to-constantunit-during-reificationfrom
bm/be-71-hashql-consider-merging-ast-stages

Conversation

@indietyp

@indietyp indietyp commented Jun 16, 2026

Copy link
Copy Markdown
Member

🌟 What is the purpose of this PR?

🏗️ This PR is large because it replaces three tightly coupled AST lowering passes with a single merged pass. Line breakdown: production code is +6295/-5451 (119 files); tests and snapshots are +2893/-2756 (692 files, mostly mechanical annotation updates due to changed diagnostic messages). The old passes (PreExpansionNameResolver, SpecialFormExpander, ImportResolver) share resolution state that the new Expander unifies, and the core module changes (universe-aware errors, KindSet, module depth, namespace .alias()) only exist to support the new pass. Splitting the core changes out would require backporting them to the old passes just to keep CI green before deleting those passes in the next PR. It is one logical change: merge three passes into one.

Replaces three AST lowering passes with a single Expander pass that resolves names, expands special forms, and resolves imports in one top-down traversal. This is the main deliverable of BE-71.

The old pipeline ran PreExpansionNameResolver, SpecialFormExpander, and ImportResolver sequentially, each walking the full AST. The new Expander does all three in a single walk, eliminating redundant traversals and making error recovery more coherent: diagnostics from nested expressions are no longer suppressed by early bail-outs in outer forms.

🔍 What does this change?

New Expander pass (hashql-ast/src/lower/expander/):

  • Single visitor that resolves paths, expands special forms (let, type, newtype, fn, use, as, if, input, index, access), and resolves imports
  • Each special form in its own file with a lower_* entry point
  • visit() method encapsulates save/restore/trampoline cycle, returns Option<CurrentItem>
  • CurrentItem { item, has_arguments } prevents aliasing of specialised references and guards intrinsic swallowing
  • Zero-allocation path rewriting via rotate_right/rotate_left + zip
  • Recovery over abort: invalid binding names use Ident::synthetic(sym::dummy) and continue collecting downstream diagnostics, returning Expr::dummy() only at the final gate
  • 39 diagnostic categories with per-variant constructors, S-expression syntax in messages, three-tier spelling suggestions, cross-universe hints ("exists as a type, not a value")

Module system changes (hashql-core):

  • KindSet bitfield (VALUE | TYPE | MODULE) for cross-universe error reporting
  • ItemNotFound and ImportNotFound enriched with expected: Option<Universe> and found: KindSet
  • Resolver scans for alternate-kind items when suggestions are enabled
  • resolve_relative produces ModuleRequired instead of ModuleNotFound when a non-module import with the same name exists
  • Module::depth as NonZero<u32> for ExactSizeIterator on absolute_path_rev
  • ModuleNamespace::alias() for identity-preserving rebinding

Removed:

  • PreExpansionNameResolver (~437 lines)
  • SpecialFormExpander (~1451 lines + ~1125 lines of diagnostics)
  • ImportResolver (~478 lines + ~712 lines of diagnostics)

Other:

  • lowering module renamed to lower
  • Compiletest suites rewired for the merged pass
  • Path truncation support for future re-exports (canonical path shorter than written path)

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

⚠️ Known issues

  • Path truncation for re-exports is implemented but untested: the intern_module debug assertion (item.module == containing_module) prevents constructing a re-export scenario in tests. Will be testable when re-export infrastructure lands.
  • Ambiguous, ModuleEmpty, and InvalidQueryLength resolution error paths are unreachable from the expander (single-universe resolution, no empty modules in stdlib, parser never produces empty paths). Diagnostic constructors exist for completeness but have no compiletest coverage.

🐾 Next steps

  • Diagnostic quality review pass
  • Re-export module infrastructure (would enable testing path truncation)

🛡 What tests cover this?

  • 557 compiletest UI tests, including 24 new tests for the expander:
    • Labeled argument rejection for all 9 special forms
    • Invalid binding names, invalid field access, invalid use paths
    • Intrinsic swallowing (let and type rebinding intrinsics)
    • Intrinsic type annotation errors
    • Generic arguments on intrinsics (both type and special form)
    • Cross-universe resolution ("cannot find value X" / "cannot find type X")
    • Duplicate generic constraints and duplicate use bindings
    • Package-not-found for nonexistent root packages
    • Multiple generic arguments on module path segments
  • All existing inherited compiletest tests from the old passes continue to pass under the new expander suite

❓ How to test this?

cargo run -p hashql-compiletest --all-features -- run

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 2 potential issues.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 0c15b58. Configure here.

Comment thread libs/@local/hashql/ast/src/lower/expander/mod.rs
Comment thread libs/@local/hashql/ast/src/lower/expander/type.rs

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.

Copilot wasn't able to review this pull request because it exceeds the maximum number of files (300). Try reducing the number of changed files and requesting a review from Copilot again.

@vercel vercel Bot temporarily deployed to Preview – petrinaut June 19, 2026 15:24 Inactive
Copilot AI review requested due to automatic review settings June 22, 2026 10:51
@indietyp indietyp force-pushed the bm/be-481-hashql-lower-null-to-constantunit-during-reification branch from 942f634 to 3ab5811 Compare June 22, 2026 10:51
@indietyp indietyp force-pushed the bm/be-71-hashql-consider-merging-ast-stages branch from 2d4919c to 1f33b16 Compare June 22, 2026 10:51

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.

Copilot wasn't able to review this pull request because it exceeds the maximum number of files (300). Try reducing the number of changed files and requesting a review from Copilot again.

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.

Copilot wasn't able to review this pull request because it exceeds the maximum number of files (300). Try reducing the number of changed files and requesting a review from Copilot again.

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

Labels

area/deps Relates to third-party dependencies (area) area/infra Relates to version control, CI, CD or IaC (area) 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