Skip to content

Conversation

fhammerschmidt
Copy link
Member

@fhammerschmidt fhammerschmidt commented Sep 2, 2025

The int overflow stuff is actually for polyvariants.

Copy link
Member

@zth zth left a comment

Choose a reason for hiding this comment

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

Nice! Could you please make sure you add error message fixture tests to the super_errors folder? So we cover all error messages we have with fixture tests.

Copy link

pkg-pr-new bot commented Sep 2, 2025

Open in StackBlitz

rescript

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

@rescript/darwin-arm64

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

@rescript/darwin-x64

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

@rescript/linux-arm64

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

@rescript/linux-x64

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

@rescript/runtime

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

@rescript/win32-x64

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

commit: 40f90a7

@fhammerschmidt fhammerschmidt force-pushed the some-error-message-improvements branch from b52a874 to 40f90a7 Compare September 2, 2025 08:51
@fhammerschmidt fhammerschmidt requested a review from zth September 2, 2025 08:57
@fhammerschmidt fhammerschmidt enabled auto-merge (squash) September 2, 2025 09:19
Copy link
Member

@tsnobip tsnobip left a comment

Choose a reason for hiding this comment

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

clearer error messages, thanks @fhammerschmidt !

@fhammerschmidt
Copy link
Member Author

Awaiting the build running through and apparently another approvement by @zth then it's good to go.

@@ -85,7 +85,9 @@ let emit_external_warnings : iterator =
( Nonrecursive,
[{ptype_kind = Ptype_variant ({pcd_res = Some _} :: _)}] ) ->
Location.raise_errorf ~loc:str_item.pstr_loc
"GADT has to be recursive types, please try `type rec'"
"GADTs require recursive type syntax.\n\
Please define your type using `type rec` instead of `type`.\n\
Copy link
Member

Choose a reason for hiding this comment

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

I think we should avoid saying "please" here, we don't do that in other error messages either.

Copy link
Member

@zth zth left a comment

Choose a reason for hiding this comment

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

Just saw the int message is for polyvariants, so in the current state none of the suggested fixes are applicable to polyvariants per se (cannot use float nor bigint). I think we can merge anyway since it's better than before. And, there's an issue to add float support to polyvariants, that should at some point be implemented.

@fhammerschmidt fhammerschmidt merged commit 5cc3938 into master Sep 3, 2025
36 of 37 checks passed
@zth zth deleted the some-error-message-improvements branch September 3, 2025 06:40
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.

5 participants