Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Non-trivial sum type exhaustiveness checking #330
Non-trivial sum type exhaustiveness checking #330
Changes from 24 commits
fe0d5db
6483179
69e8804
a1932df
4705921
e5c9a7d
694e339
3ce4248
9b09365
c93f50b
1a99272
ca90d28
d21cb0e
92416c3
f4891e1
6d16234
7f9faf4
4b96d02
2f1dd53
d8673d0
d11c93e
bc63c5e
54d256a
a7e9053
7d2ca45
5e85d5b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there are multiple opaque types in a picked value, using the same variable name would imply all opaque values are equal, as in
{picked_tuple, _Opaque, _Opaque}
. That's why I suggested adding a unique integer to the name or use just a_
. Not a big deal though since this only for an error message. There is a point in not flooding the atom table as well.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, this might be slightly misleading if more than one opaque type is used... I wanted to avoid passing extra mappings from types to names, but it shouldn't be hard to add - I'll do it in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a need for passing around extra mappings? I was thinking just
Numbers might look a bit random in this way but at least they would be unique.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same type used twice doesn't mean it's the same value, so I think it would actually be wrong to use the same variable twice in this case. [Edit] Oh, well, since we're generating an example, using the same variable for the same type would be OK...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This probably doesn't even work with the map union directly, i.e. without the user type. A minimal example of the known problem would be using the union type directly, without a user type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't really looked for the reason of this not being detected - I'm aware that typing maps in general is not 100% correct yet, so it might be because of this PR or because of the existing code. I just wanted to add map variants for completeness' sake, since ordinary tuples and records are already taken care of.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really nice that you managed to test this stuff!