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

Identify mismatched class type parameters and class parameters by ordinal in error messages. #12671

Merged
merged 5 commits into from
Oct 17, 2023

Conversation

smuenzel
Copy link
Contributor

The current error message for class type parameter mismatches looks like this:

A type parameter has type "'a" but is expected to have type "int"

This patch identifies the type parameter:

The 1st type parameter has type "'a" but is expected to have type "int"

@smuenzel smuenzel changed the title Identify mismatched class type parameters by ordinal in error messages. Identify mismatched class type parameters and class parameters by ordinal in error messages. Oct 17, 2023
@smuenzel smuenzel marked this pull request as ready for review October 17, 2023 06:40
@gasche
Copy link
Member

gasche commented Oct 17, 2023

For context: classes can have type parameters and also term/function parameters:

(* definition: *)
class ['a, 'b] ty = fun p n -> object
    method foo : ('a * 'b) * int = (p, n)
  end;;

(* inferred signature: *)
class ['a, 'b] ty : 'a * 'b -> int -> object
  method foo : ('a * 'b) * int
end

Here 'a, 'b are type parameters and p, n are parameters.

Questions for @smuenzel:

  1. The benefits of your change seem clear for term parameters, which can not be named in the error message. They are less clear to me for type parameters, where the name 'a, 'b already shows up in the error message. Why also use an ordinal on type parameters? (Maybe it is useful in some cases that are not exercised in the testsuite?)

  2. I suppose you got this idea for an improvement from a real-world situation where it would have made your life easier. Do you a representative example where the new error message helps?

@smuenzel
Copy link
Contributor Author

Questions for @smuenzel:

  1. The benefits of your change seem clear for term parameters, which can not be named in the error message. They are less clear to me for type parameters, where the name 'a, 'b already shows up in the error message. Why also use an ordinal on type parameters? (Maybe it is useful in some cases that are not exercised in the testsuite?)
  2. I suppose you got this idea for an improvement from a real-world situation where it would have made your life easier. Do you a representative example where the new error message helps?

Both questions have the same answer:
The original error that caused me to write this patch was the type parameter error (I only added the term parameter error because it seemed like that would be useful as well).
I tried to add an error to the testsuite that would approximate what I saw (it's called "Confusing"). In short: It's not immediately clear which parameter the error refers to if we only have a type variable name, since the type variable names may be different in the interface and implementation (e.g. we transposed the type variables). If the class types are very big, we might not be able to visually compare the two types on the screen.

Error: Signature mismatch:
       Modules do not match:
         sig
           class ['a, 'x] c :
             object constraint 'a = int method private id : 'a -> int end
         end
       is not included in
         sig class ['x, 'y] c : object  end end
       Class declarations do not match:
         class ['a, 'x] c :
           object constraint 'a = int method private id : 'a -> int end
       does not match
         class ['x, 'y] c : object  end
       A type parameter has type "int" but is expected to have type "'x"

Here, 'x is on both sides of the equation, in different positions, so the user doesn't have an easy time figuring out what's happening, especially when the constraint on the type is coming from a function somewhere deep in the object.

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.

Approved, but see small questions about the implementation in inline comments, to clarify before merging.

typing/ctype.ml Outdated Show resolved Hide resolved
typing/ctype.ml Outdated Show resolved Hide resolved
@smuenzel
Copy link
Contributor Author

Thanks for the review!

I wonder if allowing class arrow type declarations in addition to the current class body type declarations would be a good thing. It's certainly confusing that class type introduces a "class body type", and not what's known as a class_type in the compiler codebase.

@gasche gasche merged commit d435a29 into ocaml:trunk Oct 17, 2023
10 checks passed
@gasche
Copy link
Member

gasche commented Oct 17, 2023

Merged, thanks!

I was also surprised that class type foo = int -> object end would not work. I don't know the reasons, and making this particular area of the language more expressive is not high on my priority list. I would propose to wait for an actual need to pour more work in this direction.

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

Successfully merging this pull request may close these issues.

None yet

2 participants