More error messages #7522

zth · 2025-05-26T11:59:55Z

A bunch of more error message improvements.

pkg-pr-new · 2025-05-26T12:06:48Z

rescript

npm i https://pkg.pr.new/rescript-lang/rescript@7522

@rescript/darwin-arm64

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

@rescript/darwin-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/darwin-x64@7522

@rescript/linux-arm64

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

@rescript/linux-x64

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

@rescript/win32-x64

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

commit: 1171e16

zth · 2025-05-30T18:51:46Z

compiler/ml/error_message_utils.ml

+  match p with
+  | Path.Pdot (Path.Pident {Ident.name = jsx_module_name}, "fragmentProps", _)
+    when Some jsx_module_name = !configured_jsx_module ->
+    Some {props_record_path = p; fields = []; jsx_type = `Fragment}


This logic isn't perfect, and at some point we probably need to figure out a better approach for matching on "what is a JSX component" in order to give better error messages. But for now with the state we're in, this is an improvement.

zth · 2025-05-30T18:51:58Z

compiler/ml/error_message_utils.ml

+    when Some jsx_module_name = !configured_jsx_module ->
+    Some {props_record_path = p; fields = []; jsx_type = `Fragment}
+  | _ -> (
+    (* TODO: handle lowercase components using JSXDOM.domProps *)


Leaving this for a follow up.

zth · 2025-05-30T19:03:06Z

@cknitt @tsnobip could you look over the messages/wording please? 🙏

@cristianoc could you have a general look at the code? I tried to make passing the context more explicit to make it simpler to follow the logic, and to add new cases as needed.

cristianoc · 2025-05-31T16:24:55Z

@cknitt @tsnobip could you look over the messages/wording please? 🙏

@cristianoc could you have a general look at the code? I tried to make passing the context more explicit to make it simpler to follow the logic, and to add new cases as needed.

The code looks good. Augmenting the error info with more details seems pretty robust.

cristianoc · 2025-06-01T09:29:37Z

compiler/ml/typecore.ml

@@ -2637,7 +2651,7 @@ and type_expect_ ?type_clash_context ?in_function ?(recarg = Rejected) env sexp
      }
  | Pexp_record (lid_sexp_list, Some sexp) ->
    assert (lid_sexp_list <> []);
-    let exp = type_exp ~recarg env sexp in
+    let exp = type_exp ~context:None ~recarg env sexp in


@zth this is where I believe one can pass context to special case error messages for e.g. optional fields.

cknitt

Nice work!
Here are some suggestions for the messages.

tests/build_tests/super_errors/expected/jsx_custom_component_children.res.expected

tests/build_tests/super_errors/expected/jsx_type_mismatch_float.res.expected

tests/build_tests/super_errors/expected/optional_fn_argument_pass_option.res.expected

cknitt · 2025-06-02T11:53:34Z

tests/build_tests/super_errors/expected/optional_fn_argument_pass_option.res.expected

+  But this optional function argument is expecting: [1;33mint[0m
+
+  You're passing an optional value into an optional function argument.
+  Optional function arguments expect you to pass the concrete value, not an option, when passed directly.


Not sure if this wording is easy to understand. Maybe we can omit the "when passed directly"?

tests/build_tests/super_errors/expected/optional_record_field_pass_option.res.expected

tsnobip

kudos @zth, great QoL improvements! Just a few comments, mostly nitpicking, so you can entirely decide to ignore them ^^

tsnobip · 2025-06-02T15:57:07Z

tests/build_tests/super_errors/expected/inline_types_record_type_params.res.expected

@@ -9,6 +9,6 @@
  15 [2m┆[0m otherExtra: Some({test: true, anotherInlined: {record: true}}),

  This has type: [1;31mint[0m
-  But it's expected to have type: [1;33mstring[0m
+  But the record field [1;33mage[0m is expected to have type: [1;33mstring[0m


hell yeah, there were way too many "this" and "it"s

tests/build_tests/super_errors/expected/jsx_type_mismatch_float.res.expected

tsnobip · 2025-06-02T16:19:28Z

tests/build_tests/super_errors/expected/jsx_custom_component_children.res.expected

+  25 [2m│[0m 
+
+  This has type: [1;31mfloat[0m
+  But children passed to this component are expected to have type:


I understand why @cknitt corrected "is" to "are", but I still find it a bit confusing to use the plural for something that can be singular so we could maybe use a more neutral verb

Suggested change

But children passed to this component are expected to have type:

But children passed to this component must be of have type:

or we could even keep the default wording:

Suggested change

But children passed to this component are expected to have type:

But the component prop children is expected to have type:

But definitely nitpicking here!

tsnobip · 2025-06-02T16:20:46Z

tests/build_tests/super_errors/expected/jsx_type_mismatch_float.res.expected

+  But children of JSX fragments are expected to have type:
+    [1;33mReact.element[0m [2m(defined as[0m [1;33mJsx.element[0m[2m)[0m


it's less true here since fragments only accept React.element children that can indeed be plural.

tsnobip · 2025-06-02T16:24:17Z

tests/build_tests/super_errors/expected/optional_fn_argument_pass_option.res.expected

+  But this optional function argument [1;33m~x[0m is expecting: [1;33mint[0m
+
+  You're passing an optional value into an optional function argument.
+  Optional function arguments expect you to pass the concrete value, not an option.


nitpicking again, but I'm not a big fan of concrete here, especially since sometimes, the concrete value that is expected can actually be an option, maybe use "unwrapped" instead? I won't start a war over this, it's really ok as is too.

No that's great! I'm really not sure how to word this to make it clear, so all suggestions are great.

tsnobip · 2025-06-02T16:27:00Z

tests/build_tests/super_errors/expected/try_catch_same_type.res.expected

+  This has type: [1;31mstring[0m
+  But this try/catch is expected to return: [1;33mint[0m
+
+  The [1;33mtry[0m body and the [1;33mcatch[0m block must return the same type.
+  To fix this, change your try/catch blocks to return the expected type.
+
+  You can convert [1;33mstring[0m to [1;33mint[0m with [1;33mInt.fromString[0m.


this is a great improvement!

zth · 2025-06-02T17:03:45Z

@tsnobip @cknitt I've tweaked the wording some. Could you have another look?

tsnobip

Looks good to me!

tsnobip · 2025-06-02T21:08:29Z

tests/build_tests/super_errors/expected/optional_fn_argument_pass_option.res.expected

+  But this optional function argument [1;33m~x[0m is expecting: [1;33mint[0m
+
+  You're passing an optional value into an optional function argument.
+  Optional function arguments expect you to pass a non-optional value.


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!

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

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?

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.

That's great wording @cristianoc , thanks!

…another syntax

zth force-pushed the refactor-error-msg-context branch from 9f79885 to b88d643 Compare May 27, 2025 07:43

zth commented May 30, 2025

View reviewed changes

zth changed the title ~~[WIP] Refactor error msg context~~ More error messages May 30, 2025

zth marked this pull request as ready for review May 30, 2025 19:02

zth requested review from cknitt, tsnobip and cristianoc May 30, 2025 19:03

cristianoc reviewed Jun 1, 2025

View reviewed changes

cknitt reviewed Jun 2, 2025

View reviewed changes

zth force-pushed the refactor-error-msg-context branch from 2bcdff7 to 86f63f3 Compare June 2, 2025 15:48

tsnobip approved these changes Jun 2, 2025

View reviewed changes

zth requested review from cknitt and tsnobip June 2, 2025 17:03

tsnobip approved these changes Jun 2, 2025

View reviewed changes

cknitt approved these changes Jun 3, 2025

View reviewed changes

zth added 10 commits June 3, 2025 10:50

refactor

1637911

explicit error for while condition

96ed8a3

explicit for loop condition

b80fea5

assert condition

6e5d149

refactor

9cd6bfd

fix

01288d3

try tracking record field type checking

374d563

iron out a few more error messages

ab01839

track optional function arguments

9411f97

format

3e8a1f7

zth added 6 commits June 3, 2025 10:50

cleanup

bb358ae

PR comments

28a3d38

update test output

0ca45e7

change wording a bit

33c4c2e

change wording

c29fc23

changelog

f2d3278

zth force-pushed the refactor-error-msg-context branch from 32ccee2 to f2d3278 Compare June 3, 2025 08:50

zth enabled auto-merge (squash) June 3, 2025 08:50

zth disabled auto-merge June 3, 2025 08:52

explicit error message for optional component props, since they have …

1171e16

…another syntax

zth enabled auto-merge (squash) June 3, 2025 09:01

zth merged commit a5b8f7a into master Jun 3, 2025
16 checks passed

zth deleted the refactor-error-msg-context branch June 3, 2025 09:10

	But children passed to this component are expected to have type:
	But children passed to this component must be of have type:

	But children passed to this component are expected to have type:
	But the component prop children is expected to have type:

		But children of JSX fragments are expected to have type:
		[1;33mReact.element[0m [2m(defined as[0m [1;33mJsx.element[0m[2m)[0m

More error messages #7522

More error messages #7522

Uh oh!

Conversation

zth commented May 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pkg-pr-new bot commented May 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zth commented May 30, 2025

Uh oh!

cristianoc commented May 31, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cknitt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tsnobip left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zth commented Jun 2, 2025

Uh oh!

tsnobip left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

zth commented May 26, 2025 •

edited

Loading

pkg-pr-new bot commented May 26, 2025 •

edited

Loading