Skip to content

Fix super error mishandling uncurried function #6694

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Mar 25, 2024
Merged

Fix super error mishandling uncurried function #6694

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
09c9c72
Fix super error mishandling uncurried function
JonoPrest Mar 24, 2024
6b9d9dc
Add fix mishandling of uncurried functions in super errors to changelog
JonoPrest Mar 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,10 @@

# 11.1.0-rc.6 (Unreleased)

#### :bug: Bug Fix

- Fix mishandling of uncurried functions in super errors. https://github.com/rescript-lang/rescript-compiler/pull/6694

# 11.1.0-rc.5

#### :bug: Bug Fix
Expand Down
15 changes: 15 additions & 0 deletions jscomp/build_tests/super_errors/expected/UncurriedArgsNotApplied.res.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@

We've found a bug for you!
/.../fixtures/UncurriedArgsNotApplied.res:3:15-21

1 │ let apply = (fn: (. unit) => option<int>) => fn(. ())
2 │
3 │ let _ = apply(Some(1))
4 │

This value might need to be wrapped in a function that takes an extra
parameter of type unit

Here's the original error message
This has type: option<'a>
But it's expected to have type: (. unit) => option<int>
Comment on lines +10 to +15
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This now matches the super error that was outputted when a curried function was defined

3 changes: 3 additions & 0 deletions jscomp/build_tests/super_errors/fixtures/UncurriedArgsNotApplied.res
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
let apply = (fn: (. unit) => option<int>) => fn(. ())

let _ = apply(Some(1))
2 changes: 1 addition & 1 deletion jscomp/frontend/ast_core_type.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ let get_uncurry_arity (ty : t) =

let get_curry_arity (ty : t) =
if Ast_uncurried.coreTypeIsUncurriedFun ty then
let arity, _ = Ast_uncurried.typeExtractUncurriedFun ty in
let arity, _ = Ast_uncurried.coreTypeExtractUncurriedFun ty in
arity
else get_uncurry_arity_aux ty 0

Expand Down
2 changes: 1 addition & 1 deletion jscomp/frontend/ast_external_process.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ let spec_of_ptyp (nolabel : bool) (ptyp : Parsetree.core_type) :
| `Uncurry opt_arity -> (
let real_arity =
if Ast_uncurried.coreTypeIsUncurriedFun ptyp then
let arity, _ = Ast_uncurried.typeExtractUncurriedFun ptyp in
let arity, _ = Ast_uncurried.coreTypeExtractUncurriedFun ptyp in
Some arity
else Ast_core_type.get_uncurry_arity ptyp
in
Expand Down
15 changes: 12 additions & 3 deletions jscomp/ml/ast_uncurried.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,14 +69,20 @@ let coreTypeIsUncurriedFun (typ : Parsetree.core_type) =
true
| _ -> false

let typeIsUncurriedFun = Ast_uncurried_utils.typeIsUncurriedFun

let typeExtractUncurriedFun (typ : Parsetree.core_type) =
let coreTypeExtractUncurriedFun (typ : Parsetree.core_type) =
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed from typeExtractUncurriedFun coreTypeExtractUncurriedFun to follow naming convention in this file. I I have redefined typeExtractUncurriedFun below. (coreTypeIsUncurriedFun is named above fore the same paramater type, and for functions with Types.type_expr as a param use typeIsUncurredFun and typeExtractUncurriedFun respectively)

match typ.ptyp_desc with
| Ptyp_constr ({txt = Lident "function$"}, [tArg; tArity]) ->
(arityFromType tArity, tArg)
| _ -> assert false

let typeIsUncurriedFun = Ast_uncurried_utils.typeIsUncurriedFun

let typeExtractUncurriedFun (typ : Types.type_expr) =
match typ.desc with
| Tconstr (Pident {name = "function$"}, [tArg; _], _) ->
tArg
| _ -> assert false

(* Typed AST *)

let arity_to_type arity =
Expand Down Expand Up @@ -114,3 +120,6 @@ let uncurried_type_get_arity_opt ~env typ =
| Tconstr (Pident { name = "function$" }, [ _t; tArity ], _) ->
Some (type_to_arity tArity)
| _ -> None



3 changes: 2 additions & 1 deletion jscomp/ml/error_message_utils.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,8 @@ let typeClashContextMaybeOption ty_expected ty_res =
| ( {Types.desc = Tconstr (expectedPath, _, _)},
{Types.desc = Tconstr (typePath, _, _)} )
when Path.same Predef.path_option typePath
&& Path.same expectedPath Predef.path_option = false ->
&& Path.same expectedPath Predef.path_option = false
&& Path.same expectedPath Predef.path_uncurried = false ->
Comment on lines +207 to +208
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here was another gremlin I came across where you get a suggestion to unwrap your option. For the example in the tests/descriptioon you get:

  This value might need to be wrapped in a function that takes an extra
  parameter of type unit
                                                                                                                                            
  Here's the original error message
  This has type: option<'a>
  But it's expected to have type: (. unit) => option<int>
                                                                                                                                            
  Possible solutions:
  - Unwrap the option to its underlying value using `yourValue->Belt.Option.getWithDefault(someDefaultValue)`

Where the possible solution is not meant to be for functions but uncurried functions still pattern match on this case.

Some MaybeUnwrapOption
| _ -> None

Expand Down
1 change: 1 addition & 0 deletions jscomp/ml/predef.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ and path_extension_constructor = Pident ident_extension_constructor
and path_floatarray = Pident ident_floatarray

and path_promise = Pident ident_promise
and path_uncurried = Pident ident_uncurried

let type_int = newgenty (Tconstr(path_int, [], ref Mnil))
and type_char = newgenty (Tconstr(path_char, [], ref Mnil))
Expand Down
1 change: 1 addition & 0 deletions jscomp/ml/predef.mli
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ val path_lazy_t: Path.t
val path_extension_constructor: Path.t
val path_floatarray: Path.t
val path_promise: Path.t
val path_uncurried: Path.t

val path_match_failure: Path.t
val path_assert_failure : Path.t
Expand Down
7 changes: 6 additions & 1 deletion jscomp/ml/typecore.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -307,7 +307,9 @@ let extract_concrete_record env ty =

let extract_concrete_variant env ty =
match extract_concrete_typedecl env ty with
(p0, p, {type_kind=Type_variant cstrs}) -> (p0, p, cstrs)
(p0, p, {type_kind=Type_variant cstrs})
when not (Ast_uncurried.typeIsUncurriedFun ty)
-> (p0, p, cstrs)
Comment on lines +310 to +312
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was the first issue, leading to the initial bad super error being displayed. The uncurried function type would extract as a concrete variant since it matched this case when it should not.

Copy link
Member

Choose a reason for hiding this comment

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

I'm sure you've read and know more about this, but instead of throwing an exception with Not_found if uncurried, what about an implementation that extracts the uncurried case? Something like this.

if Ast_uncurried_utils.typeIsUncurriedFun ty then
   extract_concrete_variant env (Ast_uncurried.typeExtractUncurriedFun ty)
else (p0, p, cstrs)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @mununki thanks for the comment! Yes I think that feel a lot nicer except that in practice it would make no difference to this function since if it is an uncurried function the extracted type would not be Type_variant? So happy to add the refactor to make it more future proof but otherwise it should never hit a case where it is an uncurried function and it can extract a concrete variant

Copy link
Member

Choose a reason for hiding this comment

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

Sorry for my misunderstanding, if so why do we need that when clause by then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for my misunderstanding, if so why do we need that when clause by then?

It's because an uncurried function is represented by this pattern

Tconstr (Pident {name = "function$"}, [tArg; _], _)

where tArg is the underlying type of Tarrow

So when trying to extract the concrete type as a variant, it matches the case of Tconstr, but is not actually variant it's essentially just a wrapper around the underlying function. So we get a false positive.

| (p0, p, {type_kind=Type_open}) -> (p0, p, [])
| _ -> raise Not_found

Expand Down Expand Up @@ -662,6 +664,9 @@ let rec collect_missing_arguments env type1 type2 = match type1 with
| Some res -> Some ((label, argtype) :: res)
| None -> None
end
| t when Ast_uncurried.typeIsUncurriedFun t ->
let typ = Ast_uncurried.typeExtractUncurriedFun t in
collect_missing_arguments env typ type2
| _ -> None

let print_expr_type_clash ?typeClashContext env trace ppf = begin
Expand Down
2 changes: 1 addition & 1 deletion jscomp/syntax/src/jsx_common.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ let optionalAttr = ({txt = "res.optional"; loc = Location.none}, PStr [])

let extractUncurried typ =
if Ast_uncurried.coreTypeIsUncurriedFun typ then
let _arity, t = Ast_uncurried.typeExtractUncurriedFun typ in
let _arity, t = Ast_uncurried.coreTypeExtractUncurriedFun typ in
t
else typ

Expand Down
2 changes: 1 addition & 1 deletion jscomp/syntax/src/res_printer.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1671,7 +1671,7 @@ and printTypExpr ~(state : State.t) (typExpr : Parsetree.core_type) cmtTbl =
printObject ~state ~inline:false fields openFlag cmtTbl
| Ptyp_arrow _ -> printArrow ~uncurried:false typExpr
| Ptyp_constr _ when Ast_uncurried.coreTypeIsUncurriedFun typExpr ->
let arity, tArg = Ast_uncurried.typeExtractUncurriedFun typExpr in
let arity, tArg = Ast_uncurried.coreTypeExtractUncurriedFun typExpr in
printArrow ~uncurried:true ~arity tArg
| Ptyp_constr (longidentLoc, [{ptyp_desc = Ptyp_object (fields, openFlag)}])
->
Expand Down