Skip to content

Test: ignore inferred arity in case of %raw. #7542

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cristianoc
Copy link
Collaborator

The inferred arity of raw JS code is actively used with %ffi, but it can produce unexpected results with %raw:

let foo: int => int = %raw(`function add(x, y=5){ return x + y }`)

Console.log(foo(2))

This PR tests turning off the use of inferred arity so that int => int above is just taken at face value.

The inferred arity of raw JS code is actively used with `%ffi`, but it can produce unexpected results with `%raw`:

```
let foo: int => int = %raw(`function add(x, y=5){ return x + y }`)

Console.log(foo(2))
```
tsnobip added a commit to tsnobip/rescript-relay-router that referenced this pull request Jun 12, 2025
avoids a type error with rescript v12
cf rescript-lang/rescript#7542
@cristianoc cristianoc requested review from tsnobip and Copilot June 12, 2025 14:40
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR disables the compiler’s automatic arity inference for raw JavaScript code by preventing the existing pattern branches from matching.

  • Added when false guards to skip raw JS arity collection in lam_pass_collect.ml
  • Added when false guards to skip raw JS arity analysis in lam_arity_analysis.ml

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
compiler/core/lam_pass_collect.ml Disabled inferred arity collection for %raw by guarding the JS function pattern with when false
compiler/core/lam_arity_analysis.ml Disabled inferred arity analysis for %raw by guarding the JS function pattern with when false

@@ -68,7 +68,8 @@ let collect_info (meta : Lam_stats.t) (lam : Lam.t) =
{
primitive = Praw_js_code {code_info = Exp (Js_function {arity})};
args = _;
} ->
}
when false ->
Copy link
Preview

Copilot AI Jun 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Using a when false guard makes this branch permanently unreachable. Consider removing the dead code or replacing the guard with a named feature flag and an explanatory comment to clarify intent.

Copilot uses AI. Check for mistakes.

Comment on lines +73 to 74
when false ->
Lam_arity.info [arity] false
Copy link
Preview

Copilot AI Jun 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] This when false guard disables the raw JS arity branch but leaves dead code. It may be clearer to remove the branch or use a descriptive compile‐time flag plus comments to document why it's disabled.

Suggested change
when false ->
Lam_arity.info [arity] false
when ENABLE_RAW_JS_ARITY ->
Lam_arity.info [arity] false
(* The ENABLE_RAW_JS_ARITY flag controls whether the raw JS arity branch is enabled.
This branch is currently disabled for performance reasons and because it is not
required in the current implementation. Enable it by defining ENABLE_RAW_JS_ARITY
as true at compile time if needed in the future. *)

Copilot uses AI. Check for mistakes.

Copy link
Member

@tsnobip tsnobip left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cristianoc this means inferred arity will be ignored even when there's no type signature, right? It's dangerous anyway since it doesn't reflect it in the types.
I guess you can still use %ffi if you want to type your js code. We should likely document %ffi by the way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants