From 3ee079d9bf418413a0d00aace13372efb322035a Mon Sep 17 00:00:00 2001 From: Gabriel Nordeborn Date: Wed, 18 Oct 2023 22:19:59 +0200 Subject: [PATCH 1/5] support coercing string to elgible untagged variants --- ..._string_to_variant_no_payload.res.expected | 10 ++++++++++ ..._coercion_string_to_variant_no_payload.res | 6 ++++++ jscomp/ml/ctype.ml | 19 +++++++++++++++++++ jscomp/test/VariantCoercion.js | 16 ++++++++++++++-- jscomp/test/VariantCoercion.res | 8 ++++++++ 5 files changed, 57 insertions(+), 2 deletions(-) create mode 100644 jscomp/build_tests/super_errors/expected/variant_coercion_string_to_variant_no_payload.res.expected create mode 100644 jscomp/build_tests/super_errors/fixtures/variant_coercion_string_to_variant_no_payload.res diff --git a/jscomp/build_tests/super_errors/expected/variant_coercion_string_to_variant_no_payload.res.expected b/jscomp/build_tests/super_errors/expected/variant_coercion_string_to_variant_no_payload.res.expected new file mode 100644 index 0000000000..1b60c86925 --- /dev/null +++ b/jscomp/build_tests/super_errors/expected/variant_coercion_string_to_variant_no_payload.res.expected @@ -0,0 +1,10 @@ + + We've found a bug for you! + /.../fixtures/variant_coercion_string_to_variant_no_payload.res:6:10-15 + + 4 │ let x = "one" + 5 │ + 6 │ let y = (x :> x) + 7 │ + + Type string is not a subtype of x \ No newline at end of file diff --git a/jscomp/build_tests/super_errors/fixtures/variant_coercion_string_to_variant_no_payload.res b/jscomp/build_tests/super_errors/fixtures/variant_coercion_string_to_variant_no_payload.res new file mode 100644 index 0000000000..fd92df20c1 --- /dev/null +++ b/jscomp/build_tests/super_errors/fixtures/variant_coercion_string_to_variant_no_payload.res @@ -0,0 +1,6 @@ +@unboxed +type x = One | Two + +let x = "one" + +let y = (x :> x) diff --git a/jscomp/ml/ctype.ml b/jscomp/ml/ctype.ml index 0a3afa2027..94d4e1be75 100644 --- a/jscomp/ml/ctype.ml +++ b/jscomp/ml/ctype.ml @@ -3951,6 +3951,25 @@ let rec subtype_rec env trace t1 t2 cstrs = end | (Tconstr(p1, _, _), _) when generic_private_abbrev env p1 -> subtype_rec env trace (expand_abbrev_opt env t1) t2 cstrs + | (Tconstr(path, [], _), Tconstr(_, [], _)) when Path.same path Predef.path_string && + extract_concrete_typedecl env t2 |> Variant_coercion.can_try_coerce_variant_to_primitive |> Option.is_some + -> + (* type coercion for strings to elgible unboxed variants: + - must be unboxed + - must coercable to string + - must have a constructor case with a string payload *) + (match Variant_coercion.can_try_coerce_variant_to_primitive (extract_concrete_typedecl env t2) with + | Some (constructors, true) -> + if constructors |> Variant_coercion.can_coerce_variant ~path ~unboxed:true + && constructors |> List.exists(fun (c: constructor_declaration) -> + match c.cd_args with + | Cstr_tuple [{desc=Tconstr (p, [], _)}] when Path.same p Predef.path_string -> true + | _ -> false) + then + cstrs + else + (trace, t1, t2, !univar_pairs)::cstrs + | _ -> (trace, t1, t2, !univar_pairs)::cstrs) | (Tconstr(_, [], _), Tconstr(path, [], _)) when Variant_coercion.can_coerce_path path && extract_concrete_typedecl env t1 |> Variant_coercion.can_try_coerce_variant_to_primitive |> Option.is_some -> diff --git a/jscomp/test/VariantCoercion.js b/jscomp/test/VariantCoercion.js index de7f98ecaf..2fe7d4bd40 100644 --- a/jscomp/test/VariantCoercion.js +++ b/jscomp/test/VariantCoercion.js @@ -29,7 +29,18 @@ var CoerceWithPayload = { dd: 2 }; -var a$1 = "Three"; +var a$1 = "hello"; + +var aa = "First"; + +var CoerceFromStringToVariant = { + a: a$1, + aa: aa, + b: a$1, + bb: aa +}; + +var a$2 = "Three"; var b = "Three"; @@ -41,7 +52,7 @@ var ii = 1.1; var dd = 1.1; -exports.a = a$1; +exports.a = a$2; exports.b = b; exports.i = i; exports.d = d; @@ -49,4 +60,5 @@ exports.ii = ii; exports.dd = dd; exports.CoerceVariants = CoerceVariants; exports.CoerceWithPayload = CoerceWithPayload; +exports.CoerceFromStringToVariant = CoerceFromStringToVariant; /* No side effect */ diff --git a/jscomp/test/VariantCoercion.res b/jscomp/test/VariantCoercion.res index 90b48d8935..799485dbc6 100644 --- a/jscomp/test/VariantCoercion.res +++ b/jscomp/test/VariantCoercion.res @@ -44,3 +44,11 @@ module CoerceWithPayload = { let d: float = (c :> float) let dd: float = (cc :> float) } + +module CoerceFromStringToVariant = { + @unboxed type strings = String(string) | First | Second | Third + let a = "hello" + let aa = "First" + let b: strings = (a :> strings) + let bb: strings = (aa :> strings) +} From 0c75e289e2310d318386080dc94240de772964e0 Mon Sep 17 00:00:00 2001 From: Gabriel Nordeborn Date: Thu, 19 Oct 2023 19:32:12 +0200 Subject: [PATCH 2/5] changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2b8ee2b84d..15be7fdb23 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,7 @@ #### :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 +- Allow coercing strings to unboxed variants that are coercable to string and has a catch-all unboxed string case. https://github.com/rescript-lang/rescript-compiler/pull/6443 #### :bug: Bug Fix From e74072b16cc034bbcdb448061fee30a65d40f1ef Mon Sep 17 00:00:00 2001 From: Gabriel Nordeborn Date: Thu, 19 Oct 2023 19:34:30 +0200 Subject: [PATCH 3/5] fix --- jscomp/ml/ctype.ml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/jscomp/ml/ctype.ml b/jscomp/ml/ctype.ml index 94d4e1be75..87aa06ed5b 100644 --- a/jscomp/ml/ctype.ml +++ b/jscomp/ml/ctype.ml @@ -3960,7 +3960,7 @@ let rec subtype_rec env trace t1 t2 cstrs = - must have a constructor case with a string payload *) (match Variant_coercion.can_try_coerce_variant_to_primitive (extract_concrete_typedecl env t2) with | Some (constructors, true) -> - if constructors |> Variant_coercion.can_coerce_variant ~path ~unboxed:true + if constructors |> Variant_coercion.variant_has_same_runtime_representation_as_target ~targetPath:path ~unboxed:true && constructors |> List.exists(fun (c: constructor_declaration) -> match c.cd_args with | Cstr_tuple [{desc=Tconstr (p, [], _)}] when Path.same p Predef.path_string -> true From 4a1ea9851f39efcc94c2b64e668cbb4b2598a925 Mon Sep 17 00:00:00 2001 From: Gabriel Nordeborn Date: Thu, 19 Oct 2023 21:12:14 +0200 Subject: [PATCH 4/5] all constructors in variant does not need to be coercable to string as long as there is a catch all case --- jscomp/ml/ctype.ml | 8 +------- jscomp/ml/variant_coercion.ml | 11 +++++++++++ jscomp/test/VariantCoercion.js | 6 +++++- jscomp/test/VariantCoercion.res | 4 ++++ 4 files changed, 21 insertions(+), 8 deletions(-) diff --git a/jscomp/ml/ctype.ml b/jscomp/ml/ctype.ml index 87aa06ed5b..cb4595d27b 100644 --- a/jscomp/ml/ctype.ml +++ b/jscomp/ml/ctype.ml @@ -3956,16 +3956,10 @@ let rec subtype_rec env trace t1 t2 cstrs = -> (* type coercion for strings to elgible unboxed variants: - must be unboxed - - must coercable to string - must have a constructor case with a string payload *) (match Variant_coercion.can_try_coerce_variant_to_primitive (extract_concrete_typedecl env t2) with | Some (constructors, true) -> - if constructors |> Variant_coercion.variant_has_same_runtime_representation_as_target ~targetPath:path ~unboxed:true - && constructors |> List.exists(fun (c: constructor_declaration) -> - match c.cd_args with - | Cstr_tuple [{desc=Tconstr (p, [], _)}] when Path.same p Predef.path_string -> true - | _ -> false) - then + if constructors |> Variant_coercion.variant_has_catch_all_string_case then cstrs else (trace, t1, t2, !univar_pairs)::cstrs diff --git a/jscomp/ml/variant_coercion.ml b/jscomp/ml/variant_coercion.ml index 728a326e73..7912c76f0a 100644 --- a/jscomp/ml/variant_coercion.ml +++ b/jscomp/ml/variant_coercion.ml @@ -9,6 +9,17 @@ 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 variant_has_catch_all_string_case (constructors : Types.constructor_declaration list) = + let has_catch_all_string_case (c : Types.constructor_declaration) = + let args = c.cd_args in + match args with + | Cstr_tuple [{desc = Tconstr (p, [], _)}] -> + Path.same p Predef.path_string + | _ -> false + in + + constructors |> List.exists has_catch_all_string_case + (* 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) = diff --git a/jscomp/test/VariantCoercion.js b/jscomp/test/VariantCoercion.js index 2fe7d4bd40..6c471714c9 100644 --- a/jscomp/test/VariantCoercion.js +++ b/jscomp/test/VariantCoercion.js @@ -33,11 +33,15 @@ var a$1 = "hello"; var aa = "First"; +var c$1 = "Hi"; + var CoerceFromStringToVariant = { a: a$1, aa: aa, b: a$1, - bb: aa + bb: aa, + c: c$1, + cc: c$1 }; var a$2 = "Three"; diff --git a/jscomp/test/VariantCoercion.res b/jscomp/test/VariantCoercion.res index 799485dbc6..9aee78a3e0 100644 --- a/jscomp/test/VariantCoercion.res +++ b/jscomp/test/VariantCoercion.res @@ -51,4 +51,8 @@ module CoerceFromStringToVariant = { let aa = "First" let b: strings = (a :> strings) let bb: strings = (aa :> strings) + + @unboxed type mixed = String(string) | @as(1) One | @as(null) Null | Two + let c = "Hi" + let cc: mixed = (c :> mixed) } From 1ad35aae5335567c451d49178d138b7e223208f8 Mon Sep 17 00:00:00 2001 From: Gabriel Nordeborn Date: Thu, 19 Oct 2023 22:16:17 +0200 Subject: [PATCH 5/5] adapt changelog to changed constraints --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 15be7fdb23..4c68d5e690 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,7 +14,7 @@ #### :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 -- Allow coercing strings to unboxed variants that are coercable to string and has a catch-all unboxed string case. https://github.com/rescript-lang/rescript-compiler/pull/6443 +- Allow coercing strings to unboxed variants that has a catch-all unboxed string case. https://github.com/rescript-lang/rescript-compiler/pull/6443 #### :bug: Bug Fix