- Notifications
You must be signed in to change notification settings - Fork471
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:master
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
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:  |
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".
| {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.
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 newpexp_is_return flag is set byRes_return_marker before the built‑in PPX rewrites applications, butAst_exp_apply.app_exp_mapper now recreates the expression withpexp_is_return = false unconditionally. Any call expression that passes through this mapper (which is invoked for everyPexp_apply inbs_builtin_ppx) will therefore lose its return annotation even when it was in tail position, so the typed AST never exposesexp_is_return for the common case oflet 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@rescript/darwin-arm64@rescript/darwin-x64@rescript/linux-arm64@rescript/linux-x64@rescript/runtime@rescript/win32-x64commit: |
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