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

Support empty variants #1546

Merged
merged 21 commits into from Mar 6, 2018

Conversation

Projects
None yet
6 participants
@objmagic
Copy link
Contributor

objmagic commented Dec 25, 2017

See Mantis 7583 for discussions.

This PR allows one to declare type with zero variants:

type t = |
let f (x:t) = match x with _ -> .

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).

@Drup

Drup approved these changes Jan 28, 2018

Copy link
Contributor

Drup left a 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 ;;
@Drup

This comment has been minimized.

Copy link
Contributor

Drup commented Jan 28, 2018

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 
@Octachron

This comment has been minimized.

Copy link
Contributor

Octachron commented Jan 28, 2018

@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) eq

or

type absurd  = {x: 'a. 'a }

yields the following unusable but accepted functions

let f: empty -> int = function _ -> 3
let f : absurd -> int = function _ -> 3
@Drup

This comment has been minimized.

Copy link
Contributor

Drup commented Jan 28, 2018

@Octachron It isn't surprising. However, the typechecker knows that my t or your empty is not inhabited, and could use that information to warn the user. The typechecker doesn't know that absurd is not inhabited.

Let's call that a potential feature request for our contributors that enjoy making good errors and warnings. :D

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Jan 29, 2018

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.

@Drup

This comment has been minimized.

Copy link
Contributor

Drup commented Jan 29, 2018

@gasche Well, the exhaustiveness check does take type information into account. doesn't it ? I suppose this is done separately.

@garrigue

This comment has been minimized.

Copy link
Contributor

garrigue commented Jan 30, 2018

@gasche, @Drup The exhaustiveness check does a second pass on the missing cases, checking whether they are really inhabited using the type checker. I would expect it to ensure exhaustiveness here for free. If this is not the case, something has been overlooked.

@Drup

This comment has been minimized.

Copy link
Contributor

Drup commented Jan 30, 2018

@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!

@objmagic

This comment has been minimized.

Copy link
Contributor Author

objmagic commented Feb 4, 2018

@Drup Thanks for your review. I have address two of your comments --- add a test and use assert false

@@ -97,6 +97,7 @@ type error =
| Illegal_letrec_expr
| Illegal_class_expr
| Unbound_value_missing_rec of Longident.t * Location.t
| Empty_pattern

This comment has been minimized.

@Drup

Drup Feb 4, 2018

Contributor

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))

This comment has been minimized.

@objmagic

objmagic Feb 4, 2018

Author Contributor

@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...

This comment has been minimized.

@Drup

Drup Feb 5, 2018

Contributor

I see, that's a bit inconvenient, but I don't have a better proposition, so I suppose it's fine.

@objmagic

This comment has been minimized.

Copy link
Contributor Author

objmagic commented Feb 26, 2018

@garrigue I would appreciate if you'd like to do a round of review of this PR.

@garrigue
Copy link
Contributor

garrigue left a 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.

@@ -31,6 +31,7 @@ type-equation:
type-representation:
'=' ['|'] constr-decl { '|' constr-decl }
| '=' record-decl
| '|'

This comment has been minimized.

@garrigue

garrigue Mar 4, 2018

Contributor

missing = ?

@objmagic

This comment has been minimized.

Copy link
Contributor Author

objmagic commented Mar 4, 2018

Added the missing equal sign.

I tested -dparsetree, -dtypedtree, -dsource, -i, and ocamldoc (html and latex). They all work as expected. If I am missing something here, please feel free to point out.

@garrigue

This comment has been minimized.

Copy link
Contributor

garrigue commented Mar 5, 2018

@Drup Actually it's not that it's not implemented for redundancy, but that it is explicitly disabled when there is only one case: (not refute && pref = []). IIRC, the main goal was to avoid warnings for other syntactic constructs, such as let definitions or function formal arguments. We might either remove this restriction, or refine it according to the syntactic form (but that information is not propagated currently).

@garrigue
Copy link
Contributor

garrigue left a comment

Sorry for the delayed request.

@@ -897,7 +897,7 @@ let pat_of_constr ex_pat cstr =
let orify x y = make_pat (Tpat_or (x, y, None)) x.pat_type x.pat_env

let rec orify_many = function
| [] -> assert false
| [] -> raise Empty

This comment has been minimized.

@garrigue

garrigue Mar 5, 2018

Contributor

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.

This comment has been minimized.

@objmagic

objmagic Mar 5, 2018

Author Contributor

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.

This comment has been minimized.

@objmagic

objmagic Mar 6, 2018

Author Contributor

I have also added document in .mli

objmagic added some commits Mar 5, 2018

@garrigue garrigue merged commit 0993cd9 into ocaml:trunk Mar 6, 2018

2 checks passed

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

This comment has been minimized.

Copy link
Contributor

garrigue commented Mar 6, 2018

Just merged the PR. Thank you for all your revisions.

@fpottier

This comment has been minimized.

Copy link
Contributor

fpottier commented Oct 3, 2018

I note that this PR allows type t = | | A of int, which is taken to mean the same thing as type t = | A of int. In my eyes, this is harmless, but undesirable. I propose to fix it as part of the parser cleanup on which I am currently working.

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Oct 3, 2018

Yep, fine with me!

314eter added a commit to 314eter/tree-sitter-ocaml that referenced this pull request Jan 15, 2019

314eter added a commit to 314eter/tree-sitter-ocaml that referenced this pull request Jan 19, 2019

maxbrunsfeld added a commit to tree-sitter/tree-sitter-ocaml that referenced this pull request Feb 7, 2019

Support new features (#22)
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.