Delete Bs_ast_mapper, keeping Ast_mapper at upstream shape - #8578
Open
cristianoc wants to merge 1 commit into
Open
Delete Bs_ast_mapper, keeping Ast_mapper at upstream shape#8578cristianoc wants to merge 1 commit into
cristianoc wants to merge 1 commit into
Conversation
Bs_ast_mapper was a 600-line mechanical fork of the vendored Ast_mapper
whose entire delta - still delimited by fossilized cppo (* #if *)
comments - was three group-level hooks: type_declaration_list,
value_bindings, and value_bindings_rec. Only value_bindings ever had a
client (tuple/module-record pattern flattening for non-recursive lets);
the other two were never overridden by anything. Meanwhile the fork had
drifted: it dropped ~loc/~attrs when rebuilding JSX fragments and did
not remap JSX element-name locations. Every parsetree change paid the
duplication tax twice.
Rather than folding hooks into Ast_mapper, keep it byte-identical to
its upstream shape and move the one real behavior into the client:
bs_builtin_ppx's default_expr_mapper is now a thin wrapper that
flattens non-recursive Pexp_let groups before delegating, so every
fallback in expr_mapper inherits the flattening uniformly (hello_res's
nested [let {length: l, add} = module(List)] reaches it only through a
fallback and pins this), and structure_item_mapper handles
Pstr_value(Nonrecursive) the same way. Recursive groups map per
binding - their patterns are restricted to variables, so group
rewrites do not apply.
Also: inline Ast_typ_uncurry's one function into its one consumer and
delete the module; replace ast_external_mk's hand-rolled expression,
structure, and module-expr records with Ast_helper constructors;
replace ast_tdcls's two dead-hook accessor calls with plain List.map.
Generated output is byte-identical across the test suite.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Cristiano Calcagno <cristianoc@users.noreply.github.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0a608f64ad
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #8578 +/- ##
==========================================
- Coverage 75.95% 75.93% -0.02%
==========================================
Files 474 472 -2
Lines 62905 62608 -297
==========================================
- Hits 47779 47541 -238
+ Misses 15126 15067 -59
🚀 New features to boost your workflow:
|
rescript
@rescript/belt
@rescript/darwin-arm64
@rescript/darwin-x64
@rescript/linux-arm64
@rescript/linux-x64
@rescript/runtime
@rescript/win32-x64
commit: |
|
Developer playground preview: https://rescript-lang.github.io/rescript/dev-playground/?version=pr-8578 |
cknitt
approved these changes
Aug 25, 2026
cknitt
left a comment
cknitt
left a comment
Member
There was a problem hiding this comment.
Nice! There is a merge conflict now though.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up cleanup from the n-ary functions series (#8557); the frontend-veneer sweep it deferred.
What
Bs_ast_mapperwas a 600-line mechanical fork of the vendoredAst_mapperwhose entire delta — still delimited by fossilized cppo(* #if *)comments — was three group-level hooks:type_declaration_list,value_bindings,value_bindings_rec. Onlyvalue_bindingsever had a client (tuple/module-record pattern flattening for non-recursivelets); the other two were never overridden by anything. Meanwhile the fork had drifted: it dropped~loc/~attrswhen rebuilding JSX fragments and did not remap JSX element-name locations. Every parsetree change paid the duplication tax twice (three times in August alone during the n-ary series).Rather than folding hooks into
Ast_mapper, this keeps it byte-identical to its upstream shape and moves the one real behavior into the client:bs_builtin_ppx'sdefault_expr_mapperis now a thin wrapper that flattens non-recursivePexp_letgroups before delegating, so every fallback inexpr_mapperinherits the flattening uniformly, andstructure_item_mapperhandlesPstr_value (Nonrecursive, _)the same way. Recursive groups map per binding — their patterns are restricted to variables (Illegal_letrec_pat), so group rewrites do not apply.The fallback-uniformity point is load-bearing and corpus-guarded:
hello_res.res's nestedlet {length: l, add} = module(List)reaches the flattener only through a specialized case's fallback — an intermediate version of this change that flattened in a match case instead failed exactly there.Riders
Ast_typ_uncurry(one function, one consumer, curried-era name) inlined intoast_core_type_class_typeand deleted.ast_external_mk's four hand-rolled expression/structure/module-expr record towers replaced withAst_helperconstructors.ast_tdcls's two dead-hook accessor calls replaced with plainList.map.Verification
Generated output is byte-identical across the full test suite (
make test, analysis suites, zero snapshot or expectation changes); formatting green. Not user-facing, so no CHANGELOG entry.🤖 Generated with Claude Code