From 07948b9a953692c6549069bf8d8184ae1c4fff78 Mon Sep 17 00:00:00 2001 From: Gabriel Nordeborn Date: Wed, 18 Oct 2023 19:02:44 +0200 Subject: [PATCH 1/5] allow coercing unboxed with payload to primitive when applicable --- jscomp/ml/ctype.ml | 4 ++-- jscomp/ml/variant_coercion.ml | 23 ++++++++++++++++++----- 2 files changed, 20 insertions(+), 7 deletions(-) diff --git a/jscomp/ml/ctype.ml b/jscomp/ml/ctype.ml index 89143f44be..19c5779560 100644 --- a/jscomp/ml/ctype.ml +++ b/jscomp/ml/ctype.ml @@ -3956,8 +3956,8 @@ let rec subtype_rec env trace t1 t2 cstrs = -> (* type coercion for variants to primitives *) (match Variant_coercion.can_try_coerce_variant_to_primitive (extract_concrete_typedecl env t1) with - | Some constructors -> - if constructors |> Variant_coercion.can_coerce_variant ~path then + | Some (constructors, unboxed) -> + if constructors |> Variant_coercion.can_coerce_variant ~path ~unboxed then cstrs else (trace, t1, t2, !univar_pairs)::cstrs diff --git a/jscomp/ml/variant_coercion.ml b/jscomp/ml/variant_coercion.ml index f174313ad0..2a3830d502 100644 --- a/jscomp/ml/variant_coercion.ml +++ b/jscomp/ml/variant_coercion.ml @@ -6,15 +6,28 @@ let can_coerce_path (path : Path.t) = || Path.same path Predef.path_int || Path.same path Predef.path_float -let can_coerce_variant ~(path : Path.t) +let check_paths_same p1 p2 target_path = + Path.same p1 target_path && Path.same p2 target_path + +let can_coerce_variant ~(path : Path.t) ~unboxed (constructors : Types.constructor_declaration list) = constructors |> List.for_all (fun (c : Types.constructor_declaration) -> let args = c.cd_args in - let payload = Ast_untagged_variants.process_tag_type c.cd_attributes in + let asPayload = + Ast_untagged_variants.process_tag_type c.cd_attributes + in match args with + | Cstr_tuple [{desc = Tconstr (p, [], _)}] when unboxed -> + let path_same = check_paths_same p path in + (* unboxed String(string) :> string *) + path_same Predef.path_string + || (* unboxed Number(float) :> float *) + path_same Predef.path_float | Cstr_tuple [] -> ( - match payload with + (* Check that @as payloads match with the target path to coerce to. + No @as means the default encoding, which is string *) + match asPayload with | None | Some (String _) -> Path.same path Predef.path_string | Some (Int _) -> Path.same path Predef.path_int | Some (Float _) -> Path.same path Predef.path_float @@ -24,10 +37,10 @@ let can_coerce_variant ~(path : Path.t) let can_try_coerce_variant_to_primitive ((_, p, typedecl) : Path.t * Path.t * Types.type_declaration) = match typedecl with - | {type_kind = Type_variant constructors; type_params = []} + | {type_kind = Type_variant constructors; type_params = []; type_attributes} when Path.name p <> "bool" -> (* bool is represented as a variant internally, so we need to account for that *) - Some constructors + Some (constructors, type_attributes |> Ast_untagged_variants.has_untagged) | _ -> None let variant_representation_matches (c1_attrs : Parsetree.attributes) From 623a37acdb22ea58b0cb8cc400a97b8a63e81bdb Mon Sep 17 00:00:00 2001 From: Gabriel Nordeborn Date: Wed, 18 Oct 2023 21:57:36 +0200 Subject: [PATCH 2/5] test --- .../variant_coercion_string_unboxed.res.expected | 10 ++++++++++ .../fixtures/variant_coercion_string_unboxed.res | 6 ++++++ 2 files changed, 16 insertions(+) create mode 100644 jscomp/build_tests/super_errors/expected/variant_coercion_string_unboxed.res.expected create mode 100644 jscomp/build_tests/super_errors/fixtures/variant_coercion_string_unboxed.res diff --git a/jscomp/build_tests/super_errors/expected/variant_coercion_string_unboxed.res.expected b/jscomp/build_tests/super_errors/expected/variant_coercion_string_unboxed.res.expected new file mode 100644 index 0000000000..a152598031 --- /dev/null +++ b/jscomp/build_tests/super_errors/expected/variant_coercion_string_unboxed.res.expected @@ -0,0 +1,10 @@ + + We've found a bug for you! + /.../fixtures/variant_coercion_string_unboxed.res:6:10-20 + + 4 │ let x = One + 5 │ + 6 │ let y = (x :> string) + 7 │ + + Type x is not a subtype of string \ No newline at end of file diff --git a/jscomp/build_tests/super_errors/fixtures/variant_coercion_string_unboxed.res b/jscomp/build_tests/super_errors/fixtures/variant_coercion_string_unboxed.res new file mode 100644 index 0000000000..bce8ba5656 --- /dev/null +++ b/jscomp/build_tests/super_errors/fixtures/variant_coercion_string_unboxed.res @@ -0,0 +1,6 @@ +@unboxed +type x = One | Two | Other(float) + +let x = One + +let y = (x :> string) From b26c7d94e176bcb3e61d1e60195f6096a0dbfcf9 Mon Sep 17 00:00:00 2001 From: Gabriel Nordeborn Date: Wed, 18 Oct 2023 22:00:25 +0200 Subject: [PATCH 3/5] changelog --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index e6e760f6db..2b8ee2b84d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,9 @@ # 11.0.0-rc.5 (Unreleased) +#### :rocket: New Feature +- Allow coercing unboxed variants with only strings (now including with a single payload of string) to the primitive string. https://github.com/rescript-lang/rescript-compiler/pull/6441 + #### :bug: Bug Fix - Fix issue with dynamic import of module in nested expressions https://github.com/rescript-lang/rescript-compiler/pull/6431 From e737fab1c23546fd765427fd51232a36837059de Mon Sep 17 00:00:00 2001 From: Gabriel Nordeborn Date: Wed, 18 Oct 2023 22:03:42 +0200 Subject: [PATCH 4/5] tests --- jscomp/test/VariantCoercion.js | 20 ++++++++++++++++++-- jscomp/test/VariantCoercion.res | 14 ++++++++++++++ 2 files changed, 32 insertions(+), 2 deletions(-) diff --git a/jscomp/test/VariantCoercion.js b/jscomp/test/VariantCoercion.js index 3761e0d04e..de7f98ecaf 100644 --- a/jscomp/test/VariantCoercion.js +++ b/jscomp/test/VariantCoercion.js @@ -14,7 +14,22 @@ var CoerceVariants = { y: x }; -var a = "Three"; +var a = "hello"; + +var c = 100; + +var CoerceWithPayload = { + a: a, + aa: "First", + b: a, + bb: "First", + c: c, + cc: 2, + d: c, + dd: 2 +}; + +var a$1 = "Three"; var b = "Three"; @@ -26,11 +41,12 @@ var ii = 1.1; var dd = 1.1; -exports.a = a; +exports.a = a$1; exports.b = b; exports.i = i; exports.d = d; exports.ii = ii; exports.dd = dd; exports.CoerceVariants = CoerceVariants; +exports.CoerceWithPayload = CoerceWithPayload; /* No side effect */ diff --git a/jscomp/test/VariantCoercion.res b/jscomp/test/VariantCoercion.res index 73d8d24dfb..90b48d8935 100644 --- a/jscomp/test/VariantCoercion.res +++ b/jscomp/test/VariantCoercion.res @@ -30,3 +30,17 @@ module CoerceVariants = { let x: x = One({age: 1}) let y: y = (x :> y) } + +module CoerceWithPayload = { + @unboxed type strings = String(string) | First | Second | Third + let a: strings = String("hello") + let aa: strings = First + let b: string = (a :> string) + let bb: string = (aa :> string) + + @unboxed type floats = Number(float) | @as(1.) First | @as(2.) Second | @as(3.) Third + let c: floats = Number(100.) + let cc: floats = Second + let d: float = (c :> float) + let dd: float = (cc :> float) +} From 95be068eb72e3e08ad0280bf10708b4c8212fc55 Mon Sep 17 00:00:00 2001 From: Gabriel Nordeborn Date: Thu, 19 Oct 2023 13:12:36 +0200 Subject: [PATCH 5/5] refactor --- jscomp/ml/ctype.ml | 2 +- jscomp/ml/variant_coercion.ml | 51 ++++++++++++++++++----------------- 2 files changed, 28 insertions(+), 25 deletions(-) diff --git a/jscomp/ml/ctype.ml b/jscomp/ml/ctype.ml index 19c5779560..0a3afa2027 100644 --- a/jscomp/ml/ctype.ml +++ b/jscomp/ml/ctype.ml @@ -3957,7 +3957,7 @@ let rec subtype_rec env trace t1 t2 cstrs = (* type coercion for variants to primitives *) (match Variant_coercion.can_try_coerce_variant_to_primitive (extract_concrete_typedecl env t1) with | Some (constructors, unboxed) -> - if constructors |> Variant_coercion.can_coerce_variant ~path ~unboxed then + if constructors |> Variant_coercion.variant_has_same_runtime_representation_as_target ~targetPath:path ~unboxed then cstrs else (trace, t1, t2, !univar_pairs)::cstrs diff --git a/jscomp/ml/variant_coercion.ml b/jscomp/ml/variant_coercion.ml index 2a3830d502..728a326e73 100644 --- a/jscomp/ml/variant_coercion.ml +++ b/jscomp/ml/variant_coercion.ml @@ -9,30 +9,33 @@ let can_coerce_path (path : Path.t) = let check_paths_same p1 p2 target_path = Path.same p1 target_path && Path.same p2 target_path -let can_coerce_variant ~(path : Path.t) ~unboxed - (constructors : Types.constructor_declaration list) = - constructors - |> List.for_all (fun (c : Types.constructor_declaration) -> - let args = c.cd_args in - let asPayload = - Ast_untagged_variants.process_tag_type c.cd_attributes - in - match args with - | Cstr_tuple [{desc = Tconstr (p, [], _)}] when unboxed -> - let path_same = check_paths_same p path in - (* unboxed String(string) :> string *) - path_same Predef.path_string - || (* unboxed Number(float) :> float *) - path_same Predef.path_float - | Cstr_tuple [] -> ( - (* Check that @as payloads match with the target path to coerce to. - No @as means the default encoding, which is string *) - match asPayload with - | None | Some (String _) -> Path.same path Predef.path_string - | Some (Int _) -> Path.same path Predef.path_int - | Some (Float _) -> Path.same path Predef.path_float - | Some (Null | Undefined | Bool _ | Untagged _) -> false) - | _ -> false) +(* Checks if every case of the variant has the same runtime representation as the target type. *) +let variant_has_same_runtime_representation_as_target ~(targetPath : Path.t) + ~unboxed (constructors : Types.constructor_declaration list) = + (* Helper function to check if a constructor has the same runtime representation as the target type *) + let has_same_runtime_representation (c : Types.constructor_declaration) = + let args = c.cd_args in + let asPayload = Ast_untagged_variants.process_tag_type c.cd_attributes in + + match args with + | Cstr_tuple [{desc = Tconstr (p, [], _)}] when unboxed -> + let path_same = check_paths_same p targetPath in + (* unboxed String(string) :> string *) + path_same Predef.path_string + || (* unboxed Number(float) :> float *) + path_same Predef.path_float + | Cstr_tuple [] -> ( + (* Check that @as payloads match with the target path to coerce to. + No @as means the default encoding, which is string *) + match asPayload with + | None | Some (String _) -> Path.same targetPath Predef.path_string + | Some (Int _) -> Path.same targetPath Predef.path_int + | Some (Float _) -> Path.same targetPath Predef.path_float + | Some (Null | Undefined | Bool _ | Untagged _) -> false) + | _ -> false + in + + List.for_all has_same_runtime_representation constructors let can_try_coerce_variant_to_primitive ((_, p, typedecl) : Path.t * Path.t * Types.type_declaration) =