Skip to content

Reject coercions and variant spreads that treat bool and unit as variants - #9218

Open
mununki wants to merge 1 commit into
masterfrom
reject-unit-to-string-coercion
Open

mununki wants to merge 1 commit into
masterfrom
reject-unit-to-string-coercion

Conversation

@mununki

@mununki mununki commented Nov 4, 2026

Copy link
Copy Markdown
Member

Fixes: #9216

unit could be coerced to string, and a closed polymorphic variant whose tags are "true", "false" or "()" to bool or unit. (() :> string) compiled to nothing, so let s: string = (() :> string) left s as undefined, and s->String.length threw a TypeError; (p :> bool) for p: [#"true" | #"false"] returned the string "false", which is truthy. Type errors suggested both coercions as fixes. A variant could also spread bool or unit: type t = | ...bool | Other built true and false as booleans, but its switch compared them with the strings "true" and "false" and never matched, and (x :> string) left a boolean typed as a string.

bool and unit are declared as variants with the constructors false, true and () and no @as, but their values are false, true and undefined. Variant_coercion.can_try_coerce_variant_to_primitive left out bool but not unit, so the check took () for its name as a string, and Variant_coercion.type_is_variant, which guards the coercion from a polymorphic variant, left out neither type. A new Variant_coercion.is_bool_or_unit now keeps both types out of these two coercions (type_is_variant is renamed can_try_coerce_polyvariant_to_variant), and Variant_type_spread.map_constructors rejects a spread of either as an invalid spread. Ctype.subtype_rec also rejects a coercion between either type and another variant or record before the variant checks, whose messages described them as variants that could take @as or @unboxed.

  • (() :> string), also through an alias or a private abbreviation of unit and wherever the subtype check compares unit with string (function parameters and results, promise<unit>, tuples, record and object fields, constructor payloads), is an error, and type errors no longer suggest it.
  • Coercing a closed polymorphic variant to bool or unit, and spreading bool or unit into a variant, are errors.
  • (() :> int), ("a" :> unit) and (() :> unknown) are still errors, now with the plain "Type unit is not a subtype of int" and no advice about @as or @unboxed. (true :> string) was already rejected and is unchanged.
  • bool and unit still coerce to themselves and from their aliases and private abbreviations, and | ...bool and | ...unit still work as patterns on a bool or unit. A variant with constructors such as False | True still coerces to string, and variants with @as payloads, unboxed variants and coercions from polymorphic variants to other variants are unchanged. option, list and result have type parameters, so these coercions never applied to them. No existing super_errors snapshot or test output changes.

Tests: super_errors fixtures for unit coerced to string, int and unknown, string coerced to unit, polymorphic variants coerced to bool and unit, the missing coercion suggestion for unit, and bool and unit spread into a variant. A test in VariantCoercion.res keeps a variant False | True coercing to string and from a polymorphic variant.

Checklist

  • Added a changelog fragment (see changes/README.md), or no release notes are needed and a maintainer should apply changelog:skip.
  • Added tests for the fix or feature.
  • Gave every commit a description below its subject, saying why it is made (see "Commit messages" in CONTRIBUTING.md).
  • Signed off every commit for the DCO (git commit -s).

…ants

Why:
    `bool` and `unit` are declared as variants, with the constructors
    `false`, `true` and `()` and no `@as`, but their values are `false`,
    `true` and `undefined`. The coercion between a variant and a primitive
    left out `bool` but took `unit` for a variant whose constructor is the
    string "()", so `(() :> string)` compiled to nothing and left
    `undefined` typed as a `string`. The coercion from a closed polymorphic
    variant left out neither type and matched `#"true"` to the constructor
    `true` by name. A variant that spread either type built `true`, `false`
    and `undefined`, but matched and coerced them as strings.

`Variant_coercion.is_bool_or_unit` now keeps both types out of the
coercions between a variant and a primitive, the coercion from a
polymorphic variant (`type_is_variant`, renamed
`can_try_coerce_polyvariant_to_variant`) and variant spreads.
`Ctype.subtype_rec` also rejects a coercion between either type and another
variant or record before the variant checks, whose messages described them
as variants.

Diagnostic added (unit coerced to string):
Example:
    let s: string = (() :> string)
    let n = s->String.length
Before (js): throws a TypeError when the module loads
    let n = undefined.length;

    let s;
After (diagnostic):
    A.res:1:18-29
    Type unit is not a subtype of string

Diagnostic added (polymorphic variant coerced to bool):
Example:
    let toBool = (p: [#"true" | #"false"]): bool => (p :> bool)
    let isFalse = toBool(#"false") == false
Before (js):
    function toBool(p) {
      return p;
    }

    let isFalse = "false" === false;
After (diagnostic):
    A.res:1:50-58
    Type [#"false" | #"true"] is not a subtype of bool

Diagnostic added (bool spread into a variant):
Example:
    type t = | ...bool | Other
    let x: t = true
    let r = switch x { | true => "yes" | false => "no" | Other => "other" }
Before (js): `r` stays undefined
    switch (true) {
      case "false" :
        r = "no";
        break;
      case "true" :
        r = "yes";
        break;
      …
    }
After (diagnostic):
    A.res:1:15-18
    This type is not a valid type to spread. It's only possible to spread other variants.

Diagnostic modified (unit coerced to int):
Example:
    let i: int = (() :> int)
Before (diagnostic):
    Type unit is not a subtype of int

    The constructor () of variant unit has no @as payload, which makes it a string at runtime.
    That runtime representation is not compatible with the expected runtime representation of int.

    Fix this by making sure all constructors in variant unit has a runtime representation of int.
After (diagnostic):
    Type unit is not a subtype of int

Diagnostic modified (coercion suggested for unit):
Example:
    let u = (): unit => ()
    let s: string = u()
Before (diagnostic):
    This has type: unit
    But it's expected to have type: string

    Possible solutions:
    - These types are compatible at runtime. You can use the coercion operator to convert to the expected type: (u() :> string)
After (diagnostic):
    This has type: unit
    But it's expected to have type: string

Review: Does a working program stop compiling? One that coerces `unit` to
    `string` or a polymorphic variant to `bool` or `unit`, which gave a
    value of another type than its own, or that spreads `bool` or `unit`
    into a variant, whose switches never matched `true`, `false` or `()`.
    A variant that only used its other constructors can declare them
    without the spread.

Review: Which coercions still work? `bool` and `unit` coerce to themselves,
    from their aliases and from a private type defined as one, and a
    variant `False | True` still coerces to `string`. `option`, `list` and
    `result` have type parameters, so these coercions never applied to
    them. No existing snapshot or test output changes.

Signed-off-by: mununki <woonki.moon@gmail.com>
@github-actions

github-actions Bot commented Nov 4, 2026

Copy link
Copy Markdown

Changelog

🐛 Bug fix

  • Reject coercing unit to string, as in (() :> string), and a closed polymorphic variant to bool or unit, as from [#"true" | #"false"] to bool. These compiled, but left undefined typed as a string, or a string typed as a bool or unit, and type errors suggested them as fixes.
  • Reject spreading bool or unit into a variant, as in type t = | ...bool | Other. The variant held true, false and undefined, but its switches and coercions took them for the strings "true", "false" and "()".

💅 Polish

  • Coercion errors no longer describe bool and unit as variants that could be given @as or @unboxed.

Semantic Diff

1 commit · Full page

Reject coercions and variant spreads that treat bool and unit as variants

fa1b7f1 · mununki · Nov 04, 2026, 05:00 UTC

Why

bool and unit are declared as variants, with the constructors false, true and () and no @as, but their values are false, true and undefined. The coercion between a variant and a primitive left out bool but took unit for a variant whose constructor is the string "()", so (() :> string) compiled to nothing and left undefined typed as a string. The coercion from a closed polymorphic variant left out neither type and matched #"true" to the constructor true by name. A variant that spread either type built true, false and undefined, but matched and coerced them as strings.

Variant_coercion.is_bool_or_unit now keeps both types out of the coercions between a variant and a primitive, the coercion from a polymorphic variant (type_is_variant, renamed can_try_coerce_polyvariant_to_variant) and variant spreads. Ctype.subtype_rec also rejects a coercion between either type and another variant or record before the variant checks, whose messages described them as variants.

Diagnostic added (unit coerced to string)

let s: string = (() :> string)
let n = s->String.length

js · Before · throws a TypeError when the module loads

let n = undefined.length;

let s;

diagnostic · After

A.res:1:18-29
Type unit is not a subtype of string

Diagnostic added (polymorphic variant coerced to bool)

let toBool = (p: [#"true" | #"false"]): bool => (p :> bool)
let isFalse = toBool(#"false") == false

js · Before

function toBool(p) {
  return p;
}

let isFalse = "false" === false;

diagnostic · After

A.res:1:50-58
Type [#"false" | #"true"] is not a subtype of bool

Diagnostic added (bool spread into a variant)

type t = | ...bool | Other
let x: t = true
let r = switch x { | true => "yes" | false => "no" | Other => "other" }

js · Before · r stays undefined

switch (true) {
  case "false" :
    r = "no";
    break;
  case "true" :
    r = "yes";
    break;
  …
}

diagnostic · After

A.res:1:15-18
This type is not a valid type to spread. It's only possible to spread other variants.

Diagnostic modified (unit coerced to int)

let i: int = (() :> int)

diagnostic

 Type unit is not a subtype of int
-
-The constructor () of variant unit has no @as payload, which makes it a string at runtime.
-That runtime representation is not compatible with the expected runtime representation of int.
-
-Fix this by making sure all constructors in variant unit has a runtime representation of int.

Diagnostic modified (coercion suggested for unit)

let u = (): unit => ()
let s: string = u()

diagnostic

 This has type: unit
 But it's expected to have type: string
-
-Possible solutions:
-- These types are compatible at runtime. You can use the coercion operator to convert to the expected type: (u() :> string)

Review

  • Does a working program stop compiling? One that coerces unit to string or a polymorphic variant to bool or unit, which gave a value of another type than its own, or that spreads bool or unit into a variant, whose switches never matched true, false or (). A variant that only used its other constructors can declare them without the spread.
  • Which coercions still work? bool and unit coerce to themselves, from their aliases and from a private type defined as one, and a variant False | True still coerces to string. option, list and result have type parameters, so these coercions never applied to them. No existing snapshot or test output changes.

@codecov

codecov Bot commented Nov 4, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 90.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 81.75%. Comparing base (227b697) to head (fa1b7f1).

Files with missing lines Patch % Lines
compiler/ml/ctype.ml 80.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #9218   +/-   ##
=======================================
  Coverage   81.75%   81.75%           
=======================================
  Files         457      457           
  Lines       65332    65339    +7     
=======================================
+ Hits        53413    53419    +6     
- Misses      11919    11920    +1     
Files with missing lines Coverage Δ
compiler/ml/variant_coercion.ml 92.04% <100.00%> (+0.09%) ⬆️
compiler/ml/variant_type_spread.ml 91.42% <100.00%> (+0.25%) ⬆️
compiler/ml/ctype.ml 70.00% <80.00%> (+<0.01%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@pkg-pr-new

pkg-pr-new Bot commented Nov 4, 2026

Copy link
Copy Markdown

Open in StackBlitz

rescript

npm i https://pkg.pr.new/rescript@9218

@rescript/belt

npm i https://pkg.pr.new/@rescript/belt@9218

@rescript/darwin-arm64

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

@rescript/linux-arm64

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

@rescript/linux-x64

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

@rescript/runtime

npm i https://pkg.pr.new/@rescript/runtime@9218

@rescript/win32-x64

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

commit: fa1b7f1

@github-actions

github-actions Bot commented Nov 4, 2026

Copy link
Copy Markdown

@mununki
mununki requested review from cknitt and shulhi November 4, 2026 13:27
@mununki

mununki commented Nov 4, 2026

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Nov 4, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-11-04T13:29:04.962173Z fa1b7f1 Manual 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Delightful!

Reviewed commit: fa1b7f12bd

ℹ️ 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".

This branch has not been deployed

No deployments
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.

(() :> string) compiles, and other coercions and variant spreads treat bool and unit as variants

1 participant