Repository navigation
Represent await on module expressions with a Pmod_await node - #8738
Conversation
Signed-off-by: Christoph Knittel <christoph@knittel.cc> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@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. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #8738 +/- ##
==========================================
+ Coverage 79.88% 79.97% +0.09%
==========================================
Files 464 464
Lines 63126 63236 +110
==========================================
+ Hits 50428 50573 +145
+ Misses 12698 12663 -35
🚀 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: |
![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: 27a3592c97
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 51c9a0dfaa
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 148ba90d12
ℹ️ 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".
148ba90 to
52ca529
Compare
`await M` on a module expression was stored as a `res.await` attribute on M, while expressions already have `Pexp_await`. Add `Pmod_await` and have the parser produce it, so `res.await` only remains in the v0 PPX wire representation: `Ast_mapper_to0` writes it like for `Pexp_await` (the await node's attributes, the marker, then the inner module's) and `Ast_mapper_from0` splits them again. The frontend recognizes awaited module paths structurally (`Ast_await.awaited_module_path`) for the dynamic import rewrite and the async context check. Any other `await` on a module still has no effect: the type checker types `Pmod_await M` as `M`. Errors on an awaited module now point at the whole `await M`. The printer no longer special-cases a `res.await` attribute: `await` is printed by its own node, which also lets `await @w (await X)` and `@w (await X)` round-trip. Formatting of existing code is unchanged. Bump the current AST magic numbers for the new constructor. 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>
An await node's own attributes print before it, so a doc comment on it starts the printed argument: F((/** doc */ (await X))) printed as F(/** doc */ (await X)), which doesn't parse. Only a functor's attributes print elsewhere (on its first parameter). Signed-off-by: Christoph Knittel <christoph@knittel.cc> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The v0 bridge folds an await node and its operand into one node, marked with `res.await`. The marker was written without a location, so after a round trip through an external PPX the await node took its operand's location and lost the span of the `await` keyword. Store the await node's location on the marker, like `res.braces` does, and restore it, falling back to the node's location for markers without one. This also applies to `Pexp_await`, which had the same problem. The dynamic import rewrite replaced an await node with its operand and dropped the await node's attributes, e.g. a `@warning` in `(@warning("-3") (await List): ListT)` as a PPX may produce it. Keep them on the imported module, as when await was an attribute. Signed-off-by: Christoph Knittel <christoph@knittel.cc> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`module M = await (await List)` used to be a dynamic import: both awaits became `res.await` attributes on the same node. With `Pmod_await` it is two nested nodes, and the dynamic import forms only looked through one, so it silently became a static module reference and skipped the async context check. `Ast_await.awaited_module_path` now looks through any number of awaits around the module path and around the constraint, and the toplevel and local module type pre-scans use it too. `create_await_module_expression` removes nested awaits as well. Compiling 19 module await forms in 5 contexts gives the same JavaScript and errors as master. Signed-off-by: Christoph Knittel <christoph@knittel.cc> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
52ca529 to
b599968
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b599968499
ℹ️ 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".
|
Developer playground preview: https://rescript-lang.github.io/rescript/dev-playground/?version=pr-8738 |
`awaited_module_path` and `create_await_module_expression` each removed the awaits around a dynamic import, the second time keeping their attributes. `awaited_module_path` now returns the module to import with its awaits removed and their attributes kept, and `create_await_module_expression` just packs it. The toplevel and local module cases each declared `module type __List__ = module type of List` once per module with the same code; it is now `local_module_type_decl`. Both callers bind its result before mapping the rest, since a later import of the same module must find it already declared. Signed-off-by: Christoph Knittel <christoph@knittel.cc> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`type_module_type_of` looked a module path up without the warning scope of the module's attributes, unlike `type_module`, so `module type of @warning("-3") DeprecatedModule` still reported warning 3. That includes the `module type __M__ = module type of M` a dynamic import declares, so `module M = @warning("-3") (await DeprecatedModule)` reported it too (also before `Pmod_await`). Look the module up in that scope. 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: 8d52a7a708
ℹ️ 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".
Outside the dynamic import forms, `await` on a module has no effect; as
a `res.await` attribute it was invisible to the type checker. As a
`Pmod_await` node it hid the module's shape where the type checker
treats shapes specially: `module type of await M` was strengthened
(abstract types became `M.t`) instead of taking the unstrengthened
module path, and `F(await {})` was rejected for a generative functor.
Like `Pexp_await`, which never reaches the type checker, the builtin
ppx now removes the remaining awaits after the dynamic import rewrite,
keeping their attributes on the module they wrap. The type checker's
`Pmod_await` case only remains for trees that bypass it.
Type checking every input of the module expression round-trip harness
(7950 cases) gives the same exit codes, JavaScript and errors as master.
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>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 29f7bc5bff
ℹ️ 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".
PPXs saw await on a module as the res.await attribute, after the module's own attributes and before those written after await or around the await: [await @b (@A M)] was [@A; res.await; @b]. The attributes after await now belong to the Pmod_await node, as those of [@b (await M)] already did, and the v0 bridge emits them in that order. The printer prints an await's attributes after await and parenthesizes an attributed operand. Signed-off-by: Christoph Knittel <christoph@knittel.cc> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
The trailing underscore in Ast_helper is for OCaml keywords, as in Mod.functor_ and Mod.constraint_; await isn't one, and the expression helper is Exp.await. Signed-off-by: Christoph Knittel <christoph@knittel.cc> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2 participants
Resolves the "
res.awaiton module expressions (await M)" item of #8624, and #7383.await Mon a module expression was stored as ares.awaitattribute onM, while expressions have had aPexp_awaitnode since #7368 (whose changelog entry noted that the attribute was still used for modules). This addsPmod_await of module_expr, sores.awaitnow only appears in the v0 PPX wire representation.Changes
Parsetree:
Pmod_await, withAst_helper.Mod.await, the mapper and iterator,printast,pprintast,dependand the syntax debugger. The current AST magic numbers are bumped.Parser: produces
Pmod_await; its location includes theawaitkeyword. The attributes written afterawaitbelong to the await node, like those of@w (await M), and the module keeps its own:await @b (@a M).v0 bridge: PPXs see the same attributes, in the same order, as with the old attribute encoding.
Ast_mapper_to0writes the inner module's attributes, then theres.awaitmarker (carrying the await's location), then the await node's attributes, e.g.[@a; res.await; @b]forawait @b (@a M);Ast_mapper_from0splits them at the last marker.Frontend:
Ast_await.awaited_module_pathrecognizesawait M,await (M: S)and(await M: S)structurally, also with repeated awaits. It's used for the dynamic import rewrite and for the "not in an async context" check.Ast_await.is_awaitandAst_attributes.has_await_payloadare removed; they also accepted a user-written@awaitattribute for that check, which no longer means anything.Builtin ppx: like
Pexp_await,Pmod_awaitdoesn't reach the type checker. After the dynamic import rewrite, any otherawaitis removed, with its attributes kept on the module it wraps, after the module's own. That keeps the current behavior: such anawaithas no effect, e.g.module M = await F(X)is justF(X), and the type checker sees the same trees as on master. Making that an error would be a separate, user-visible change.Type checker:
module type ofnow looks up its module path inside the scope of the module's@warningattributes, somodule type of @warning("-3") Oldand a dynamic import of a deprecated module with@warning("-3")no longer report warning 3 (CHANGELOG entry).Printer:
awaitis printed by its own node instead of being special-cased among the attributes, removing most of theawaithandling Fix module expressions losing attributes, await or parens when formatting #8735 needed:await @w M, with the await node's attributes afterawait, and parens around an attributed, constrained or awaited operand:await @w (@a M),await (await M);(await F)(X)when applied.await (@a X)andawait @w (await X), which Fix module expressions losing attributes, await or parens when formatting #8735 could not round-trip because they were stored on one node, now print as written.Behavior
awaitforms and 5 contexts (top-level and local, sync and async) gives the same JavaScript and the same errors as master; this covers dynamic imports, the async-context error and the no-op cases. The only difference is that errors on an awaited module now point at the wholeawait Mrather than atM..res/.resifiles gives the same output as master.Tests
ast-mapping/ModuleAwait.res: everyawaitform, with attributes on the await node and on the inner module, and a nestedawait. The parsetree after the v0 round trip is identical to the original.printer/modExpr/attributes.res:@w (await X),await @w (await X)and an awaited structure as a functor argument.expressions/await.resnow shows theawaitnode.ounit_ast_mapper0_tests.ml: the v0 attribute order for parsed sources, the await location through the v0 round trip, a v0 marker without a location, and nested awaits as dynamic imports.super_errors/warning_3_module_type_of_scope.res:@warning("-3")onmodule type ofand on a dynamic import.tests/tests/src/module_await_noop.res:module type of await Mdoesn't strengthen, andG(await {})applies a generative functor.🤖 Generated with Claude Code