Skip to content

More informative error for mixed product arrays #4105

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.

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 5 commits into from
Jun 6, 2025

Conversation

rtjoa
Copy link
Contributor

@rtjoa rtjoa commented Jun 6, 2025

Give a more informative error message for product arrays whose elements are neither scannable nor external.

Old:

1 | let f_bad (x : #(string * float#)) = make_vect 42 x
                                         ^^^^^^^^^^^^^^
Error: Unboxed product array elements must be external or contain all gc
       scannable types. The product type this function is applied at is
       not external but contains an element of sort float64.

New:

1 | let f_bad (x : #(string * float#)) = make_vect 42 x
                                         ^^^^^^^^^^^^^^
Error: An unboxed product array element must be formed from all
       external types (which are ignored by the gc) or all gc-scannable types.
       But this array operation is peformed for an array whose
       element type is #(string * float#), which is an unboxed product
       that is not external and contains a type with the non-scannable
       layout float64.
       Hint: if the array contents should not be scanned, annotating
       contained abstract types as [mod external] may resolve this error.

@rtjoa rtjoa requested a review from ccasin June 6, 2025 19:38
@rtjoa rtjoa requested a review from goldfirere June 6, 2025 19:53
@ccasin
Copy link
Contributor

ccasin commented Jun 6, 2025

I'll just chime in to say I think I prefer "external" to "non-gc". The former is something with a concrete meaning - it appears in kinds, and that is what we are checking. It will also soon appear in modes. The latter is not used elsewhere, and seems imprecise to me.

Notably, this error message should not be trying to "contrast" these things at all. There is overlap (immediate) on purpose

@goldfirere
Copy link
Collaborator

OK. I'm fine with keeping external. But bonus points if the message can be phrased in a way that someone who doesn't know what that means can figure it out from context.

@ccasin
Copy link
Contributor

ccasin commented Jun 6, 2025

OK. I'm fine with keeping external. But bonus points if the message can be phrased in a way that someone who doesn't know what that means can figure it out from context.

I think you are suggesting adding a hint or something defining external, which sounds like a reasonable idea to me!

@rtjoa rtjoa enabled auto-merge (squash) June 6, 2025 21:02
@rtjoa rtjoa merged commit 55e92f8 into main Jun 6, 2025
32 checks passed
@rtjoa rtjoa deleted the rtjoa.better-mixed-product-array-error branch June 6, 2025 21:39
@mshinwell
Copy link
Collaborator

The whole point of my original comment here was that "external" seems confusing. I'd like to discuss this further.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants