fix error message on curried/uncurried signature mismatch #6414

tsnobip · 2023-09-27T15:22:50Z

before this fix, error messages on curried/uncurried signature mismatch did not make sense in uncurried mode:

module Foo: {
  let add: (int, int) => int
} = {
  @@uncurried.swap
  let add = (. a, b) => a + b
}

yielded:

Signature mismatch:
  ...
  Values do not match:
    let add: (int, int) => int
  is not included in
    let add: (int, int) => int

Now it yields:

  Values do not match:
    let add: (. int, int) => int
  is not included in
    let add: (int, int) => int

The not so elegant part is that it means we print curried functions with a dot in uncurried mode, which might not be very clear, but I can't think of a better solution for now.

Fixes #6413

EDIT:

It now generates instead:

  Values do not match:
    let add: (int, int) => int (curried)
  is not included in
    let add: (int, int) => int (uncurried)

tsnobip · 2023-09-27T15:39:51Z

hmm yeah it broke a test because it then reformats in uncurried mode:

ignore(. 3)

which is pretty ugly

Another solution would be to give the curry mode of the function in the signature mismatch error message when it's different from the other function (or just different from the config), something like this:

Signature mismatch:
  ...
  Values do not match:
    let add: (int, int) => int (curried)
  is not included in
    let add: (int, int) => int (uncurried)

zth · 2023-09-27T15:44:57Z

What about special casing just this error message? Or is the code part controlling that a bit hairy perhaps?

tsnobip · 2023-09-29T07:34:12Z

What about special casing just this error message? Or is the code part controlling that a bit hairy perhaps?

Yeah that's what I'm trying to do:

  | Value_descriptions(id, 
      ({ val_type = { desc = Tarrow _}} as d1), 
      ({ val_type = { desc = Tconstr (Pident {name = "function$"},_,_)}} as d2)) ->
          fprintf ppf
            "@[<hv 2>Values do not match:@ %a@ (curied);<1 -2>is not included in@ %a@ (uncurried)]"
            (value_description id) d1 (value_description id) d2;
          show_locs ppf (d1.val_loc, d2.val_loc)

Weirdly, for some reason, this doesn't seem to work, am I not pattern matching curried and uncurried functions correctly?

@cristianoc any idea?

zth · 2023-09-29T07:42:05Z

@tsnobip there's this helper: Ast_uncurried_utils.typeIsUncurriedFun. Did you try matching on both combinations? Uncurried is the first type vs the last type.

tsnobip · 2023-09-29T07:52:15Z

@tsnobip there's this helper: Ast_uncurried_utils.typeIsUncurriedFun. Did you try matching on both combinations? Uncurried is the first type vs the last type.

hmm no I haven't tried it, thanks!

tsnobip · 2023-09-29T09:00:28Z

OK I solved it, the first value was wrapped inside a Tlink.

I now generate this:

  Values do not match:
    let add: (int, int) => int (curried)
  is not included in
    let add: (int, int) => int (uncurried)

are we good?

zth · 2023-09-29T09:22:13Z

Oh, right, maybe you need to recursively dig for the actual type_expr (as in: handle Tlink, Tsubst etc). That might be safest. What do you think @cristianoc ?

Outside of that it looks fantastic!

tsnobip · 2023-09-29T09:38:11Z

Oh, right, maybe you need to recursively dig for the actual type_expr (as in: handle Tlink, Tsubst etc). That might be safest. What do you think @cristianoc ?

Outside of that it looks fantastic!

Yes I could easily recursively dig for that, I pushed the current version, but if you think a recursive lookup is needed, I can add it.

zth · 2023-09-29T10:48:13Z

Yes I could easily recursively dig for that, I pushed the current version, but if you think a recursive lookup is needed, I can add it.

@cristianoc can correct me but I think it's needed, otherwise triggering this will depend on the type checking itself (whether it's a substituted type, a link, and so on).

zth · 2023-09-29T10:56:20Z

There's a helper for it btw, Ctype.expand_head.

cristianoc · 2023-09-29T11:00:03Z

There's a helper for it btw, Ctype.expand_head.

Yes helper, no explicit recursion .

tsnobip · 2023-09-29T11:09:33Z

ok I'll use the helper, but I have another issue that is driving me crazy, it seems like tests of super errors are run twice with different parameters and they generate two different outputs:

  Signature mismatch:
  Modules do not match:
    {
  let add: (int, int) => int
}
  is not included in
    {
  let add: (. int, int) => int
}
  Values do not match:
    let add: (int, int) => int (curried)
  is not included in
    let add: (. int, int) => int (uncurried)

or

  Signature mismatch:
  ....
  Values do not match:
    let add: (int, int) => int (curried)
  is not included in
    let add: (. int, int) => int (uncurried)

So the expected files are always wrong in one of the configurations.

zth · 2023-09-29T11:53:19Z

jscomp/ml/includemod.ml

+        | { desc = Tarrow _ }, 
+            { desc = Tconstr (Pident {name = "function$"},_,_)} -> (" (curried)", " (uncurried)")
+        | { desc = Tconstr (Pident {name = "function$"},_,_)}, 
+            { desc = Tarrow _ } -> (" (uncurried)", " (curried)")


Suggested change

| { desc = Tarrow _ },

{ desc = Tconstr (Pident {name = "function$"},_,_)} -> (" (curried)", " (uncurried)")

| { desc = Tconstr (Pident {name = "function$"},_,_)},

{ desc = Tarrow _ } -> (" (uncurried)", " (curried)")

| { desc = Tarrow _ },

t when Ast_uncurried_utils.typeIsUncurriedFun t -> (" (curried)", " (uncurried)")

| t,

{ desc = Tarrow _ } when Ast_uncurried_utils.typeIsUncurriedFun t -> (" (uncurried)", " (curried)")

hmm unfortunately it doesn't work with this helper, maybe it's too restrictive? Like working only on function value not on signatures maybe?

It probably needs expand head too first, forgot about that

Edit: scratch that, already done above

zth · 2023-10-03T19:50:46Z

ok I'll use the helper, but I have another issue that is driving me crazy, it seems like tests of super errors are run twice with different parameters and they generate two different outputs:
  Signature mismatch:
  Modules do not match:
    {
  let add: (int, int) => int
}
  is not included in
    {
  let add: (. int, int) => int
}
  Values do not match:
    let add: (int, int) => int (curried)
  is not included in
    let add: (. int, int) => int (uncurried)
or
  Signature mismatch:
  ....
  Values do not match:
    let add: (int, int) => int (curried)
  is not included in
    let add: (. int, int) => int (uncurried)
So the expected files are always wrong in one of the configurations.

@cristianoc you got any idea about this?

cristianoc · 2023-10-04T05:35:01Z

ok I'll use the helper, but I have another issue that is driving me crazy, it seems like tests of super errors are run twice with different parameters and they generate two different outputs:
  Signature mismatch:
  Modules do not match:
    {
  let add: (int, int) => int
}
  is not included in
    {
  let add: (. int, int) => int
}
  Values do not match:
    let add: (int, int) => int (curried)
  is not included in
    let add: (. int, int) => int (uncurried)
or
  Signature mismatch:
  ....
  Values do not match:
    let add: (int, int) => int (curried)
  is not included in
    let add: (. int, int) => int (uncurried)
So the expected files are always wrong in one of the configurations.
@cristianoc you got any idea about this?

Not sure. Worth checking if running tests twice is intentional.

tsnobip · 2023-10-04T15:00:21Z

Not sure. Worth checking if running tests twice is intentional.

Ok I checked again, and the test is only run once but what happens is that it first complains that the old output is A and new is B, but if I manually change the expected file content to B, then it complains that it should be A.

This is quite confusing and frustrating, especially given that the tests pass locally.

This reverts commit 1f1b4b8.

mununki · 2023-11-21T12:38:55Z

I don't figure out why yet. But, it was suspicious that the error message from the ci pointed to the wrong res and expected. Therefore I tried to rename the fixtures with the shorten name and update the expected, it seems fine. #6488

EDIT:
I rename the function name in the fixtures repectively to make sure.

zth

Great to finally land this, good job!

youngkidwarrior · 2023-11-21T16:10:27Z

Also extremely happy about this because it
blocked vite HMR for me. Hopefully with this fix I can use .resi files

fix error msg on curried/uncurried sig mismatch

66a0601

tsnobip force-pushed the signature_mismatch_curried_uncurried branch 2 times, most recently from f63152c to 70cd60b Compare September 29, 2023 10:44

mark function curry kind in value mismatch

b6efc4c

tsnobip force-pushed the signature_mismatch_curried_uncurried branch from 70cd60b to b6efc4c Compare September 29, 2023 11:12

use expand_head to check type of functions

018b17d

zth reviewed Sep 29, 2023

View reviewed changes

try passing tests in CI

1f1b4b8

Revert "try passing tests in CI"

04fd108

This reverts commit 1f1b4b8.

tsnobip added 2 commits November 21, 2023 14:56

shorten test file names

af30226

shorten test file names even more

2870c96

tsnobip requested a review from zth November 21, 2023 14:39

mununki mentioned this pull request Nov 21, 2023

CI failures that appear to be related to the length of the file name #6489

Open

zth approved these changes Nov 21, 2023

View reviewed changes

zth merged commit d74aa84 into master Nov 21, 2023
14 checks passed

zth deleted the signature_mismatch_curried_uncurried branch November 21, 2023 15:49

tsnobip mentioned this pull request Apr 26, 2024

Compiler bug in applying an uncurried function when the function has a signature #6742

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix error message on curried/uncurried signature mismatch #6414

fix error message on curried/uncurried signature mismatch #6414

tsnobip commented Sep 27, 2023 •

edited

tsnobip commented Sep 27, 2023

zth commented Sep 27, 2023

tsnobip commented Sep 29, 2023

zth commented Sep 29, 2023

tsnobip commented Sep 29, 2023

tsnobip commented Sep 29, 2023 •

edited

zth commented Sep 29, 2023 •

edited

tsnobip commented Sep 29, 2023

zth commented Sep 29, 2023

zth commented Sep 29, 2023

cristianoc commented Sep 29, 2023

tsnobip commented Sep 29, 2023

zth Sep 29, 2023

tsnobip Sep 29, 2023

zth Sep 29, 2023 •

edited

zth commented Oct 3, 2023

cristianoc commented Oct 4, 2023

tsnobip commented Oct 4, 2023 •

edited

mununki commented Nov 21, 2023 •

edited

zth left a comment

youngkidwarrior commented Nov 21, 2023 •

edited

fix error message on curried/uncurried signature mismatch #6414

fix error message on curried/uncurried signature mismatch #6414

Conversation

tsnobip commented Sep 27, 2023 • edited

tsnobip commented Sep 27, 2023

zth commented Sep 27, 2023

tsnobip commented Sep 29, 2023

zth commented Sep 29, 2023

tsnobip commented Sep 29, 2023

tsnobip commented Sep 29, 2023 • edited

zth commented Sep 29, 2023 • edited

tsnobip commented Sep 29, 2023

zth commented Sep 29, 2023

zth commented Sep 29, 2023

cristianoc commented Sep 29, 2023

tsnobip commented Sep 29, 2023

zth Sep 29, 2023

Choose a reason for hiding this comment

tsnobip Sep 29, 2023

Choose a reason for hiding this comment

zth Sep 29, 2023 • edited

Choose a reason for hiding this comment

zth commented Oct 3, 2023

cristianoc commented Oct 4, 2023

tsnobip commented Oct 4, 2023 • edited

mununki commented Nov 21, 2023 • edited

zth left a comment

Choose a reason for hiding this comment

youngkidwarrior commented Nov 21, 2023 • edited

tsnobip commented Sep 27, 2023 •

edited

tsnobip commented Sep 29, 2023 •

edited

zth commented Sep 29, 2023 •

edited

zth Sep 29, 2023 •

edited

tsnobip commented Oct 4, 2023 •

edited

mununki commented Nov 21, 2023 •

edited

youngkidwarrior commented Nov 21, 2023 •

edited