Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
#### :nail_care: Polish

- Make parser less strict around leading attributes. https://github.com/rescript-lang/rescript/pull/7787
- Dedicated error message for ternary type mismatch. https://github.com/rescript-lang/rescript/pull/7804

#### :house: Internal

Expand Down
8 changes: 8 additions & 0 deletions compiler/ml/error_message_utils.ml
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ type type_clash_context =
| IfCondition
| AssertCondition
| IfReturn
| TernaryReturn
| SwitchReturn
| TryReturn
| StringConcat
Expand Down Expand Up @@ -125,6 +126,7 @@ let context_to_string = function
| Some (FunctionArgument _) -> "FunctionArgument"
| Some ComparisonOperator -> "ComparisonOperator"
| Some IfReturn -> "IfReturn"
| Some TernaryReturn -> "TernaryReturn"
| Some Await -> "Await"
| None -> "None"

Expand Down Expand Up @@ -168,6 +170,7 @@ let error_expected_type_text ppf type_clash_context =
| Some AssertCondition -> fprintf ppf "But assertions must always be of type:"
| Some IfReturn ->
fprintf ppf "But this @{<info>if@} statement is expected to return:"
| Some TernaryReturn -> fprintf ppf "But this ternary is expected to return:"
| Some ArrayValue ->
fprintf ppf "But this array is expected to have items of type:"
| Some (SetRecordField _) -> fprintf ppf "But the record field is of type:"
Expand Down Expand Up @@ -332,6 +335,11 @@ let print_extra_type_clash_help ~extract_concrete_typedecl ~env loc ppf
"\n\n\
\ @{<info>if@} expressions must return the same type in all branches \
(@{<info>if@}, @{<info>else if@}, @{<info>else@})."
| Some TernaryReturn, _ ->
fprintf ppf
"\n\n\
\ Ternaries (@{<info>?@} and @{<info>:@}) must return the same type in \
both branches."
| Some MaybeUnwrapOption, _ ->
fprintf ppf
"\n\n\
Expand Down
20 changes: 16 additions & 4 deletions compiler/ml/typecore.ml
Original file line number Diff line number Diff line change
Expand Up @@ -2826,13 +2826,25 @@ and type_expect_ ~context ?in_function ?(recarg = Rejected) env sexp ty_expected
exp_env = env;
}
| Pexp_ifthenelse (scond, sifso, sifnot) -> (
(* TODO(attributes) Unify the attribute handling in the parser and rest of the compiler. *)
let is_ternary =
let rec has_ternary = function
| [] -> false
| ({Location.txt = "res.ternary"}, _) :: _ -> true
Comment on lines +2830 to +2833
Copy link

Copilot AI Aug 25, 2025

Choose a reason for hiding this comment

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

The magic string "res.ternary" should be defined as a constant to avoid duplication and make it easier to maintain if the attribute name changes.

Suggested change
let is_ternary =
let rec has_ternary = function
| [] -> false
| ({Location.txt = "res.ternary"}, _) :: _ -> true
let res_ternary_attribute = "res.ternary" in
let is_ternary =
let rec has_ternary = function
| [] -> false
| ({Location.txt = res_ternary_attribute}, _) :: _ -> true

Copilot uses AI. Check for mistakes.

| _ :: rest -> has_ternary rest
in
has_ternary sexp.pexp_attributes
in
let return_context =
if is_ternary then Some TernaryReturn else Some IfReturn
in
Comment on lines +2829 to +2840
Copy link
Member Author

Choose a reason for hiding this comment

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

Putting in my TODO to unify all of these attribute names across the syntax and compiler. A lot of them should probably be proper AST nodes.

Copy link
Member

Choose a reason for hiding this comment

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

Would be great to track this in an issue instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done #7805

Copy link
Member

Choose a reason for hiding this comment

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

Thanks!

let cond =
type_expect ~context:(Some IfCondition) env scond Predef.type_bool
in
match sifnot with
| None ->
let ifso =
type_expect ~context:(Some IfReturn) env sifso Predef.type_unit
type_expect ~context:return_context env sifso Predef.type_unit
in
rue
{
Expand All @@ -2844,10 +2856,10 @@ and type_expect_ ~context ?in_function ?(recarg = Rejected) env sexp ty_expected
exp_env = env;
}
| Some sifnot ->
let ifso = type_expect ~context:(Some IfReturn) env sifso ty_expected in
let ifnot = type_expect ~context:(Some IfReturn) env sifnot ty_expected in
let ifso = type_expect ~context:return_context env sifso ty_expected in
let ifnot = type_expect ~context:return_context env sifnot ty_expected in
(* Keep sharing *)
unify_exp ~context:(Some IfReturn) env ifnot ifso.exp_type;
unify_exp ~context:return_context env ifnot ifso.exp_type;
re
{
exp_desc = Texp_ifthenelse (cond, ifso, Some ifnot);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@

We've found a bug for you!
/.../fixtures/ternary_branch_mismatch.res:1:24-26

1 │ let x = true ? "123" : 123
2 │

This has type: int
But this ternary is expected to return: string

Ternaries (? and :) must return the same type in both branches.

You can convert int to string with Int.toString.
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
let x = true ? "123" : 123
Loading