BNFC / bnfc

BNF Converter
http://bnfc.digitalgrammars.com/
582 stars 161 forks source link

Printer for nonempty lists: "pattern matching is not exhaustive" warning in ocaml backend (suppressed in Haskell backend) #384

Closed JoAllg closed 2 years ago

JoAllg commented 2 years ago

Background: I want to have a list of Data category elements with at least two elements. The easiest working solution I found is

DPair. Data ::= "Pair" Pair ;
DPairSeq1.   Pair ::= Data Data ;
DPairSeq2.   Pair ::= Data Pair ;

However, here are two other ways i thought of, that cause a "pattern matching is not exhaustive" warning in the ocaml / ocaml-menhir backend:

DPair. Data ::= "Pair" Pair ;
terminator nonempty Pair "" ;
-- DPairSeq1.   Pair ::= Data Data ;
DPairSeq2. Pair ::= Data [Pair] ;

The full error message:

...
ocamllex LexMichelson.mll
28 states, 718 transitions, table size 3040 bytes
ocamlc  -o TestTest BNFC_Util.ml AbsTest.ml SkelTest.ml ShowTest.ml PrintTest.ml ParTest.mli ParTest.ml LexTest.ml TestTest.ml
File "PrintTest.ml", lines 91-93, characters 33-63:
91 | .................................match (i, es) with
92 |     (_,[x]) -> (concatD [prtPair 0 x])
93 |   | (_,x::xs) -> (concatD [prtPair 0 x ; prtPairListBNFC 0 xs])
Warning 8: this pattern-matching is not exhaustive.
Here is an example of a case that is not matched:
(_, [])

The second gramar that causes the same error:

DPair. Data ::= "Pair" Data [Pair] ;
terminator nonempty Pair "" ;
DPairSeq2. Pair ::= Data ;

The warning does not occur if nonempty is removed.

andreasabel commented 2 years ago
terminator nonempty Pair "" ;
File "PrintTest.ml", lines 91-93, characters 33-63:
91 | .................................match (i, es) with
92 |     (_,[x]) -> (concatD [prtPair 0 x])
93 |   | (_,x::xs) -> (concatD [prtPair 0 x ; prtPairListBNFC 0 xs])
Warning 8: this pattern-matching is not exhaustive.
Here is an example of a case that is not matched:
(_, [])

Since Pair lists cannot be empty, the case for empty lists cannot occur when trees are printed that were generated by the parser. So this warning can be ignored.

In #371 it is suggested that nonempty lists should be also represented as such in the AST. Haskell by now has a type for non-empty lists in the base library. I do not know how the situation is in Ocaml. In any case, this would be backwards-incompatible, so I am hesitant to make such a change.

One could of course silence the Ocaml warning by adding a case for the empty list that creates the empty document. This could be beneficial if the programmer wants to create empty lists in the AST (only not via parsing).

However, if the programmer rather wants to insist that lists there can indeed not be empty, then it is cleaner to leave the match partial so that the program crashes if an empty list should be encountered. (Well, maybe you do not want to crash in the printer...)

JoAllg commented 2 years ago

Ah sorry I thought it was a problem with my certain case and not generally connected to nonempty and ocaml.

One could implement a nonempty list, but that seem overkill for me for the case of the printer. May I suggest to add the case | [] -> failwith "Empty List"? I think that would satisfy the consideration you explained last and still allow for manuall modification for the other case.

andreasabel commented 2 years ago

The Haskell backend also creates an incomplete match, but suppresses the warning. The imperative backends (C, C++, Java) simply print nothing when the list is empty. Maybe we should do the same in the functional backends.

andreasabel commented 2 years ago

There is a related problem with lists of precedence categories:

terminator Exp2 "";
EInt.  Exp2 ::= Integer;

Generated printer in OCaml (same in Haskell):

87 | ................................match (i, es) with
88 |     (2,[]) -> (concatD [])
89 |   | (2,x::xs) -> (concatD [prtExp 2 x ; prtExpListBNFC 2 xs])
Warning 8: this pattern-matching is not exhaustive.
Here is an example of a case that is not matched:
(0, _)

This would be a bit harder to fix than the OP.

andreasabel commented 2 years ago

PR #385 would make the OP go away, but not https://github.com/BNFC/bnfc/issues/384#issuecomment-924813886. So I am not sure whether to merge this. Alternatively, maybe the warning could be switched off in OCaml.

JoAllg commented 2 years ago

As for disabling the warning in OCaml, to catch the case and throw an error seems to be the most straightforward way | (_, [] ) -> failwith "Error Message"

However apparently it is also possible to disable the warning just for this part of the code by adding [@warning "-8"] ... [@warning "+8"] like that:

(generated Printer in Ocaml)

and [@warning "-8"] prtPPairListBNFC  i  es  : doc [@warning "+8"] = match (i, es)  with
    (_,[x]) -> (concatD [prtPPair 0 x])
  | (_,x::xs) -> (concatD [prtPPair 0 x ; prtPPairListBNFC 0 xs])

(cf. https://stackoverflow.com/questions/46489249/use-ocaml-warning-attribute-to-disable-warning-8-unexhaustive-match )

andreasabel commented 2 years ago

Thanks for the research @JoAllg!

I am now also fixing the problem with the precedence category lists, so the pattern matches should be complete now for both OCaml and Haskell.