Conversation
Drup
left a comment
There was a problem hiding this comment.
I added a variant Empty_pattern in error type so that implementation can be easier and cleaner, although user will never encounter it (its error message).
having looked at the code, I got very confused at why this was an error, so I believe it would be better to keep an assert false there.
The rest is very simple and quite straightforward, since absurd patterns are already there.
This code is accepted, I do not know if this is good or bad.
type t = |
let f : t -> int = function _ -> 3 ;;|
Also, I think you should add a test that the following code passes without triggering exhaustiveness warning (currently it does, and it is cool :p). type t = |
let f : t option -> int = function None -> 3 |
|
@Drup : I am not sure if the behavior in your first example is surprising, since the same behavior already happens for any empty type: either type (_,_) eq = Eq:('a,'a) eq
type empty = (int,float) eqor type absurd = {x: 'a. 'a }yields the following unusable but accepted functions let f: empty -> int = function _ -> 3
let f : absurd -> int = function _ -> 3 |
|
@Octachron It isn't surprising. However, the typechecker knows that my Let's call that a potential feature request for our contributors that enjoy making good errors and warnings. :D |
|
The current design of the pattern-matching codebase is untyped (it does not take the type of the rows as input), which means that detecting that this case is unused is not an easy change. Having a design that propagates type information is reasonable in theory, and we might want to look at it eventually, but that's not a short-term change. |
|
@gasche Well, the exhaustiveness check does take type information into account. doesn't it ? I suppose this is done separately. |
|
@garrigue Exhaustiveness does work here, as you expect. It's reachability that is not complete. Given the presence of empty types, reachability of wildcards can benefits from type information just like exhaustiveness, but this is not done (yet!). All of this is not necessary to make this feature useful, and since the only opposition on the Mantis Issue is regarding the difficulty of implementing empty records, I propose we merge! |
|
@Drup Thanks for your review. I have address two of your comments --- add a test and use |
| | Illegal_letrec_expr | ||
| | Illegal_class_expr | ||
| | Unbound_value_missing_rec of Longident.t * Location.t | ||
| | Empty_pattern |
There was a problem hiding this comment.
We don't need the whole Empty_pattern thing anymore, right ?
| let (sp, constrs, labels) = | ||
| try | ||
| Parmatch.ppat_of_type !env expected_ty | ||
| with Parmatch.Empty -> raise (Error (loc, !env, Empty_pattern)) |
There was a problem hiding this comment.
@Drup Seems I misunderstood your review. Exception raise Error... sometimes is used to backtrack in this function. So I have to add a new variant Empty_pattern in error to use raise Err...
There was a problem hiding this comment.
I see, that's a bit inconvenient, but I don't have a better proposition, so I suppose it's fine.
|
@garrigue I would appreciate if you'd like to do a round of review of this PR. |
garrigue
left a comment
There was a problem hiding this comment.
The change seems fine, and has already been approved at the November developer's meeting.
Note however that this can break lots of stuff, so you need to test all the tools in the distribution, in particular ocamldoc, and also you need to update the manual to reflect this change.
manual/manual/refman/typedecl.etex
Outdated
| type-representation: | ||
| '=' ['|'] constr-decl { '|' constr-decl } | ||
| | '=' record-decl | ||
| | '|' |
|
Added the missing equal sign. I tested |
|
@Drup Actually it's not that it's not implemented for redundancy, but that it is explicitly disabled when there is only one case: |
garrigue
left a comment
There was a problem hiding this comment.
Sorry for the delayed request.
typing/parmatch.ml
Outdated
|
|
||
| let rec orify_many = function | ||
| | [] -> assert false | ||
| | [] -> raise Empty |
There was a problem hiding this comment.
Looking back at this code, I'm concerned by this exception.
orify_many is called in many places, and are you sure that they are all ready to catch Empty?
My feeling is that you should rather modify ppat_of_type to raise an exception when pats = [], without calling orify_many on the empty list. And this should be documented in the .mli.
There was a problem hiding this comment.
Good points. I realized this when I finished implementation. I checked all call sites and thought it should be fine.
Obviously it is better to raise in ppat_of_type. I have pushed the change.
There was a problem hiding this comment.
I have also added document in .mli
|
Just merged the PR. Thank you for all your revisions. |
|
I note that this PR allows |
|
Yep, fine with me! |
* Fix problems with character literals in comments * Do not parse unclosed comments and quoted strings * Update known failures * Only allow line number directives with filename (ocaml/ocaml#931) * Rename dot_operator to indexing_operator * Disallow .~ in indexing operator (ocaml/ocaml#2106) * Add test for indexing operator * Support empty variants (ocaml/ocaml#1546) * Support binding operators (ocaml/ocaml#1947) * Use tree-sitter 0.14.0 * Cleanup trvis config
Co-authored-by: Christine Rose <christinerose@users.noreply.github.com>
See Mantis 7583 for discussions.
This PR allows one to declare type with zero variants:
I added a variant
Empty_patterninerrortype so that implementation can be easier and cleaner, although user will never encounter it (its error message).