From 49bd91f37a2cc8f34b8b6bc0cc0dae31d9bf89b1 Mon Sep 17 00:00:00 2001 From: Cristiano Calcagno Date: Mon, 18 Sep 2023 19:37:56 +0200 Subject: [PATCH] Gentype: fix issue with labelled args which should not be grouped. (#6406) * Gentype: fix issue with labelled args which should not be grouped. Grouping labelled args is a leftover incompatible with zero cost conversion. Fixes https://github.com/rescript-lang/rescript-compiler/issues/6361 * Make optional args of option type. --- CHANGELOG.md | 4 ++ jscomp/gentype/Converter.ml | 20 +----- jscomp/gentype/EmitJs.ml | 2 +- jscomp/gentype/EmitType.ml | 2 +- jscomp/gentype/GenTypeCommon.ml | 1 - jscomp/gentype/NamedArgs.ml | 70 +++---------------- jscomp/gentype/TranslateStructure.ml | 17 +++-- jscomp/gentype/TypeVars.ml | 7 +- .../package-lock.json | 9 ++- .../src/Docstrings.gen.tsx | 2 +- .../src/Hooks.gen.tsx | 8 +-- .../src/ImportHooks.gen.tsx | 4 +- .../src/LabeledFun.bs.js | 12 ++++ .../src/LabeledFun.gen.tsx | 9 +++ .../src/LabeledFun.res | 2 + .../src/TestPromise.gen.tsx | 2 +- .../src/Uncurried.gen.tsx | 2 +- .../src/hookExample.tsx | 4 +- .../typescript-react-example/src/index.tsx | 2 +- 19 files changed, 67 insertions(+), 112 deletions(-) create mode 100644 jscomp/gentype_tests/typescript-react-example/src/LabeledFun.bs.js create mode 100644 jscomp/gentype_tests/typescript-react-example/src/LabeledFun.gen.tsx create mode 100644 jscomp/gentype_tests/typescript-react-example/src/LabeledFun.res diff --git a/CHANGELOG.md b/CHANGELOG.md index 5d67dcbaed..51d816a9fa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,10 @@ # 11.0.0-rc.4 (Unreleased) +#### :bug: Bug Fix + +- Fix issue with GenType and labelled arguments https://github.com/rescript-lang/rescript-compiler/pull/6406 + #### :rocket: New Feature - Support renaming fields in inline records with `@as` attribute. [#6391](https://github.com/rescript-lang/rescript-compiler/pull/6391) diff --git a/jscomp/gentype/Converter.ml b/jscomp/gentype/Converter.ml index f31f0b602a..297da01cc6 100644 --- a/jscomp/gentype/Converter.ml +++ b/jscomp/gentype/Converter.ml @@ -13,9 +13,6 @@ let typeGetInlined ~config ~lookupId ~typeNameIsInterface type0 = let argConverted = argTypes |> List.map (argTypeToGroupedArg ~visited) in let retNormalized = retType |> visit ~visited in Function {function_ with argTypes = argConverted; retType = retNormalized} - | GroupOfLabeledArgs _ -> - (* This case should only fire from withing a function *) - normalized_ | Ident {builtin = true} -> normalized_ | Ident {builtin = false; name; typeArgs} -> ( if visited |> StringSet.mem name then ( @@ -77,21 +74,8 @@ let typeGetInlined ~config ~lookupId ~typeNameIsInterface type0 = in normalized and argTypeToGroupedArg ~visited {aName; aType} = - match aType with - | GroupOfLabeledArgs fields -> - let fieldsConverted = - fields - |> List.map (fun ({type_} as field) -> (field, type_ |> visit ~visited)) - in - let tNormalized = - GroupOfLabeledArgs - (fieldsConverted - |> List.map (fun (field, t) -> {field with type_ = t})) - in - {aName; aType = tNormalized} - | _ -> - let tNormalized = aType |> visit ~visited in - {aName; aType = tNormalized} + let tNormalized = aType |> visit ~visited in + {aName; aType = tNormalized} in let normalized = type0 |> visit ~visited:StringSet.empty in if !Debug.converter then diff --git a/jscomp/gentype/EmitJs.ml b/jscomp/gentype/EmitJs.ml index 0c1fee6fbc..660cb3a538 100644 --- a/jscomp/gentype/EmitJs.ml +++ b/jscomp/gentype/EmitJs.ml @@ -549,7 +549,7 @@ let propagateAnnotationToSubTypes ~codeItems (typeMap : CodeItem.exportTypeMap) | Function {argTypes; retType} -> argTypes |> List.iter (fun {aType} -> visit aType); retType |> visit - | GroupOfLabeledArgs fields | Object (_, fields) -> + | Object (_, fields) -> fields |> List.iter (fun {type_} -> type_ |> visit) | Option t | Null t | Nullable t | Promise t -> t |> visit | Tuple innerTypes -> innerTypes |> List.iter visit diff --git a/jscomp/gentype/EmitType.ml b/jscomp/gentype/EmitType.ml index 1b7ec24ceb..c8e6601084 100644 --- a/jscomp/gentype/EmitType.ml +++ b/jscomp/gentype/EmitType.ml @@ -105,7 +105,7 @@ let rec renderType ~(config : Config.t) ?(indent = None) ~typeNameIsInterface | Function {argTypes; retType; typeVars} -> renderFunType ~config ~indent ~inFunType ~typeNameIsInterface ~typeVars argTypes retType - | GroupOfLabeledArgs fields | Object (_, fields) -> + | Object (_, fields) -> let indent1 = fields |> Indent.heuristicFields ~indent in fields |> renderFields ~config ~indent:indent1 ~inFunType ~typeNameIsInterface diff --git a/jscomp/gentype/GenTypeCommon.ml b/jscomp/gentype/GenTypeCommon.ml index 3e40729dc7..42c5e9a377 100644 --- a/jscomp/gentype/GenTypeCommon.ml +++ b/jscomp/gentype/GenTypeCommon.ml @@ -69,7 +69,6 @@ type type_ = | Array of type_ * mutable_ | Dict of type_ | Function of function_ - | GroupOfLabeledArgs of fields | Ident of ident | Null of type_ | Nullable of type_ diff --git a/jscomp/gentype/NamedArgs.ml b/jscomp/gentype/NamedArgs.ml index 2d056e950a..a5c0d0638d 100644 --- a/jscomp/gentype/NamedArgs.ml +++ b/jscomp/gentype/NamedArgs.ml @@ -1,62 +1,14 @@ open GenTypeCommon -type groupedArg = Group of fields | Arg of type_ - -(** - For convenient processing turns consecutive named arguments into a - `NamedArgs` group, and individual non-named arguments into `Arg`s. -*) -let rec groupReversed ~revCurGroup ~revResult labeledTypes = - match (revCurGroup, labeledTypes) with - | [], (Nolabel, type_) :: tl -> - groupReversed ~revCurGroup:[] ~revResult:(Arg type_ :: revResult) tl - | _, (OptLabel name, type_) :: tl -> - (* Add it to the current group, not result. *) - groupReversed - ~revCurGroup: - ({ - mutable_ = Immutable; - nameJS = name; - optional = Optional; - type_; - docString = DocString.empty; - } - :: revCurGroup) - ~revResult tl - | _, (Label name, type_) :: tl -> - groupReversed - ~revCurGroup: - ({ - mutable_ = Immutable; - nameJS = name; - optional = Mandatory; - type_; - docString = DocString.empty; - } - :: revCurGroup) - ~revResult tl - | [], [] -> revResult - | _grpHd :: _grpTl, ([] as _tl) | _grpHd :: _grpTl, (Nolabel, _) :: _tl -> - (* Just form the group, and recurse ignoring the (None, t) in that case. - t will be handled in recursion. *) - groupReversed ~revCurGroup:[] - ~revResult:(Group revCurGroup :: revResult) - labeledTypes - -(** Special reverse that not only reverses the entire list but also the order of - items in the NamedArgs grouping. *) -let rec reverse ?(soFar = []) lst = - match lst with - | [] -> soFar - | [Arg type_] when type_ = unitT && soFar = [] -> - (* treat a single argument of type unit as no argument *) - [] - | Arg type_ :: tl -> reverse ~soFar:({aName = ""; aType = type_} :: soFar) tl - | Group fields :: tl -> - reverse - ~soFar: - ({aName = ""; aType = GroupOfLabeledArgs (List.rev fields)} :: soFar) - tl - let group labeledTypes = - labeledTypes |> groupReversed ~revCurGroup:[] ~revResult:[] |> reverse + let types = + Ext_list.map labeledTypes (fun (lbl, aType) -> + match lbl with + | Nolabel -> {aName = ""; aType} + | Label lbl -> {aName = lbl; aType} + | OptLabel lbl -> {aName = lbl; aType = Option aType}) + in + match types with + | [{aType}] when aType = unitT -> + [] (* treat a single argument of type unit as no argument *) + | _ -> types diff --git a/jscomp/gentype/TranslateStructure.ml b/jscomp/gentype/TranslateStructure.ml index b6eb3e8f2a..66185837fc 100644 --- a/jscomp/gentype/TranslateStructure.ml +++ b/jscomp/gentype/TranslateStructure.ml @@ -3,14 +3,19 @@ open GenTypeCommon let rec addAnnotationsToTypes_ ~config ~(expr : Typedtree.expression) (argTypes : argType list) = match (expr.exp_desc, expr.exp_type.desc, argTypes) with - | _, _, {aName; aType = GroupOfLabeledArgs fields} :: nextTypes -> - let fields1, nextTypes1 = - addAnnotationsToFields ~config expr fields nextTypes - in - {aName; aType = GroupOfLabeledArgs fields1} :: nextTypes1 - | Texp_function {param; cases = [{c_rhs}]}, _, {aType} :: nextTypes -> + | Texp_function {arg_label; param; cases = [{c_rhs}]}, _, {aType} :: nextTypes + -> let nextTypes1 = nextTypes |> addAnnotationsToTypes_ ~config ~expr:c_rhs in let aName = Ident.name param in + let _ = Printtyped.implementation in + let aName = + if aName = "*opt*" then + match arg_label with + | Optional l -> + l + | _ -> "" (* should not happen *) + else aName + in {aName; aType} :: nextTypes1 | Texp_construct ({txt = Lident "Function$"}, _, [funExpr]), _, _ -> (* let uncurried1: function$<_, _> = Function$(x => x |> string_of_int, [`Has_arity1]) *) diff --git a/jscomp/gentype/TypeVars.ml b/jscomp/gentype/TypeVars.ml index 780ea36dd1..3957362cb7 100644 --- a/jscomp/gentype/TypeVars.ml +++ b/jscomp/gentype/TypeVars.ml @@ -40,11 +40,6 @@ let rec substitute ~f type0 = |> List.map (fun {aName; aType = t} -> {aName; aType = t |> substitute ~f}); } - | GroupOfLabeledArgs fields -> - GroupOfLabeledArgs - (fields - |> List.map (fun field -> - {field with type_ = field.type_ |> substitute ~f})) | Ident {typeArgs = []} -> type0 | Ident ({typeArgs} as ident) -> Ident {ident with typeArgs = typeArgs |> List.map (substitute ~f)} @@ -80,7 +75,7 @@ let rec free_ type0 : StringSet.t = StringSet.diff ((argTypes |> freeOfList_) +++ (retType |> free_)) (typeVars |> StringSet.of_list) - | GroupOfLabeledArgs fields | Object (_, fields) -> + | Object (_, fields) -> fields |> List.fold_left (fun s {type_} -> StringSet.union s (type_ |> free_)) diff --git a/jscomp/gentype_tests/typescript-react-example/package-lock.json b/jscomp/gentype_tests/typescript-react-example/package-lock.json index 4ae7344f20..89a23a9fa4 100644 --- a/jscomp/gentype_tests/typescript-react-example/package-lock.json +++ b/jscomp/gentype_tests/typescript-react-example/package-lock.json @@ -21,7 +21,7 @@ }, "../../..": { "name": "rescript", - "version": "11.0.0-rc.2", + "version": "11.0.0-rc.4", "dev": true, "hasInstallScript": true, "license": "SEE LICENSE IN LICENSE", @@ -241,10 +241,9 @@ "rescript": { "version": "file:../../..", "requires": { - "mocha": "^10.1.0", - "nyc": "^15.0.0", - "prettier": "^2.7.1", - "rollup": "^0.49.2" + "mocha": "10.1.0", + "nyc": "15.0.0", + "prettier": "2.7.1" } }, "scheduler": { diff --git a/jscomp/gentype_tests/typescript-react-example/src/Docstrings.gen.tsx b/jscomp/gentype_tests/typescript-react-example/src/Docstrings.gen.tsx index c53ea4f951..8b26dec8a1 100644 --- a/jscomp/gentype_tests/typescript-react-example/src/Docstrings.gen.tsx +++ b/jscomp/gentype_tests/typescript-react-example/src/Docstrings.gen.tsx @@ -39,7 +39,7 @@ export const unnamed2: (param_0:number, param_1:number) => number = DocstringsBS export const unnamed2U: (param_0:number, param_1:number) => number = DocstringsBS.unnamed2U; -export const grouped: (_1:{ readonly x: number; readonly y: number }, a:number, b:number, c:number, _5:{ readonly z: number }) => number = DocstringsBS.grouped; +export const grouped: (x:number, y:number, a:number, b:number, c:number, z:number) => number = DocstringsBS.grouped; export const unitArgWithoutConversion: () => string = DocstringsBS.unitArgWithoutConversion; diff --git a/jscomp/gentype_tests/typescript-react-example/src/Hooks.gen.tsx b/jscomp/gentype_tests/typescript-react-example/src/Hooks.gen.tsx index 5fa19cd4c6..9bf99f853c 100644 --- a/jscomp/gentype_tests/typescript-react-example/src/Hooks.gen.tsx +++ b/jscomp/gentype_tests/typescript-react-example/src/Hooks.gen.tsx @@ -14,7 +14,7 @@ import type {TypedArray2_Uint8Array_t as Js_TypedArray2_Uint8Array_t} from '../s export type vehicle = { readonly name: string }; // tslint:disable-next-line:interface-over-type-literal -export type cb = (_1:{ readonly _to: vehicle }) => void; +export type cb = (_to:vehicle) => void; // tslint:disable-next-line:interface-over-type-literal export type r = { readonly x: string }; @@ -71,11 +71,7 @@ export type NoProps_make_Props = {}; export const NoProps_make: React.ComponentType<{}> = HooksBS.NoProps.make; -export const functionWithRenamedArgs: (_1:{ - readonly _to: vehicle; - readonly _Type: vehicle; - readonly cb: cb -}) => string = HooksBS.functionWithRenamedArgs; +export const functionWithRenamedArgs: (_to:vehicle, _Type:vehicle, cb:cb) => string = HooksBS.functionWithRenamedArgs; // tslint:disable-next-line:interface-over-type-literal export type WithRename_componentWithRenamedArgs_Props = { diff --git a/jscomp/gentype_tests/typescript-react-example/src/ImportHooks.gen.tsx b/jscomp/gentype_tests/typescript-react-example/src/ImportHooks.gen.tsx index e0874a2b7d..a16f0b9bbe 100644 --- a/jscomp/gentype_tests/typescript-react-example/src/ImportHooks.gen.tsx +++ b/jscomp/gentype_tests/typescript-react-example/src/ImportHooks.gen.tsx @@ -23,10 +23,10 @@ export const makeRenamed: unknown = makeRenamedTypeChecked as React.ComponentTyp }>; // In case of type error, check the type of 'foo' in 'ImportHooks.res' and './hookExample'. -export const fooTypeChecked: (_1:{ readonly person: person }) => string = fooNotChecked; +export const fooTypeChecked: (person:person) => string = fooNotChecked; // Export 'foo' early to allow circular import from the '.bs.js' file. -export const foo: unknown = fooTypeChecked as (_1:{ readonly person: person }) => string; +export const foo: unknown = fooTypeChecked as (person:person) => string; // tslint:disable-next-line:interface-over-type-literal export type person = { readonly name: string; readonly age: number }; diff --git a/jscomp/gentype_tests/typescript-react-example/src/LabeledFun.bs.js b/jscomp/gentype_tests/typescript-react-example/src/LabeledFun.bs.js new file mode 100644 index 0000000000..2be82a13b9 --- /dev/null +++ b/jscomp/gentype_tests/typescript-react-example/src/LabeledFun.bs.js @@ -0,0 +1,12 @@ +// Generated by ReScript, PLEASE EDIT WITH CARE + + +function labelled(a, bOpt, c, d, e, f) { + var b = bOpt !== undefined ? bOpt : 3; + return ((((a + b | 0) + c | 0) + d | 0) + e | 0) + f | 0; +} + +export { + labelled , +} +/* No side effect */ diff --git a/jscomp/gentype_tests/typescript-react-example/src/LabeledFun.gen.tsx b/jscomp/gentype_tests/typescript-react-example/src/LabeledFun.gen.tsx new file mode 100644 index 0000000000..1d9c03e505 --- /dev/null +++ b/jscomp/gentype_tests/typescript-react-example/src/LabeledFun.gen.tsx @@ -0,0 +1,9 @@ +/* TypeScript file generated from LabeledFun.res by genType. */ +/* eslint-disable import/first */ + + +// @ts-ignore: Implicit any on import +import * as LabeledFunBS__Es6Import from './LabeledFun.bs'; +const LabeledFunBS: any = LabeledFunBS__Es6Import; + +export const labelled: (a:number, b:(undefined | number), c:number, _4:number, e:number, f:number) => number = LabeledFunBS.labelled; diff --git a/jscomp/gentype_tests/typescript-react-example/src/LabeledFun.res b/jscomp/gentype_tests/typescript-react-example/src/LabeledFun.res new file mode 100644 index 0000000000..42956e69d9 --- /dev/null +++ b/jscomp/gentype_tests/typescript-react-example/src/LabeledFun.res @@ -0,0 +1,2 @@ +@genType +let labelled = (a, ~b=3, ~c, d, ~e, ~f) => a + b + c + d + e + f diff --git a/jscomp/gentype_tests/typescript-react-example/src/TestPromise.gen.tsx b/jscomp/gentype_tests/typescript-react-example/src/TestPromise.gen.tsx index 2b8f97eadb..8aef01e6ce 100644 --- a/jscomp/gentype_tests/typescript-react-example/src/TestPromise.gen.tsx +++ b/jscomp/gentype_tests/typescript-react-example/src/TestPromise.gen.tsx @@ -17,4 +17,4 @@ export type toPayload = { readonly result: string }; export const convert: (_1:Promise) => Promise = TestPromiseBS.convert; -export const barx: (_1:{ readonly x?: Promise<(undefined | string)> }, _2:void) => boolean = TestPromiseBS.barx; +export const barx: (x:(undefined | Promise<(undefined | string)>), _2:void) => boolean = TestPromiseBS.barx; diff --git a/jscomp/gentype_tests/typescript-react-example/src/Uncurried.gen.tsx b/jscomp/gentype_tests/typescript-react-example/src/Uncurried.gen.tsx index 1fa912985f..aec96f4a1e 100644 --- a/jscomp/gentype_tests/typescript-react-example/src/Uncurried.gen.tsx +++ b/jscomp/gentype_tests/typescript-react-example/src/Uncurried.gen.tsx @@ -46,4 +46,4 @@ export const sumU2: (n:number) => (_1:number) => void = UncurriedBS.sumU2; export const sumCurried: (n:number, _2:number) => void = UncurriedBS.sumCurried; -export const sumLblCurried: (s:string, _2:{ readonly n: number; readonly m: number }) => void = UncurriedBS.sumLblCurried; +export const sumLblCurried: (s:string, n:number, m:number) => void = UncurriedBS.sumLblCurried; diff --git a/jscomp/gentype_tests/typescript-react-example/src/hookExample.tsx b/jscomp/gentype_tests/typescript-react-example/src/hookExample.tsx index ba71a545e3..ac51a9ff0f 100644 --- a/jscomp/gentype_tests/typescript-react-example/src/hookExample.tsx +++ b/jscomp/gentype_tests/typescript-react-example/src/hookExample.tsx @@ -1,8 +1,6 @@ import * as React from "react"; -export const foo = (x: { - person: { readonly name: string; readonly age: number }; -}) => x.person.name; +export const foo = (person: { readonly name: string; readonly age: number }) => person.name; type Props = { readonly person: { readonly name: string; readonly age: number }; diff --git a/jscomp/gentype_tests/typescript-react-example/src/index.tsx b/jscomp/gentype_tests/typescript-react-example/src/index.tsx index 179abad72d..14bc1a0134 100644 --- a/jscomp/gentype_tests/typescript-react-example/src/index.tsx +++ b/jscomp/gentype_tests/typescript-react-example/src/index.tsx @@ -61,7 +61,7 @@ consoleLog( Uncurried.sumU(3, 4); Uncurried.sumU2(3)(4); Uncurried.sumCurried(3, 4); -Uncurried.sumLblCurried("hello", { n: 3, m: 4 }); +Uncurried.sumLblCurried("hello", 3, 4); ReactDOM.render(