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

Specialized error message for syntactic function arity type clash #12386

Conversation

ncik-roberts
Copy link
Contributor

@ncik-roberts ncik-roberts commented Jul 17, 2023

This continues a discussion with @Octachron from #12236. The topic is what error message to generate in a case of typing backward-incompatibility. Here's an example of such a case:

type (_, _) eq = Eq : ('a, 'a) eq
let f : type a. (a, int -> int) eq -> a =
  fun Eq x -> x 

As of #12236, the error message is a generic-looking unification error:

Error: This expression has type "(a, int -> int) eq -> 'a -> 'b"
       but an expression was expected of type "(a, int -> int) eq -> a"
       Type "'a -> 'b" is not compatible with type "a"

We thought we should do better to explain the problem, especially given that the change to typing is backward-incompatible. I'm making this PR to summarize the ideas so far, suggest a wording, and solicit further refinements/ideas.


(A) First try from original PR that we decided against:

3 |   fun Eq x -> x
      ^^^^^^^^^^^^^
Error: The type of a function with 2 arguments must have at least 2 arrows.
       This expression has type "(a, int -> int) eq -> 'a -> 'b"
       but an expression was expected of type "(a, int -> int) eq -> a"
       Type "'a -> 'b" is not compatible with type "a"

Some remarks that @Octachron made:

  • The criteria is not actually syntactic on types, despite the error message's claim to the contrary, since having a type (int -> int -> int as 'a) ->'a is fine.
  • The type (a, int -> int) eq -> 'a -> 'b is potentially confusing to include in the output. It doesn't match any type written by the user, so they may struggle to connect it to the main point of the error message.

(B) @Octachron 's suggestion that focuses on syntactic arity:

3 |   fun Eq x -> x
      ^^^^^^^^^^^^^
Error: The syntactic arity of the function doesn't match the type constraint:
       syntactic function arity and function type arity must agree without relying
       on local type equations.
       This function has two syntactic arguments, but its type is constrained to 
       "(a,int->int) eq -> a".

(C) Current draft:

3 |   fun Eq x -> x
      ^^^^^^^^^^^^^
Error: The syntactic arity of the function doesn't match the type constraint:
       Syntactic function arity must agree with (i.e., must be less than or
       equal to) function type arity without using local type equations.
       This function has 2 syntactic arguments, but its type is constrained
         to "(a, int -> int) eq -> a".
       Hint: consider splitting up the function's syntactic arity by
       inserting "-> fun" after the argument that introduces the local
       type equation.

This latest version:

  • clarifies what it means for syntactic function arity to "agree" with the function type arity. (It's not just that they are equal, as the function type can have more arrows than the function has syntactic arguments without any issue.)
  • attempts to suggest a fix.

This fix is meant to guide the user to this program, which type-checks:

type (_, _) eq = Eq : ('a, 'a) eq
let f : type a. (a, int -> int) eq -> a =
  fun Eq -> fun x -> x 

Here are a few areas of discussion that immediately come to mind:

  • In the hint, we could generate a list of candidate parameters with GADT patterns, as these are the only ones that could have introduced local type equations.
  • It doesn’t seem great to say “insert -> fun” as the grammatical object is a snippet of text, not a sensible AST. But maybe it is still a good way to guide the user to a type-checking program.

@gasche
Copy link
Member

gasche commented Jul 17, 2023

Your proposal:

Error: The syntactic arity of the function doesn't match the type constraint:
       Syntactic function arity must agree with (i.e., must be less than or
       equal to) function type arity without using local type equations.
       This function has 2 syntactic arguments, but its type is constrained
         to "(a, int -> int) eq -> a".
       Hint: consider splitting up the function's syntactic arity by
       inserting "-> fun" after the argument that introduces the local
       type equation.

I find the "must agree with" sentence too general/formal. I prefer the same message without it:

Error: The syntactic arity of the function doesn't match the type constraint:
       This function has 2 syntactic arguments, but its type is constrained
         to "(a, int -> int) eq -> a".
       Hint: consider splitting up the function's syntactic arity by
       inserting "-> fun" after the argument that introduces the local
       type equation.

@Octachron
Copy link
Member

Octachron commented Jul 18, 2023

If we want to have a hint which is grammatically valid, I propose to add ellipses:

Hint: consider splitting up the function definition into "fun ... gadt_pat -> fun ... "
where "gadt_pat" is the GADT constructor pattern that introduces the local type equation on "a". 

which from my point view has the advantage of making obvious what we mean by "splitting up the function".

I think it would helpful for users on their first reading of the error message to mention the local type abstract blocking the arity in the external view. If I am not mistaken, we can extract this local abstract type by looking up at the last Diff of the error trace.

Listing the GADT constructor pattern could be helpful but I believe that we are hitting diminishing return at this point.

@ncik-roberts
Copy link
Contributor Author

I like both of those suggestions, and I find the reworded hint helpful. In full, with minor tweaks to wording, that would be:

3 |   fun Eq x -> x
      ^^^^^^^^^^^^^
Error: The syntactic arity of the function doesn't match the type constraint:
       This function has 2 syntactic arguments, but its type is constrained
         to "(a, int -> int) eq -> a".
       Hint: consider splitting the function definition into "fun ... gadt_pat -> fun ..."
       where "gadt_pat" is the pattern with the GADT constructor that introduces the
       local type equation on "a".

I will look into extracting the locally abstract type's name, as I agree that it helps clarify the message.

@ncik-roberts
Copy link
Contributor Author

(Is a Changes entry needed? My feeling was no, but I will defer to others.)

@gasche
Copy link
Member

gasche commented Jul 18, 2023

You could add the PR number to the #12236 changes entry.

@ncik-roberts
Copy link
Contributor Author

Good idea, thanks. And done.

to "?opt:(a, int -> int) eq -> unit -> a".
Hint: consider splitting up the function's syntactic arity by
inserting "-> fun" after the argument that introduces the local
type equation.
Copy link
Member

Choose a reason for hiding this comment

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

I think that the testsuite output is not up-to-date with the error message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, sorry about that. I pushed a fix just as you left this comment.

@ncik-roberts ncik-roberts force-pushed the syntactic-function-arity-specialized-error-message branch from 9fbf74d to e97025b Compare July 18, 2023 20:29
@gasche gasche merged commit ed26585 into ocaml:trunk Jul 19, 2023
9 checks passed
ncik-roberts added a commit to ncik-roberts/flambda-backend that referenced this pull request Sep 11, 2023
Tests from:
  - ocaml/ocaml#12236 (and the corresponding updates to outputs found in ocaml/ocaml#12386 and ocaml/ocaml#12391)
  - ocaml/ocaml#12496 (not merged)
ncik-roberts added a commit to ncik-roberts/flambda-backend that referenced this pull request Sep 12, 2023
Tests from:
  - ocaml/ocaml#12236 (and the corresponding updates to outputs found in ocaml/ocaml#12386 and ocaml/ocaml#12391)
  - ocaml/ocaml#12496 (not merged)
ncik-roberts added a commit to ncik-roberts/flambda-backend that referenced this pull request Sep 13, 2023
Tests from:
  - ocaml/ocaml#12236 (and the corresponding updates to outputs found in ocaml/ocaml#12386 and ocaml/ocaml#12391)
  - ocaml/ocaml#12496 (not merged)
ncik-roberts added a commit to ncik-roberts/flambda-backend that referenced this pull request Sep 14, 2023
Tests from:
  - ocaml/ocaml#12236 (and the corresponding updates to outputs found in ocaml/ocaml#12386 and ocaml/ocaml#12391)
  - ocaml/ocaml#12496 (not merged)
ncik-roberts added a commit to ncik-roberts/flambda-backend that referenced this pull request Sep 15, 2023
Tests from:
  - ocaml/ocaml#12236 (and the corresponding updates to outputs found in ocaml/ocaml#12386 and ocaml/ocaml#12391)
  - ocaml/ocaml#12496 (not merged)
ncik-roberts added a commit to ncik-roberts/flambda-backend that referenced this pull request Oct 4, 2023
Tests from:
  - ocaml/ocaml#12236 (and the corresponding updates to outputs found in ocaml/ocaml#12386 and ocaml/ocaml#12391)
  - ocaml/ocaml#12496 (not merged)
ncik-roberts added a commit to ncik-roberts/flambda-backend that referenced this pull request Oct 5, 2023
Tests from:
  - ocaml/ocaml#12236 (and the corresponding updates to outputs found in ocaml/ocaml#12386 and ocaml/ocaml#12391)
  - ocaml/ocaml#12496 (not merged)
ncik-roberts added a commit to ncik-roberts/flambda-backend that referenced this pull request Oct 5, 2023
Tests from:
  - ocaml/ocaml#12236 (and the corresponding updates to outputs found in ocaml/ocaml#12386 and ocaml/ocaml#12391)
  - ocaml/ocaml#12496 (not merged)
ncik-roberts added a commit to ncik-roberts/flambda-backend that referenced this pull request Oct 5, 2023
Tests from:
  - ocaml/ocaml#12236 (and the corresponding updates to outputs found in ocaml/ocaml#12386 and ocaml/ocaml#12391)
  - ocaml/ocaml#12496 (not merged)
ncik-roberts added a commit to ncik-roberts/flambda-backend that referenced this pull request Oct 17, 2023
Tests from:
  - ocaml/ocaml#12236 (and the corresponding updates to outputs found in ocaml/ocaml#12386 and ocaml/ocaml#12391)
  - ocaml/ocaml#12496 (not merged)
ncik-roberts added a commit to ocaml-flambda/flambda-backend that referenced this pull request Dec 28, 2023
* Newtypes

* Constraint/coercion

* Add map_half_typed_cases

* Implement type-checking/translation

This also promotes tests whose output changes.

* Add upstream tests

Tests from:
  - ocaml/ocaml#12236 (and the corresponding updates to outputs found in ocaml/ocaml#12386 and ocaml/ocaml#12391)
  - ocaml/ocaml#12496 (not merged)

* Fix ocamldoc

* Update chamelon minimizer

* Respond to requested changes to minimizer

* update new test brought in from rebase

* Fix bug in chunking code

* `make bootstrap`

* Add Ast_invariant check

* Fix type-directed disambiguation of optional arg defaults

* Minor comments from review

* Run syntactic-arity test, update output, and fix printing bug

* Remove unnecessary call to escape

* Backport changes from upstream to comparative alloc tests

* Avoid the confusing [Split_function_ty] module

* Comment [split_function_ty] better.

* [contains_gadt] as variant instead of bool

* Calculate is_final_val_param on the fly rather than precomputing indexes

* Note suboptimality

* Get typecore typechecking

* Finish resolving merge conflicts and run tests

* make bootstrap

* Add iteration on / mapping over locations and attributes

* Reduce diff and fix typo in comment:

* promote change to zero-alloc arg structure

* Undo unintentional formatting changes to chamelon

* Fix minimizer

* Minimize diff

* Fix bug with local-returning method

* Fix regression where polymorphic parameters weren't allowed to be used in same parameter list as GADTs

* Fix merge conflicts and make bootstrap

* Apply expected diff to zero-alloc test changed in this PR
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.

None yet

3 participants