Start work on improving subtype error messages #7404

zth · 2025-04-24T08:16:31Z

This improves the subtype error messages to make them understandable/actionable where relevant.

In this first iteration I've focused on adding optional context to each subtype error. We use that to, where possible, provide better subtype errors, that are actionable.

This does not cover all cases, and there's plenty of follow ups (some listed below). But, it's a start, and validates the concept that we can do this this way.

Follow ups

Will put these in a separate issue to be tracked

Variant constructor inline record payloads
Variant constructor tuple payloads
Record representation
Polyvariant :> variant

Then there's the larger follow up of tracking where in a nested subtype check an error is happening. Example: Records in records, where a record further down the type structure is not a subtype.

zth · 2025-05-23T11:21:35Z

compiler/ml/ctype.ml

+      issues: Record_coercion.record_field_subtype_violation list;
+    }
+
+exception Subtype of type_pairs * type_pairs * subtype_context option


Adding subtype_context is the real change here. It tries to track the context a subtype issue was found in, so better error messages can be presented.

zth · 2025-05-23T11:21:59Z

compiler/ml/ctype.ml

      | Tarrow (l1, t1, u1, _, _), Tarrow (l2, t2, u2, _, _)
        when Asttypes.Noloc.same_arg_label l1 l2 ->
        let cstrs = subtype_rec env ((t2, t1) :: trace) t2 t1 cstrs in
        subtype_rec env ((u1, u2) :: trace) u1 u2 cstrs
-      | Ttuple tl1, Ttuple tl2 -> subtype_list env trace tl1 tl2 cstrs
+      | Ttuple tl1, Ttuple tl2 ->
+        (* TODO(subtype-errors) Tuple as context *)


Can leave these TODO:s in here since I'll do another pass on this to extend it more soonish.

pkg-pr-new · 2025-05-23T11:35:43Z

Open in StackBlitz

rescript

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

@rescript/darwin-arm64

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

@rescript/darwin-x64

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

@rescript/linux-arm64

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

@rescript/linux-x64

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

@rescript/win32-x64

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

commit: 2d3efd5

zth · 2025-05-23T12:03:47Z

@cristianoc would you mind having a look at the general approach here, to make sure it's good enough?

@tsnobip @cknitt would you mind looking at the error messages, like you have in the previous PRs? 🙏

tsnobip

@zth doing God's work! Keep them coming!

tsnobip · 2025-05-23T13:21:29Z

tests/build_tests/super_errors/expected/record_coercion_optional_mismatch.res.expected

+  The record [1;33mx[0m cannot be coerced to the record [1;33my[0m because:
+  - The field [1;33mx[0m is optional in record [1;33mx[0m, but is not optional in record [1;33my[0m


the error message is very detailed and insightful, nice! That reminds me that the other way around is not yet possible unfortunately, you can't coerce to a type that has more optional fields than the original type.

module Foo = { type t = {foo: int} } module Bar = { type t = {foo: int, bar?: int} } let foo = {Foo.foo: 1} let bar = (foo :> Bar.t) // error

Would that be reasonable to support?

definitely low-priority but I once had the need for such a thing and I can't think of any argument against it.

@cristianoc you see any issues with supporting it?

I'd forgotten about it, but the reason is now coming back: transitivity.

Since {foo:int, bar:string} :> {foo:int}, it means that Foo.t says nothing about bar. It does not say that bar should not be there.
So the conversion Foo.t :> Bar.t would be dangerous as it could accidentally capture a value that is already in bar and that value does not have to be an int either, it could be a string or anything.

The only safe way to do that coercion would be to add some runtime code, which removes the field bar. Type coercion is currently type-only. So we'd have to change the nature of type coercion, in a way that goes beyond the type checking phase.

Right, got it! So for regular coercion, it's a no go. At some point we talked very loosely about "runtime conversion" (maybe with its own operator like ::>) as a concept. The discussions were in the context of JSON. This would be another place where that type of thing could be useful.

compiler/ml/ast_untagged_variants.ml

cknitt · 2025-05-23T14:04:42Z

tests/build_tests/super_errors/expected/record_coercion_as_payload_mismatch_both.res.expected

+  Type x is not a subtype of y 
+
+  The record [1;33mx[0m cannot be coerced to the record [1;33my[0m because:
+  - Field [1;33mx[0m runtime representation is configured to be [1;33m"z"[0m (via the @as attribute) in record [1;33my[0m, but in record [1;33mx[0m it is configured to be [1;33m"w"[0m (via the @as attribute). Runtime representations must match.


This is really nice!

cristianoc

The approach looks great!
Left some comments.

cristianoc · 2025-05-26T07:44:50Z

compiler/ml/printtyp.ml

+             @{<info>@unboxed@} attribute to the variant @{<info>%s@}."
+            (Path.name variant_name) (Path.name primitive)
+            (Path.name variant_name)
+        | Primitive_coercion_target_variant_no_catch_all


Not sure about the terminology catch-all as well as primitive in error messages.
I think the former means: a case with a payload of that type. While "catch-all" is more reminiscent of _ in pattern matching or 'a in types.
And for primitive: I think it just refers to "a type".

Yeah good point. I was struggling to come up with good, clear names. I'll do another pass at it.

cristianoc · 2025-05-26T07:53:41Z

tests/build_tests/super_errors/expected/record_coercion_optional_mismatch.res.expected

+  The record [1;33mx[0m cannot be coerced to the record [1;33my[0m because:
+  - The field [1;33mx[0m is optional in record [1;33mx[0m, but is not optional in record [1;33my[0m


I'd forgotten about it, but the reason is now coming back: transitivity.

Since {foo:int, bar:string} :> {foo:int}, it means that Foo.t says nothing about bar. It does not say that bar should not be there.
So the conversion Foo.t :> Bar.t would be dangerous as it could accidentally capture a value that is already in bar and that value does not have to be an int either, it could be a string or anything.

The only safe way to do that coercion would be to add some runtime code, which removes the field bar. Type coercion is currently type-only. So we'd have to change the nature of type coercion, in a way that goes beyond the type checking phase.

cristianoc · 2025-05-26T07:56:22Z

tests/build_tests/super_errors/expected/variant_coercion_inline_record.res.expected

+
+  Type x is not a subtype of string 
+
+  The constructor [1;33mOne[0m of variant [1;33mx[0m has an inline record as payload. Inline records cannot be coerced.


perhaps add "currently" as there's no in-principle reason why inline records could not be coerced in future

cristianoc · 2025-05-26T08:00:22Z

compiler/ml/ast_untagged_variants.ml

@@ -71,6 +71,17 @@ type block_type =
  | ObjectType
  | UnknownType

+let block_type_to_string = function


I first thought this was for debug, but it's for user-visible error messages.
Should probably clarify in the function name or comments what role this print plays. If it's about the runtime representation, then it should say "number". If it's about the rescript type then int.

cristianoc · 2025-05-26T08:02:24Z

compiler/ml/printtyp.ml

+    fprintf ppf
+      "The constructor @{<info>%s@} of variant @{<info>%s@} has an \
+       @{<info>@as@} payload that has a runtime representation of \
+       @{<info>%s@}, which is not compatible with the expected of \


"with the expected of" has some typo in it

This reverts commit 55476b9.

zth force-pushed the better-subtype-error-messages branch 2 times, most recently from 55476b9 to 1093261 Compare May 23, 2025 11:02

zth commented May 23, 2025

View reviewed changes

zth changed the title ~~[WIP] Improve subtype error messages~~ Start work on improving subtype error messages May 23, 2025

zth force-pushed the better-subtype-error-messages branch from fcffc7e to e3a2b95 Compare May 23, 2025 12:02

zth marked this pull request as ready for review May 23, 2025 12:02

zth requested review from cristianoc, cknitt and tsnobip and removed request for cristianoc May 23, 2025 12:03

tsnobip approved these changes May 23, 2025

View reviewed changes

cknitt reviewed May 23, 2025

View reviewed changes

cristianoc approved these changes May 26, 2025

View reviewed changes

zth added 13 commits May 26, 2025 11:44

structure for improving subtype errors

71d27f1

variant configuration errors

8faca07

different constructor counts

c77d475

format

6c80c90

update test output

634a1f8

different type kinds

9032051

missing fn

9c033ac

record fields mismatch

f0ef8d1

more hints about what types we are looking at

8faefd4

Revert "more hints about what types we are looking at"

46938e3

This reverts commit 55476b9.

refactor

7b62cb0

format

ab8d62e

cleanup

2c82760

zth added 5 commits May 26, 2025 11:44

expand comments tracking work left to do

42fa818

add comments

b6d7b28

changelog

ea5adae

change fn name

3058798

pr comments

2d3efd5

zth force-pushed the better-subtype-error-messages branch from c2a1805 to 2d3efd5 Compare May 26, 2025 09:44

zth merged commit 6f4275c into master May 26, 2025
16 checks passed

zth deleted the better-subtype-error-messages branch May 26, 2025 10:06

		The record [1;33mx[0m cannot be coerced to the record [1;33my[0m because:
		- The field [1;33mx[0m is optional in record [1;33mx[0m, but is not optional in record [1;33my[0m


		Type x is not a subtype of string

		The constructor [1;33mOne[0m of variant [1;33mx[0m has an inline record as payload. Inline records cannot be coerced.

Start work on improving subtype error messages #7404

Start work on improving subtype error messages #7404

Uh oh!

Conversation

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

Uh oh!

Follow ups

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

zth commented May 23, 2025

Uh oh!

tsnobip left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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!

Choose a reason for hiding this comment

Uh oh!

cristianoc 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

zth commented Apr 24, 2025 •

edited

Loading

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

edited

Loading

tsnobip May 23, 2025 •

edited

Loading

tsnobip May 23, 2025 •

edited

Loading