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

In or-patterns, propagate path disambiguation from one prefixed constructor to other constructors #7386

Closed
vicuna opened this issue Oct 14, 2016 · 8 comments

Comments

@vicuna
Copy link

vicuna commented Oct 14, 2016

Original bug ID: 7386
Reporter: @gasche
Status: acknowledged (set by @xavierleroy on 2017-02-23T16:06:20Z)
Resolution: open
Priority: normal
Severity: feature
Version: 4.03.0
Category: typing
Related to: #5759 #5980 #7443
Child of: #6951 #7409
Monitored by: @ygrek @jmeber @yallop @hcarty

Bug description

If we write a record pattern or expression with fields { a; M.b; c }, it is understood as equivalent to { M.a; M.b; M.c }. I would like this property to also hold of variant constructors appear in an or-pattern (or at the head of a list of clauses): (A | M.B | C) should be equivalent to (M.A | M.B | M.C), and

  | A -> ...
  | M.B -> ...
  | C -> ...

equivalent to

  | M.A -> ...
  | M.B -> ...
  | M.C -> ...

Additional information

Here is a concrete example I just had of a situation where this feature would be useful to me:

  let fill_cache article_map =
    let open Canopy_content in
    let fold_fn key value acc =
      value () >>= fun content ->
      date_updated_created key >>= fun (updated, created) ->
      let uri = String.concat "/" key in
      match of_string ~uri ~content ~created ~updated with
      | Ok article ->
        article_map := KeyMap.add key article !article_map;
        Lwt.return acc
      | Error error ->
        let error_msg = Printf.sprintf "Error while parsing %s: %s" uri error in
        Lwt.return (error_msg::acc)
      | Unknown ->
        let error_msg = Printf.sprintf "%s : Unknown content type" uri in
        Lwt.return (error_msg::acc)
    in
    new_task () >>= fun t ->
    fold (t "Folding through values") fold_fn []

This is code is from https://github.com/Engil/Canopy . When compiled from 4.03, it raises the following warnings:

  File "canopy_store.ml", line 78, characters 4-716:
  Warning 45: this open statement shadows the constructor Ok (which is later used)
  File "canopy_store.ml", line 78, characters 4-716:
  Warning 45: this open statement shadows the constructor Error (which is later used)

The best way I see to silence the warning is to write

      match of_string ~uri ~content ~created ~updated with
      | Canopy_content.Ok article ->
        article_map := KeyMap.add key article !article_map;
        Lwt.return acc
      | Canopy_content.Error error ->
        let error_msg = Printf.sprintf "Error while parsing %s: %s" uri error in
        Lwt.return (error_msg::acc)
      | Unknown ->
        let error_msg = Printf.sprintf "%s : Unknown content type" uri in
        Lwt.return (error_msg::acc)

but I would find it natural if it was possible to prefix only the first constructor.

We previously discussed this feature during the type-based-constructor/field-disambiguation debate, but the applicability scope I had in mind was too broad and with an unclear specification. I believe that "are in a single or-pattern" (or, as a generalization, at the head of the clauses of a matching) is a clear, simple and useful specification.

@vicuna
Copy link
Author

vicuna commented Oct 14, 2016

Comment author: @gasche

I realize now that another way to avoid this warning would be to not warn when type-directed disambiguation in fact selects a non-ambiguous candidate: this means that the syntactic ambiguity is in fact benign, as the types determine the only possible choice. That's a feature request for the ambiguity warning.

@vicuna
Copy link
Author

vicuna commented Oct 17, 2016

Comment author: @alainfrisch

  1. Another direction which would address this specific case is to disambiguate using all constructors "at the same position" (in your example, lookup a type containing the three constructors Ok/Error/Unknown), as we do for records. The definition of "at the same position" could be the same as the one used to propagate paths.

  2. Do you think that extending the notion of "at the same position" to traverse tuples would add too much complexity to this proposal? (i.e. support function (M.A, _) -> ... | (B, _) -> ...).

@github-actions
Copy link

This issue has been open one year with no activity. Consequently, it is being marked with the "stale" label. What this means is that the issue will be automatically closed in 30 days unless more comments are added or the "stale" label is removed. Comments that provide new information on the issue are especially welcome: is it still reproducible? did it appear in other contexts? how critical is it? etc.

@github-actions github-actions bot added the Stale label Mar 24, 2021
@gasche gasche removed the Stale label Mar 24, 2021
@gasche
Copy link
Member

gasche commented Mar 24, 2021

I still believe that "at the head of the same or-pattern" or "at the head of clauses" is a good criterion to propagate qualifying modules or do multi-label disambiguation, because it is simple, predictable, useful in several situations and consistent with what we do for record fields. I am putting "implementing this" on my medium-term TODO-list, please complain if you don't like the idea!

@jberdine
Copy link
Contributor

I think this would be valuable. This is something to which I've seen newcomers have a sort of "silly type-checker is just being intentionally obtuse" reaction. It seems to be run into reasonably frequently, though we tend to react by putting a type annotation on e in match e with rather than prefixing some of the constructors in the pattern.

@lpw25
Copy link
Contributor

lpw25 commented Mar 24, 2021

I'm a little worried about it because I assume that:

match e1, e2 with
| A, C -> ...
| M.B, D -> ...

will work, but:

match e2, e1 with
| C, A -> ...
| D, M.B -> ...

won't

@gasche
Copy link
Member

gasche commented Mar 24, 2021

With the simple criterion I proposed, neither would work, because they are not at the "head" of the pattern. Any extension that goes under constructor may run into trouble (in theory "at the same position under the same constructors" should work, but in fact because of GADTs it would only be correct in general to take the first argument of the constructor, giving the feeling of arbitrariness that Leo demonstrates in his example).

@github-actions
Copy link

This issue has been open one year with no activity. Consequently, it is being marked with the "stale" label. What this means is that the issue will be automatically closed in 30 days unless more comments are added or the "stale" label is removed. Comments that provide new information on the issue are especially welcome: is it still reproducible? did it appear in other contexts? how critical is it? etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants