-
Notifications
You must be signed in to change notification settings - Fork 471
Mark return expressions in parsed and typed ASTs #7898
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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:
Screenshot 2025年09月18日 at 05 00 29
@chatgpt-codex-connector
chatgpt-codex-connector
bot
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@codex see if you can repro the issue in a test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
npm i https://pkg.pr.new/rescript-lang/rescript@7898
@rescript/darwin-arm64
npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/darwin-arm64@7898
@rescript/darwin-x64
npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/darwin-x64@7898
@rescript/linux-arm64
npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/linux-arm64@7898
@rescript/linux-x64
npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/linux-x64@7898
@rescript/runtime
npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/runtime@7898
@rescript/win32-x64
npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/win32-x64@7898
commit: a39fb92
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