Repository navigation
Conversation
print_mod_expr never printed pmod_attributes, so the formatter dropped attributes such as `module M = @attr F(X)`, `module M = @attr {}` and `include @attr F(X)`. Print them before the module expression, except on functors, whose attributes are already printed on their first parameter. Two placements need care to round-trip: - An attributed functor in an application is parenthesized, `(@attr F)(X)`, and an attributed inner application is no longer flattened into the outer one, `(@attr F(A))(B)`; otherwise the attribute would move to the whole application. - `await` is printed before the attributes, since `@attr await M` does not parse. Signed-off-by: Christoph Knittel <christoph@knittel.cc> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Christoph Knittel <christoph@knittel.cc> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #8735 +/- ##
==========================================
+ Coverage 79.86% 79.87% +0.01%
==========================================
Files 464 464
Lines 63090 63127 +37
==========================================
+ Hits 50388 50424 +36
- Misses 12702 12703 +1
🚀 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-8735 |
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 7d925133e2
ℹ️ 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".
An attribute on a constraint was printed without parens, so it moved to the constrained module on reparse (`F(@attr (X: S))` printed as `F(@attr X: S)`, and likewise for `include`). On the right-hand side of a module binding it was dropped, because `module M = @attr (X: S)` was printed as `module M: S = X`. Parenthesize attributed constraints, keep them on the right-hand side of a binding, and don't add a second pair of parens after `include`. Signed-off-by: Christoph Knittel <christoph@knittel.cc> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Use the existing Parsetree_viewer.has_printable_attributes instead of filtering the attributes and comparing with [], and share the rule that an attributed module constraint prints its own parens between the printer and Parens.include_mod_expr. Signed-off-by: Christoph Knittel <christoph@knittel.cc> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 77e9cb704e
ℹ️ 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".
`await @attr (X: S)` printed as `await (@attr X: S)`, moving the attribute from the constraint to X: await's parens enclosed the attributes but not the constraint itself. An attributed constraint now always prints its own parens, and await only adds parens around an unattributed one. Signed-off-by: Christoph Knittel <christoph@knittel.cc> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8329512020
ℹ️ 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".
Besides print_mod_expr, several printer paths print or take apart a module expression on their own, and each dropped or moved attributes and await, or printed code that doesn't parse: - a module constraint was hoisted into a local module's binding (`module M = @attr (X: S)` lost @attr) and a functor's result signature, the latter also dropping await - the `include F({type t = ...})` shortcut ignored all attributes - application flattening and callee parens ignored await, so `(await F(A))(B)` printed as `F(A, B)` and `(await F)(A)` as `await F(A)` - nested functors were merged through an awaited inner functor - `F(@attr {})` printed as `F()` - applied constraints, functors and extensions lost their parens, as did constraints after `module M: T =`, `module type of` and inside `module(...)` Found by formatting and re-parsing every combination of 15 contexts and about 450 module expressions, comparing parsetrees. Signed-off-by: Christoph Knittel <christoph@knittel.cc> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
cknitt
changed the title
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fba1d121ce
ℹ️ 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".
- `mod_expr_has_attributes` now uses the same filter as `print_attributes`. It used `has_printable_attributes`, which also excludes `@JSX`, so `(@jsx F)(A)` printed as `@JSX F(A)` and `(@jsx F(A))(B)` lost the attribute. - A functor argument whose printed form starts with a doc comment is parenthesized, since the parser doesn't accept a doc comment there. - A constraint now prints its own parens whenever it has attributes or `await`, which replaces `Parens.attributed_mod_constraint` and the constraint case of `await`. - `print_mod_expr_constraint_parens` covers what `Parens.mod_expr_parens` did for module bindings, so that is removed. Signed-off-by: Christoph Knittel <christoph@knittel.cc> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Removing it changed how existing code is formatted: after a signature holding a single module, `} = (M')` lost its parens and printed as `} = M'`. Both parse the same, but this PR shouldn't reformat existing code (tests/tests/src/coercion_module_alias_test.res failed the format check). Signed-off-by: Christoph Knittel <christoph@knittel.cc> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This branch has not been deployed
1 participant
The formatter dropped or moved attributes and
awaiton module expressions, and printed code that doesn't parse for module constraints and functors in some positions.print_mod_exprnever printedpmod_attributes, and several other printer paths print or take apart a module expression on their own:module M = @attr F({})module M = F()module M = (@attr F)({})module M = F()module M = @attr (X: S)module M: S = XF(@attr (X: S))F(@attr X: S)(attribute moves toX)module M = @attr (X: S)module M: S = X(X) => @attr (Y: S)(_: X): S => Y(Z: T) => await (X: S)(Z: T): S => X(await dropped)include @attr F({type t = int})include F({type t = int})(await F(A))(B)F(A, B)(await dropped)(await F)(A)await F(A)H(@attr {})H()module M: T = (X: S)module M: T = X: S(doesn't parse)((X: S))(Z),((Y: S) => {})(Z),(%ext)(Z)module type of (X: S),module((X: S))Found while adding a fixture for #8734:
module M = @inlined F({})couldn't be kept in a formatted test file.Changes
Parsetree_viewer.mod_expr_has_attributes: the attributesprint_attributesprints, orawait. It guards every place that hoists a constraint (module bindings, local modules, functor results), flattens applications or merges nested functors, so those only apply to unannotated nodes.awaitprints its own parens:@attr (X: S),await @attr (X: S). Without them, the position decides: aftermodule M: T =, in a functor's result, aftermodule type ofand insidemodule(...).- Applied functors are parenthesized when they're a constraint, a functor, an extension, or carry attributes orawait.include F({type t = ...})shortcut and theF()shorthand only apply without attributes.(M: {...}), like the existing binding parens.No existing syntax snapshot changes.
How it was checked
Beyond the syntax tests, I formatted every combination of 15 contexts (bindings,
module rec, local modules,include, functor bodies and arguments, applied functors,module(...),module type of,await) and about 450 module expressions with attributes andawaiton different nodes, nested two levels deep: 6750 cases. For each, the reparsed tree must equal the original and formatting must be idempotent.@JSX,@attr("payload"),@a.band a doc comment in place of@attr.awaitwritten twice on one module,await @w (await X), which the parser stores as twores.awaiton the same node. That can't type-check and is left alone.Tests
printer/modExpr/attributes.res: attributes andawaiton each kind of module expression and in each of the positions above.printer/modExpr/parens.res: unattributed constraints and functors in positions that need parens.🤖 Generated with Claude Code