Remove @return(undefined_to_opt) and %undefined_to_opt primitive #7462

cknitt · 2025-05-11T14:16:23Z

@return(undefined_to_opt)
@send external get: (t<'k, 'v>, 'k) => option<'v> = "get"

produces compiler output like

let v = m.get("A");

let v$1 = v === undefined ? undefined : Primitive_option.some(v);

which doesn't make any sense. This is because it is a leftover from the days when options were compiled differently and None was not represented as undefined in the JS output yet.

As this feature was never documented, remove it.

Similarly, I think we can replace %undefined_to_opt by %identity. This leads to slight changes in the compiler output. @cristianoc could you have a look to check if everything is indeed fine?

pkg-pr-new · 2025-05-11T15:33:48Z

Open in StackBlitz

rescript

npm i https://pkg.pr.new/rescript-lang/rescript@7462

@rescript/darwin-arm64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/darwin-arm64@7462

@rescript/darwin-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/darwin-x64@7462

@rescript/linux-arm64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/linux-arm64@7462

@rescript/linux-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/linux-x64@7462

@rescript/win32-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/win32-x64@7462

commit: 96d12d0

cristianoc · 2025-05-12T08:15:31Z

There's some behaviour change after this PR, which can be illustrated through array pop.

This pops an array with a single element and prints whether the resulting value is absent, or prints the value inside the option if present https://rescript-lang.org/try?version=v11.1.4&module=esmodule&code=DYUwLgBGIM5gCgewA4QLwQG7oHwCgIIYB3ASzAGMALCAKRgDoBBAJxYEMBPB5FACgDamALoBKCAG8CEAD4QAcogB2IXBADCymIlANgiAOZ8AROwBGMEErDHR0uQGVEAWxB8AHuLQ4NWnSD1DDztCAF88PE0lbV19I2MAHSUAKmSIFDBSZQAeUmsfVNs8aDgkZD5FFTsShH4nVz4AZlE7SL9YoMSUtIyspWzenLywHALkopqyvnq3SpAW4tha8pmmhaA

If we store in the array an element of option<int>, there's no difference before and after the PR. Notice in particular that popping [None] returns "absent", just like for []. So the presence of an element is not respected.

If we move to elements of option<option<int>>, popping [Some(None)] returns that a value is present, and the value is Some(None) before this PR, but None after this PR.

So the more legit case of storing a single optional value in the array does not behave in the expected way anyway.
And the pretty weird case of storing double options behaves differently.

cristianoc

One more round on this.
We do want to simplify the compiler, yet avoid to change observable behaviour.
In particular, the conversion from undefined to option should have the expected semantics: x => (x === undefined) ? None : (Some(x))

Will add a small commit on top to address the changes.

cristianoc · 2025-05-20T07:34:25Z

compiler/core/lam.ml

@@ -719,8 +715,6 @@ let result_wrap loc (result_type : External_ffi_types.return_wrapper) result =
  | Return_null_to_opt -> prim ~primitive:Pnull_to_opt ~args:[result] loc
  | Return_null_undefined_to_opt ->
    prim ~primitive:Pnull_undefined_to_opt ~args:[result] loc
-  | Return_undefined_to_opt ->


Removing @return(undefined_to_opt) sounds good, as it does not really have a clear use case.

cristianoc · 2025-05-20T07:35:45Z

compiler/core/lam.ml

@@ -298,8 +298,8 @@ let rec apply ?(ap_transformed_jsx = false) fn args (ap_info : ap_info) : t =
          Lprim
            {
              primitive =
-                ( Pundefined_to_opt | Pnull_to_opt | Pnull_undefined_to_opt


Removing the primitive to convert undefined to option makes sense, as the undefined type is de-emphasized (no Stdlib_undefined module), so it would be weird to special case this conversion in the compiler.

cristianoc · 2025-05-20T07:44:48Z

runtime/Belt_internalBucketsType.res

@@ -31,7 +31,7 @@ type container<'hash, 'eq, 'c> = {
 }

 module A = Belt_Array
-external toOpt: opt<'a> => option<'a> = "%undefined_to_opt"
+external toOpt: opt<'a> => option<'a> = "%identity"


This change is fine as it is internal to Belt, and produces identical code.

cristianoc · 2025-05-20T07:45:43Z

runtime/Js.res

@@ -190,7 +190,7 @@ type nullable<+'a> = Js_null_undefined.t<'a> = Value('a) | @as(null) Null | @as(
 type null_undefined<+'a> = nullable<'a>

 external toOption: nullable<'a> => option<'a> = "%nullable_to_opt"
-external undefinedToOption: undefined<'a> => option<'a> = "%undefined_to_opt"
+external undefinedToOption: undefined<'a> => option<'a> = "%identity"


This is an observable behaviour change, need to use an existing conversion function (now that the primitive does not exist anymore).

cristianoc · 2025-05-20T07:46:29Z

runtime/Js_array.res

@@ -245,7 +245,6 @@ Js.Array.pop(empty) == None
 ```
 */
 @send
-@return(undefined_to_opt)


These are fine, as a conversion from undefined to optional seems arbitrary here.

cristianoc · 2025-05-20T07:47:22Z

runtime/Js_undefined.res

@@ -26,8 +26,8 @@

 type t<+'a> = Primitive_js_extern.undefined<'a>

-external to_opt: t<'a> => option<'a> = "%undefined_to_opt"
-external toOption: t<'a> => option<'a> = "%undefined_to_opt"
+external to_opt: t<'a> => option<'a> = "%identity"


Behavioural change exposed to the user, need to preserve original semantics.

cristianoc

Added conversion fix on top. Good to go for me.

cknitt · 2025-05-20T12:41:07Z

@cristianoc Thanks a lot! Will rebase to get it to build again.

Copilot

Pull Request Overview

This PR removes the legacy undefined_to_opt functionality that produced incorrect JavaScript output and was never documented, as well as its associated patterns across the codebase. Key changes include:

Removing the %undefined_to_opt primitive from the compiler’s internal tables and AST.
Eliminating the handling of undefined_to_opt from external FFI types and attribute processing.
Adjusting JavaScript output (Js.js) to replace the removed primitive with undefinedToOption based on Primitive_option.fromUndefined.

Reviewed Changes

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

Show a summary per file

File	Description
lib/es6/Js.js	Adds export of undefinedToOption in place of the removed primitive.
compiler/ml/translcore.ml	Removes %undefined_to_opt from the primitives table.
compiler/ml/printlambda.ml & others	Removes references to Pundefined_to_opt from various modules.
compiler/frontend/*	Deletes undefined_to_opt handling in FFI types and attributes.
compiler/core/*	Cleans up pattern matching and conversion logic for undefined_to_opt.

Comments suppressed due to low confidence (3)

lib/es6/Js.js:71

[nitpick] Consider reviewing the naming of 'undefinedToOption' to ensure consistency with other exported functions in this module.

let undefinedToOption = Primitive_option.fromUndefined;

compiler/frontend/ast_external_process.ml:192

Ensure that there are tests in place to validate the external API behavior now that 'undefined_to_opt' has been removed.

match txt with

compiler/core/lam_compile_primitive.ml:95

Double-check that the removal of the Pundefined_to_opt branch does not affect downstream logic or error handling, and update tests accordingly.

Removed branch for Pundefined_to_opt

cknitt requested a review from cristianoc May 11, 2025 17:24

cknitt force-pushed the remove-return-undefined-to-opt branch from 2eddf54 to fbb3a5c Compare May 19, 2025 13:31

cristianoc reviewed May 20, 2025

View reviewed changes

cristianoc approved these changes May 20, 2025

View reviewed changes

cknitt force-pushed the remove-return-undefined-to-opt branch from d222fc7 to c46839d Compare May 20, 2025 12:41

cknitt and others added 2 commits May 21, 2025 16:42

Remove @return(undefined_to_opt) and %undefined_to_opt primitive

b11b327

Respect conversion semantics from undefined to option.

96d12d0

cknitt force-pushed the remove-return-undefined-to-opt branch from c46839d to 96d12d0 Compare May 21, 2025 15:22

cristianoc requested a review from Copilot June 13, 2025 11:47

Copilot AI reviewed Jun 13, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Remove @return(undefined_to_opt) and %undefined_to_opt primitive #7462

Remove @return(undefined_to_opt) and %undefined_to_opt primitive #7462

Uh oh!

cknitt commented May 11, 2025 •

edited

Loading

Uh oh!

pkg-pr-new bot commented May 11, 2025 •

edited

Loading

Uh oh!

cristianoc commented May 12, 2025

Uh oh!

cristianoc left a comment

Uh oh!

cristianoc May 20, 2025

Uh oh!

cristianoc May 20, 2025

Uh oh!

cristianoc May 20, 2025

Uh oh!

cristianoc May 20, 2025

Uh oh!

cristianoc May 20, 2025

Uh oh!

cristianoc May 20, 2025

Uh oh!

cristianoc left a comment

Uh oh!

cknitt commented May 20, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Remove @return(undefined_to_opt) and %undefined_to_opt primitive #7462

Are you sure you want to change the base?

Remove @return(undefined_to_opt) and %undefined_to_opt primitive #7462

Uh oh!

Conversation

cknitt commented May 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pkg-pr-new bot commented May 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cristianoc commented May 12, 2025

Uh oh!

cristianoc left a comment

Choose a reason for hiding this comment

Uh oh!

cristianoc May 20, 2025

Choose a reason for hiding this comment

Uh oh!

cristianoc May 20, 2025

Choose a reason for hiding this comment

Uh oh!

cristianoc May 20, 2025

Choose a reason for hiding this comment

Uh oh!

cristianoc May 20, 2025

Choose a reason for hiding this comment

Uh oh!

cristianoc May 20, 2025

Choose a reason for hiding this comment

Uh oh!

cristianoc May 20, 2025

Choose a reason for hiding this comment

Uh oh!

cristianoc left a comment

Choose a reason for hiding this comment

Uh oh!

cknitt commented May 20, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

cknitt commented May 11, 2025 •

edited

Loading

pkg-pr-new bot commented May 11, 2025 •

edited

Loading