Skip to content

More error messages #7522

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 17 commits into from
Jun 3, 2025
Merged

More error messages #7522

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
1637911
refactor
zth May 24, 2025
96ed8a3
explicit error for while condition
zth May 24, 2025
b80fea5
explicit for loop condition
zth May 24, 2025
6e5d149
assert condition
zth May 24, 2025
9cd6bfd
refactor
zth May 26, 2025
01288d3
fix
zth May 26, 2025
374d563
try tracking record field type checking
zth May 27, 2025
ab01839
iron out a few more error messages
zth May 30, 2025
9411f97
track optional function arguments
zth May 30, 2025
3e8a1f7
format
zth May 30, 2025
bb358ae
cleanup
zth May 30, 2025
28a3d38
PR comments
zth Jun 2, 2025
0ca45e7
update test output
zth Jun 2, 2025
33c4c2e
change wording a bit
zth Jun 2, 2025
c29fc23
change wording
zth Jun 3, 2025
f2d3278
changelog
zth Jun 3, 2025
1171e16
explicit error message for optional component props, since they have …
zth Jun 3, 2025
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
Prev Previous commit
Next Next commit
change wording a bit
  • Loading branch information
@zth
zth committed Jun 3, 2025
commit 33c4c2e47b964a1cdb1291058bd15427c19fec42
10 changes: 4 additions & 6 deletions compiler/ml/error_message_utils.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,12 +168,11 @@ let error_expected_type_text ppf type_clash_context =
| Some
(RecordField {field_name = "children"; jsx = Some {jsx_type = `Fragment}})
->
fprintf ppf "But children of JSX fragments are expected to have type:"
fprintf ppf "But children of JSX fragments must be of type:"
| Some
(RecordField
{field_name = "children"; jsx = Some {jsx_type = `CustomComponent}}) ->
fprintf ppf
"But children passed to this component are expected to have type:"
fprintf ppf "But children passed to this component must be of type:"
| Some (RecordField {field_name; jsx = Some _}) ->
fprintf ppf "But the component prop @{<info>%s@} is expected to have type:"
field_name
Expand Down Expand Up @@ -435,7 +434,7 @@ let print_extra_type_clash_help ~extract_concrete_typedecl ~env loc ppf
@,\
@{<info>%s@} is an optional record field, and you're passing an \
optional value to it.@,\
Optional fields expect you to pass the concrete value, not an option.\n\
Optional fields expect you to pass a non-optional value.\n\
\ @,\
Possible solutions: @,\
- Unwrap the option and pass a concrete value directly@,\
Expand All @@ -451,8 +450,7 @@ let print_extra_type_clash_help ~extract_concrete_typedecl ~env loc ppf
"@,\
@,\
You're passing an optional value into an optional function argument.@,\
Optional function arguments expect you to pass the concrete value, not \
an option.\n\
Optional function arguments expect you to pass a non-optional value.\n\
\ @,\
Possible solutions: @,\
- Unwrap the option and pass a concrete value directly@,\
Expand Down
2 changes: 1 addition & 1 deletion tests/build_tests/super_errors/expected/jsx_custom_component_children.res.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
25 │

This has type: float
But children passed to this component are expected to have type:
But children passed to this component must be of type:
React.element (defined as Jsx.element)

In JSX, all content must be JSX elements. You can convert float to a JSX element with React.float.
2 changes: 1 addition & 1 deletion tests/build_tests/super_errors/expected/jsx_type_mismatch_float.res.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
18 │

This has type: float
But children of JSX fragments are expected to have type:
But children of JSX fragments must be of type:
React.element (defined as Jsx.element)

In JSX, all content must be JSX elements. You can convert float to a JSX element with React.float.
2 changes: 1 addition & 1 deletion tests/build_tests/super_errors/expected/jsx_type_mismatch_int.res.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
18 │

This has type: int
But children of JSX fragments are expected to have type:
But children of JSX fragments must be of type:
React.element (defined as Jsx.element)

In JSX, all content must be JSX elements. You can convert int to a JSX element with React.int.
2 changes: 1 addition & 1 deletion tests/build_tests/super_errors/expected/jsx_type_mismatch_string.res.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
18 │

This has type: string
But children of JSX fragments are expected to have type:
But children of JSX fragments must be of type:
React.element (defined as Jsx.element)

In JSX, all content must be JSX elements. You can convert string to a JSX element with React.string.
2 changes: 1 addition & 1 deletion tests/build_tests/super_errors/expected/optional_fn_argument_pass_option.res.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
But this optional function argument ~x is expecting: int

You're passing an optional value into an optional function argument.
Optional function arguments expect you to pass the concrete value, not an option.
Optional function arguments expect you to pass a non-optional value.
Copy link
Member

Choose a reason for hiding this comment

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

As I said, an optional parameter can expect an optional type in some cases, but I can't find a better wording and in 99% of the cases it's indeed a non optional value, so let's keep this for now!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We can add a condition to check that the field isn't expecting an option per se.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually, it'd be better if we could just adjust the wording to be generic enough to cover both cases. I'll try and think of something. Maybe @cknitt or @cristianoc has ideas?

Copy link
Collaborator

Choose a reason for hiding this comment

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

If the argument had option type we wouldn't have this error (except if we're passing nested when a single is expected).

Attempt: values passed to optional arguments don't need to be wrapped into an option.
You might need to adjust the type of the value supplied: avoid wrapping the value into an option, or unwrap the option from the value.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's great wording @cristianoc , thanks!


Possible solutions:
- Unwrap the option and pass a concrete value directly
Expand Down
2 changes: 1 addition & 1 deletion tests/build_tests/super_errors/expected/optional_record_field_pass_option.res.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
But the record field test is expected to have type: bool

test is an optional record field, and you're passing an optional value to it.
Optional fields expect you to pass the concrete value, not an option.
Optional fields expect you to pass a non-optional value.

Possible solutions:
- Unwrap the option and pass a concrete value directly
Expand Down