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

#8702: fix some polymorphic variant error messages #8777

Merged
merged 10 commits into from Jul 12, 2019

Conversation

@Octachron
Copy link
Contributor

commented Jun 28, 2019

This PR fixes a 4.08 regression reported in #8702 when mixing polymorphic variant and universal type variables

let foo : 'a . [`Foo] list -> ([> `Foo] as 'a) list =
    fun x -> x;;

Error: This expression has type [ `Foo ] list
       but an expression was expected of type [> `Foo ] list
       The second variant type does not allow tag(s) 

Along the way, it fixes a similarly confusing error message

type t = private [< `A]
let f (x:t): [> `A] =  x;;

Error: This expression has type t but an expression was expected of type
         [> `A ]
       Types for tag `A are incompatible

or

type any = Any: [< `A] -> any
let f (Any x): [ `A ] = x

Error: This expression has type [< `A ] but an expression was expected of type
         [ `A ]
       Types for tag `A are incompatible

Both confusing error messages stems from the fact that the error printer fails to notice that one of the row type involved was fixed (due to an universal type, a private type or a GADT).

This PR proposes to fix this issue by tracking in the types themselves the explanation for the fixed row. Then, the error message can print this explanation. For instance, the previous error messages become:

 Error: This expression has type [ `Foo ] list
       but an expression was expected of type [> `Foo ] list
       The second variant type is bound to the universal type variable 'a
Error: This expression has type t but an expression was expected of type
         [> `A ]
       The first variant type is private
Error: This expression has type [< `A ]
       but an expression was expected of type [ `A ]
       The first variant type is bound to $Any

Closes #8702

@trefis

This comment has been minimized.

Copy link
Contributor

commented Jul 4, 2019

I'm going to say something that will sound absurd as soon as you put things back in context but: I'm not a fan of the proposed error messages.
The existing ones are of course awful, but I don't feel like the new ones are really adding much explanation.

How about:

# type t = private [< `A]
  let f (x:t): [> `A] =  x;;
Error: This expression has type t but an expression was expected of type
         [> `A ]
       The first variant type is private and may not allow tag(s) `A

which is still not great, but is at least no worse than:

# let f (x: [< `A]) : [> `A | `B] = x;;
Error: This expression has type [< `A ]
       but an expression was expected of type [> `A | `B ]
       The first variant type does not allow tag(s) `B

I guess the other messages you change can be updated in the same fashion.


As for the implementation itself: I see a lot of row.row_fixed = None and row.row_fixed <> None. I feel like an is_fixed predicate (working on rows) would be slightly nicer (or you could pattern match more often).

@Octachron

This comment has been minimized.

Copy link
Contributor Author

commented Jul 5, 2019

I agree that keeping the tag information is a good idea. I have split the error message in two subcases:

let f: 'a. ( [< `A] as 'a) -> [` A] = fun x -> x;;
Error: This expression has type [< `A ]
       but an expression was expected of type [ `A ]
       The first variant type is bound to the universal type variable 'a,
       it may not allow the tag `A 

and

let f: 'a. [ `A | `B ] -> ([> `A ] as 'a) = fun x -> x
Error: This expression has type [ `A | `B ] but an expression was expected of type
        [> `A ]
      The second variant type is bound to the universal type variable 'a,
      it cannot be closed

I am wondering if the formulation is not too soft, and if we should replace may not with

it does not always allow the tag `A

On the implementation side, I removed the comparisons to None with a Btype.is_fixed function .

@Octachron Octachron force-pushed the Octachron:nonsense_mixed_error branch from 0902433 to b4a2dbf Jul 6, 2019
typing/btype.ml Outdated Show resolved Hide resolved
typing/btype.mli Show resolved Hide resolved
typing/ctype.ml Outdated Show resolved Hide resolved
typing/ctype.ml Outdated Show resolved Hide resolved
typing/ctype.ml Outdated Show resolved Hide resolved
typing/ctype.ml Outdated Show resolved Hide resolved
typing/ctype.ml Show resolved Hide resolved
typing/printtyp.ml Outdated Show resolved Hide resolved
@Octachron Octachron force-pushed the Octachron:nonsense_mixed_error branch from 242207b to a8bbb60 Jul 12, 2019
@trefis
trefis approved these changes Jul 12, 2019
Copy link
Contributor

left a comment

LGTM.

@trefis

This comment has been minimized.

Copy link
Contributor

commented Jul 12, 2019

I'll let you do the actual merge; if you don't want to bother cleaning up the history before merging then please do squash everything.

@Octachron

This comment has been minimized.

Copy link
Contributor Author

commented Jul 12, 2019

I don't really see any intermediary point in the commit history worth to be saved. Would you prefer to have a trio of {base,reviewed,Change} commits rather than a squashed commit?

@trefis

This comment has been minimized.

Copy link
Contributor

commented Jul 12, 2019

Nope, squash is fine.

@Octachron Octachron merged commit 6582335 into ocaml:trunk Jul 12, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Octachron Octachron deleted the Octachron:nonsense_mixed_error branch Jul 12, 2019
@trefis

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2019

This PR updates swap_elt to also handle variants, I think that part should be backported to 4.09 (this non-swapping is a regression in 4.08).

@Octachron

This comment has been minimized.

Copy link
Contributor Author

commented Aug 14, 2019

Good point, I will do so.

gasche added a commit that referenced this pull request Sep 11, 2019
4.09.0: backport the error message fix in #8777
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.