Skip to content

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

Merged
alainfrisch merged 3 commits intoocaml:trunkfrom
Octachron:non_exhaustivity_wording
Jul 1, 2016
Merged

PR#7169: Rephrase non-exhaustivity warning message to avoid confusion#501
alainfrisch merged 3 commits intoocaml:trunkfrom
Octachron:non_exhaustivity_wording

Conversation

@Octachron
Copy link
Copy Markdown
Member

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 =
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Maybe pretty_record_elision_mark to be more explicit?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yep, even better.

@gasche
Copy link
Copy Markdown
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
Copy link
Copy Markdown
Contributor

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

@Octachron Octachron force-pushed the non_exhaustivity_wording branch from 389cc3b to d34a05b Compare July 1, 2016 13:59
@Octachron
Copy link
Copy Markdown
Member Author

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
@alainfrisch
Copy link
Copy Markdown
Contributor

Thanks!

@gasche
Copy link
Copy Markdown
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
Copy link
Copy Markdown
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
PR#7169: Rephrase non-exhaustivity warning message to avoid confusion
EduardoRFS pushed a commit to esy-ocaml/ocaml that referenced this pull request May 17, 2021
Keryan-dev pushed a commit to Keryan-dev/ocaml that referenced this pull request Jun 28, 2021
stedolan pushed a commit to stedolan/ocaml that referenced this pull request Mar 22, 2022
* Revert "Revert bswap PRs (480 and 482) (ocaml#501)"

This reverts commit 52cd50e.

* Test for bswap32

* Add more test inputs

* Refactor test

* Add more tests
stedolan pushed a commit to stedolan/ocaml that referenced this pull request May 24, 2022
stedolan pushed a commit to stedolan/ocaml that referenced this pull request May 24, 2022
* Revert "Revert bswap PRs (480 and 482) (ocaml#501)"

This reverts commit 52cd50e.

* Test for bswap32

* Add more test inputs

* Refactor test

* Add more tests
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.

4 participants