Mark return expressions in parsed and typed ASTs #7898
Conversation
Signed-off-by: Cristiano Calcagno <cristianoc@users.noreply.github.com>
|
@zth I had to write the prompt AND the patch—next time try not to outsource the entire job to your toaster. |
|
The magic prompt: 
|
![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 suggestions.
Reply with @codex fix comments to fix any unresolved comments.
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, or 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 fix this CI failure" or "@codex address that feedback".
| {funct = fn1; args = (Nolabel, a) :: args; partial; transformed_jsx}; | ||
| pexp_loc = e.pexp_loc; | ||
| pexp_attributes = e.pexp_attributes @ f.pexp_attributes; | ||
| pexp_is_return = false; | ||
| } |
There was a problem hiding this comment.
[P1] Preserve return markers when rewriting application expressions
The new pexp_is_return flag is set by Res_return_marker before the built‑in PPX rewrites applications, but Ast_exp_apply.app_exp_mapper now recreates the expression with pexp_is_return = false unconditionally. Any call expression that passes through this mapper (which is invoked for every Pexp_apply in bs_builtin_ppx) will therefore lose its return annotation even when it was in tail position, so the typed AST never exposes exp_is_return for the common case of let f x = g x. The mapper should carry over the original expression’s flag (and similar for nested cases) rather than resetting it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@codex see if you can repro the issue in a test
There was a problem hiding this comment.
Summary
- Added an
extract_fun_bodyhelper to consolidate structure matching and introduced an OUnit test that reproduces the loss of return flags after the built-in application rewrite. tests/ounit_tests/ounit_ast_return_tests.mlL12-L91 - Linked the OUnit test executable against the
frontendlibrary so the new test can invokePpx_entry.rewrite_implementation. tests/ounit_tests/duneL1-L14
Testing
⚠️ dune runtest tests/ounit_tests(fails becauseduneis not available in the container)
rescript
@rescript/darwin-arm64
@rescript/darwin-x64
@rescript/linux-arm64
@rescript/linux-x64
@rescript/runtime
@rescript/win32-x64
commit: |
Summary\n- annotate parsed expressions with mutable return flags and propagate into the typed tree\n- add a parser-time pass that marks tail positions and expose an accessor for tooling\n- update helpers and add OUnit coverage for return semantics\n\n## Testing\n- dune build\n- dune runtest tests/ounit_tests\n- make