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

PR#7169: Rephrase non-exhaustivity warning message to avoid confusion #501

Merged
merged 3 commits into from Jul 1, 2016

Conversation

Projects
None yet
4 participants
@Octachron
Contributor

Octachron commented Mar 9, 2016

This PR proposes to rephrase the warning for non-exhaustive patterns to make it clearer that counter-examples printed by the warning are patterns or cases and not values (that could be constructed).

For instance, with the current warning message, the following code

type t = { left:int ; right:int }
let f = function { left = 0; _ } -> ();;

raises a warning

Warning 8: this pattern-matching is not exhaustive.
Here is an example of a value that is not matched:                                     
{left=1}

This warning is confusing for beginners on at least two fronts. First, {left=1} is not a valid ocaml value. A beginner trying to see what happens when computing f {left=1} may be lost in the error raised by the compiler. The counter-example {left=1} is a pattern and should not be introduced as a value. Rewording the warning message as

Warning 8: this pattern-matching is not exhaustive.
Here is an example of a case that is not matched:                                     
{left=1}

solves this ambiguity without decreasing the information density of the warning.
This is the object of the first commit in this PR.

Second, the pattern {left=1} elides the field right without any indication that fields have been elided. Adding a trailing ; _ when fields have been elided would correct this lack of information and reinforces the notion that the counter-example is a pattern:

Warning 8: this pattern-matching is not exhaustive.
Here is an example of a value that is not matched:                                     
{left=1; _ }

The second commit in this PR modifies the pattern pretty printer in typing\parmatch.ml
to add this trailing _ when record patterns are not closed.

See also mantis:7169 for more discussions.

List.iter check_defined lbl_pat_list;
Some(all,defined)
let pretty_record_closure ppf lbl_list =

This comment has been minimized.

@gasche

gasche Mar 9, 2016

Member

I find the "closure" wording confusing (I think of function closures). You could maybe use "ending" instead?

This comment has been minimized.

@Octachron

Octachron Mar 9, 2016

Contributor

Maybe pretty_record_elision_mark to be more explicit?

This comment has been minimized.

@gasche

gasche Mar 9, 2016

Member

Yep, even better.

@gasche

This comment has been minimized.

Member

gasche commented Mar 9, 2016

Thanks a lot for this implementation work!

cc @maranget: he is the expert on the implementation aspect of parmatch, and I would like to know whether he agrees with the proposed change in phrasing of the warning.

@damiendoligez damiendoligez added this to the 4.04 milestone Mar 31, 2016

@alainfrisch

This comment has been minimized.

Contributor

alainfrisch commented Jul 1, 2016

Looks good to me. Could you please rebase (unfortunately, there are conflicts in the testsuite) and add an entry to Changes?

@Octachron

This comment has been minimized.

Contributor

Octachron commented Jul 1, 2016

Done: I have rebased the PR, squashed the fix commit, and added an entry to Changes.

@alainfrisch alainfrisch merged commit bed7e23 into ocaml:trunk Jul 1, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@alainfrisch

This comment has been minimized.

Contributor

alainfrisch commented Jul 1, 2016

Thanks!

@gasche

This comment has been minimized.

Member

gasche commented Jul 1, 2016

I'm worried that there may have been a problem during the rebase; I tested locally with intent to merge and the testsuite fails on my machine.

@gasche

This comment has been minimized.

Member

gasche commented Jul 1, 2016

So I reran the test from a clean state and the tests failure are gone. This may come from the fact that make world does not freshen some things exercized in the test (the toplevel?). Plus there is no issue for the CI machines, so sorry for the false alarm.

camlspotter pushed a commit to camlspotter/ocaml that referenced this pull request Oct 17, 2017

Merge pull request ocaml#501 from Octachron/non_exhaustivity_wording
PR#7169: Rephrase non-exhaustivity warning message to avoid confusion
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment