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

Fix false nonexhaustive record patterns warning #419

Merged

Conversation

erszcz
Copy link
Collaborator

@erszcz erszcz commented Jun 1, 2022

This stems from the research in #418 and supersedes any experiments done there. By dropping argument-wise exhaustiveness checking we get rid of the false positive warning and therefore go from:

$ make gradualize | wc -l
make: *** [gradualize] Error 1
     361

to

$ make gradualize | wc -l
make: *** [gradualize] Error 1
     307

@erszcz erszcz requested a review from zuiderkwast June 1, 2022 23:04
@erszcz erszcz force-pushed the fix-false-nonexhaustive-record-patterns-2 branch from b3c62a9 to 3a9b218 Compare June 1, 2022 23:05
@erszcz erszcz force-pushed the fix-false-nonexhaustive-record-patterns-2 branch from 3a9b218 to 5359c86 Compare June 1, 2022 23:08
@erszcz erszcz marked this pull request as draft June 1, 2022 23:10
@erszcz
Copy link
Collaborator Author

erszcz commented Jun 1, 2022

Some comments need fixing, so I'm converting this to a draft for now.

@berbiche
Copy link
Contributor

berbiche commented Jun 2, 2022

Neat, this seems better than #411

I will try the PR in the upcoming days 👍

edit: see #414 (comment)

Copy link
Collaborator

@zuiderkwast zuiderkwast left a comment

Choose a reason for hiding this comment

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

Looks OK, but the one below worked before and now it's move to known problems (should fail). Do you think it is less important than the added test case? Can you think of a way to solve both?

-type t() :: ala | ola.

-spec f(t(), any()) -> ok.
f(ala, _) -> ok.

@erszcz
Copy link
Collaborator Author

erszcz commented Jun 2, 2022

Do you think it is less important than the added test case?

Yes, I think it's less important. We get rid of a very annoying and very common false positive warning that way.

Can you think of a way to solve both?

I don't know how to do that and I'm unsure if it's possible at all without really significant changes to the typechecker. The long explanation is in #418, together with some approaches I tried, but to no success :/

@erszcz erszcz force-pushed the fix-false-nonexhaustive-record-patterns-2 branch from 5359c86 to 74a98cc Compare June 2, 2022 11:40
@erszcz erszcz marked this pull request as ready for review June 2, 2022 11:40
@zuiderkwast
Copy link
Collaborator

Then I think we can document that we do exhaustiveness if and only if all of the parameters are typed, not any(). Does it make sense? It's good to have a clear rule to follow.

Copy link
Collaborator

@zuiderkwast zuiderkwast left a comment

Choose a reason for hiding this comment

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

OK, feel free to merge when ready.

The edoc comments are mainly for internal use. The user documentation (as far as we have any) is on the wiki so we can consider updating that too.

@erszcz erszcz merged commit 002f5f7 into josefs:master Jun 3, 2022
@erszcz erszcz deleted the fix-false-nonexhaustive-record-patterns-2 branch June 3, 2022 06:34
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.

3 participants