Skip to content
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

Improve Unclear error FS3204 Multicase union struct #14105

Conversation

edgarfgp
Copy link
Contributor

@edgarfgp edgarfgp commented Oct 13, 2022

Fixes #3648

Problem

  • Existing error is not necessarily wrong. But should be optimized for a person who did not see it before, and maybe even for a person who might not know that fields in DUs can have optimal names.
  • The implementation uses the type range and shows only one error that covers all the errors in Multicase DU

Poposal

  • Update the error message to:
    If a union type has more than one case and is a struct, then all fields within the union type must be given unique field names. For example: 'type A = B of b: int | C of c: int' (unique field names 'b' and 'c' assigned). based @T-Gro suggestion

  • Use the field range to report the error in all the fields that does not have a compiler autogenerated name "Item"

Update : Would be good to put his in a preview lang flag

@edgarfgp edgarfgp changed the title Improve Unclear error FS3204 Multicase union case struct Improve Unclear error FS3204 Multicase union struct Oct 13, 2022
…truct-then-all-fields-within-the-union-type-must-be-given-unique-names
…all-fields-within-the-union-type-must-be-given-unique-names' of https://github.com/edgarfgp/fsharp into union-type-has-more-than-one-case-and-is-a-struct-then-all-fields-within-the-union-type-must-be-given-unique-names
@edgarfgp edgarfgp marked this pull request as ready for review October 14, 2022 12:03
@edgarfgp edgarfgp force-pushed the union-type-has-more-than-one-case-and-is-a-struct-then-all-fields-within-the-union-type-must-be-given-unique-names branch from 870c56d to 56c28c0 Compare October 14, 2022 13:01
@edgarfgp edgarfgp force-pushed the union-type-has-more-than-one-case-and-is-a-struct-then-all-fields-within-the-union-type-must-be-given-unique-names branch from 56c28c0 to 77e1a72 Compare October 14, 2022 13:41
@edgarfgp
Copy link
Contributor Author

There are some baseline tests failing. I will wait for feedback about this proposal before I update all :)

@T-Gro
Copy link
Member

T-Gro commented Oct 14, 2022

Good to see this, thank you @edgarfgp . Added a few comments about scenarios where I would like to maintain backwards compat. with what was compiling until now.

( We do not have to be strict about code that has errors even before. But for working code, I would prefer not to introduce breaking changes unless we have to)

@edgarfgp edgarfgp requested a review from T-Gro October 15, 2022 12:41
…truct-then-all-fields-within-the-union-type-must-be-given-unique-names
…truct-then-all-fields-within-the-union-type-must-be-given-unique-names
src/Compiler/FSComp.txt Outdated Show resolved Hide resolved
@edgarfgp edgarfgp requested review from 0101, T-Gro and psfinaki and removed request for T-Gro and 0101 October 17, 2022 13:13
@T-Gro T-Gro enabled auto-merge (squash) October 17, 2022 13:52
Copy link
Member

@psfinaki psfinaki left a comment

Choose a reason for hiding this comment

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

Great job, again

@T-Gro T-Gro self-requested a review October 17, 2022 15:26
@T-Gro
Copy link
Member

T-Gro commented Oct 17, 2022

The error message was good before, but now it is not.
The condition is having about unique FIELD NAMES within the cases , with the special focus on making sure fields of the cases can have names (not everyone knows/uses named fields) and not to confuse them with the name of the case (which was a complaint about the existing message).

In the new version, the fact that this is about FIELDs has disappeared.

@psfinaki
Copy link
Member

If a multicase union type is a struct, then all union cases must have unique FIELD names. For example: "type A = B of b: int | C of c: int".

@T-Gro how about that? I wanted to remove the tautology in the previous version and ended up removing too much :D

@edgarfgp
Copy link
Contributor Author

edgarfgp commented Oct 17, 2022

The error message was good before, but now it is not. The condition is having about unique FIELD NAMES within the cases , with the special focus on making sure fields of the cases can have names (not everyone knows/uses named fields) and not to confuse them with the name of the case (which was a complaint about the existing message).

In the new version, the fact that this is about FIELDs has disappeared.

I think both are ok for me :) as new we are also using the field ranges . Let me know what is the final error message and I will update it

@dsyme
Copy link
Contributor

dsyme commented Oct 17, 2022

I'm happy to see this being improved!

…truct-then-all-fields-within-the-union-type-must-be-given-unique-names
…truct-then-all-fields-within-the-union-type-must-be-given-unique-names
@T-Gro T-Gro merged commit ea04308 into dotnet:main Oct 18, 2022
@edgarfgp
Copy link
Contributor Author

@T-Gro Thanks for merging this :)

@voronoipotato
Copy link
Contributor

Thank you!!! this is huge as the previous error lacked an example demonstrating what it means in this context to name the cases, and how to do it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
7 participants