- Notifications
You must be signed in to change notification settings - Fork471
Fix fatal error for external with @as#7978
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
Conversation
| in | ||
| let fn_type=Ast_helper.Typ.arrows~loc args result_typein | ||
| (build_uncurried_type~arity:(List.length args)fn_type, | ||
| ( fn_type, |
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.
build_uncurried_type was trying to set a function type even if no args were left.
But the function type with the correct arity is already set byAst_helper.Typ.arrows anyway when required.
pkg-pr-newbot commentedOct 22, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
rescript@rescript/darwin-arm64@rescript/darwin-x64@rescript/linux-arm64@rescript/linux-x64@rescript/runtime@rescript/win32-x64commit: |
Or do we want/need to add an "artificial" |
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.
The fact that the function without any args gets called automatically seems pretty weird to me.
I would have expected this would not be allowed.
But if that's what one wants, OK.
Notice the risk is this change affects other cases unrelated to this. But I assume some though went into removing build_uncurried_type as mentioned in the comment.
Not sure if that's what one wants. 🙂 I can look into introducing an "artificial" unit arg.
|
@cristianoc now adding a synthetic unit argument, please have another look. |
@codex review |
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 automated review suggestions for this pull request.
ℹ️ 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".
| (Ast_helper.Typ.arrows~loc args result_type, | ||
| External_ffi_types.ffi_bs arg_type_specs return_wrapper ffi, | ||
| unused_attrs, |
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.
Preserve uncurried arity when rebuilding external type
Previously this code rebuilt the return type withbuild_uncurried_type ~arity:(List.length args) so externals declared with an uncurried arrow retained theirarity metadata. After the change it returnsAst_helper.Typ.arrows directly and thebuild_uncurried_type wrapper is never applied in the non-object path, so externals such asexternal add: (int, int) => int are now typed asint -> int -> int. Calls written with uncurried syntax (add(1, 2)) will start raisingApply_non_function/arity mismatches during type checking because the arity flag is gone. The new code should still applybuild_uncurried_type to the reconstructed arrow to keep uncurried externals working.
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 ButAst_helper.Typ.arrows does already set the arity for the outer function toList.length args.
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.
To use Codex here,create an environment for this repo.
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.
Definitely not true,external add: (int, int) => int still works correctly.
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.
How comes codex runs now? That changed?
It used to get stuck trying to install ocaml.
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.
I think it didn't actually run anything, it just reviews the code.
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.
Looks great.
Btw conceptually this just builds a function of arity 0 (the same way as arity 2 with unit type). I thought we already had helpers that take arity as arg and support 0.
But not 100% sure.
3565257 intomasterUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Fixes#7972
When the external function has a single arg that gets a fixed value via@as(json, all that's left is a actually constant, see test.