Skip to content

13034 error messages: mismatched labels#13260

Merged
Octachron merged 5 commits intoocaml:trunkfrom
Octachron:label_mismatch_errors
Jul 5, 2024
Merged

13034 error messages: mismatched labels#13260
Octachron merged 5 commits intoocaml:trunkfrom
Octachron:label_mismatch_errors

Conversation

@Octachron
Copy link
Member

@Octachron Octachron commented Jun 25, 2024

This PR adds a specialized explanation for mismatched function labels in type error messages.
For instance, in

module Label1 : sig
  val f: int -> unit
end = struct
  let f ~x = ()
end

the existing error message

      Modules do not match:
        sig val f : x:'a -> unit end
      is not included in
        sig val f : int -> unit end
      Values do not match:
        val f : x:'a -> unit
      is not included in
        val f : int -> unit
      The type "x:'a -> unit" is not compatible with the type "int -> unit"

is extended with

      The argument "x" is labeled, but an unlabeled argument was expected

This complements the existing specialized error message for syntactic argument with a label incompatible with the expected type.

close #13034

typing/ctype.ml Outdated
| false, false -> link_commu ~inside:c1 c2
| true, true -> ()
end else
raise_for Unify (Function_label_mismatch {got=l1; expected=l2})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: wouldn't this be easier on the eye with the error case first?

if not (compatible_labels ...) then
  raise_for ...;
unify uenv t1 t2; unify uenv u1 u2;
begin match ... with
  ...
end

(Same question in the other, simpler cases.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Woudn't it be even simpler if we moved the raise inside the compatible_labels function ?
This would reduce code duplication (and even more if more label comparison are added).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are calls to compatible_labels in mcomp and the subtyping mode that doesn't raise the exception, either because we are only checking compatibility or because we are storing an error trace for later use.

However, we could have a raise_on_incompatible_labels function which has the advantage of keeping the control flow obvious.

Copy link
Member

@gasche gasche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am uneasy with the wording The optional argument "?x", because ?x is a label and not an argument`. I proposed some reformulations that avoid this issue. Also in the case of a mismatch between an optional and a non-optional label, I propose to simplify with a more approximative but shorter error message. Feel free to adapt the suggestions or discard them outright.

is not included in
type t = int -> int
The type "x:int -> int" is not equal to the type "int -> int"
The argument "x" is labeled, but an unlabeled argument was expected
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Proposal: The argument has label "x", but ....

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Counterproposal: The first argument is labeled "x"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine with me.

is not included in
val f : int -> unit
The type "x:'a -> unit" is not compatible with the type "int -> unit"
The argument "x" is labeled, but an unlabeled argument was expected
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The argument has label "x", ...`

is not included in
val f : int -> unit
The type "?x:'a -> unit" is not compatible with the type "int -> unit"
The argument "?x" is optional, but an unlabeled argument was expected
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The argument has label ?x, ....

is not included in
val f : x:int -> unit
The type "?x:'a -> unit" is not compatible with the type "x:int -> unit"
The optional argument "?x" was not expected to be optional
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Labels "?x" and "x" do not match.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree with repeating twice the same name with one minute change.
I propose: Label "?x" was expected to not be optional.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, what would the error be on a mismatch between ?x:int -> unit and y:int -> unit? I assumed that the use of x on both sides was irrelevant to the current situation and wording.

Copy link
Member Author

@Octachron Octachron Jul 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case, Labels "?x" and "y" do not match seems a good simplification to me (the previous error message was The optional argument "?x" was expected to be a labeled argument named "y")

is not included in
val f : ?x:int -> unit
The type "'a -> unit" is not compatible with the type "?x:int -> unit"
An optional argument "?x" was expected
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A label "?x" was expected.

but an expression was expected of type "unit -> unit"
The argument "x" is labeled, but an unlabeled argument was expected
The first argument is labeled "x",
but an unlabeled argument was expected
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Side remark: the absence of a final . at the end of these captialized sentences is odd, but this is orthogonal to the current change.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree and generally struggle to remember to not add period in those explanations.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Improve error message for optional arguments mismatches

3 participants