Repository navigation
Conversation
Move internal canonicalization and match-pattern walkers out of TypedProgram without changing their order or behavior Preserve the public target-resolution API and add focused identity and source-span regressions Signed-off-by: hiyouwhatsapp <89997383+hiyouwhatsapp@users.noreply.github.com>
LunaStev
left a comment
There was a problem hiding this comment.
Reviewed exact head 79c6054 with Rust 1.89.0.
P1 — the extraction breaks compilation of numeric_checks
Reproduce from this PR:
cargo +1.89.0 test --locked --jobs 2 -p hir --test canonicalization_traversal -- --nocaptureThe crate fails before the new tests can run, with 14 compiler errors. Representative errors:
error[E0433]: failed to resolve: use of undeclared type StatementNode
--> front/hir/src/numeric_checks.rs:463:32
error[E0433]: failed to resolve: use of undeclared type HashSet
--> front/hir/src/numeric_checks.rs:511:35
error[E0425]: cannot find function walk_pattern in module super
--> front/hir/src/numeric_checks.rs:501:28
error[E0425]: cannot find function walk_node in module super
error[E0425]: cannot find function walk_expression in module super
numeric_checks imports super::* and still depends on parent-module imports of StatementNode, HashSet, walk_node and walk_expression. This patch removes those names from lib.rs. It also moves walk_pattern into traversal.rs as a private function, while numeric_checks still calls super::walk_pattern.
Make those dependencies explicit in the consuming module and restore the correct walker paths/visibility, or use equivalent narrowly scoped imports. Fix the inaccessible walk_pattern call as well as the removed type/import names; restoring only one import will not make the crate compile. Preserve traversal order, IDs, spans and numeric diagnostics. No further file split is needed.
Remaining formatting gate
The changed lib.rs and canonicalization_traversal.rs also fail cargo +1.89.0 fmt --all -- --check. Format the touched files after the compilation repair.
Then rerun the new traversal suite plus existing typed-HIR, alias, conversion, numeric and pattern-span coverage. The top-level moved function comparison preserved all 16 function bodies apart from the intended visibility changes, but that structural comparison does not cover the sibling-module dependencies above.
LunaStev
left a comment
There was a problem hiding this comment.
Re-reviewed updated head 7b5e27f.
The prior P1 compilation failure is fixed. The explicit StatementNode/HashSet/walker imports and traversal::walk_pattern visibility now compile correctly. I ran 31 focused tests successfully across the HIR library, canonicalization_traversal, typed_hir, phase1_frontend and float_literal_ranges suites.
The remaining blocker is the formatting gate. Full-workspace formatting on the exact source still exits 1, matching CI 41823816406.
cargo +1.89.0 fmt --all -- --checkRequired touched-file corrections include:
- front/hir/src/lib.rs:23: module ordering puts canonicalization before conversions.
- lib.rs:33: the traversal import follows the other grouped imports after formatting.
- front/hir/tests/canonicalization_traversal.rs:7,17,34,43,52: array/for layout, compact let-else blocks and the pattern/span iteration differ from pinned rustfmt.
Use the pinned formatter with the repository configuration on the touched files, then rerun the full check and focused HIR coverage. Preserve the now-fixed explicit dependencies, traversal order, IDs and numeric diagnostics.
The earlier compilation thread is addressed; this review requests the remaining formatting correction, not another extraction or a rewrite of the HIR design.
|
Remaining changes before merge — reviewed head The previous 14 compilation errors are fixed, and 31 focused HIR tests passed. Please finish formatting before merging. Remaining differences include front/hir/src/lib.rs module/import ordering and canonicalization_traversal.rs array, let-else and loop layouts. Apply the pinned formatter and verify the full check: rustfmt +1.89.0 --edition 2021 --config-path rustfmt.toml front/hir/src/lib.rs front/hir/tests/canonicalization_traversal.rs
cargo +1.89.0 fmt --all -- --check
cargo +1.89.0 test --locked --jobs 2 -p hir --lib --test canonicalization_traversal --test typed_hir --test phase1_frontend --test float_literal_rangesPreserve the now-correct explicit numeric_checks imports, walker paths/visibility, traversal order and source spans. No further extraction or redesign is requested. Detailed findings and reproduction: review. |
LunaStev
left a comment
There was a problem hiding this comment.
Reviewed head 368fcbbf33c654693a34111e526665057a5f203f.
The previous compilation and formatting requests are fully addressed. The change since the last tested head is formatter output only; the corrected explicit numeric checker imports and traversal visibility remain intact. The pinned full Cargo formatting check now passes locally. The previous 31 focused HIR tests passed, and exact-head CI 42057714165 is completed/success with all 50 distinct required jobs successful, including native/package lanes.
No remaining actionable finding in this patch. Approved for normal squash merge at this exact head.
Fixes #770
TypedProgram kept backend-boundary type canonicalization and match-pattern traversal in the same file as program storage and expression identities
I moved canonicalization and target-width resolution into one internal module, and pattern traversal into another
Kept the public resolve_target_types API, exact traversal order and the timing of canonicalization after semantic analysis unchanged
Added regression tests for both target pointer widths and for nested pattern IDs and source spans
Existing alias, generic, variant and conversion tests remain in place
This touches canonicalization also changed by #1074 and may need a rebase if that lands first
Haven't run the Rust suite locally, CI still needs to verify it