Fix Gentype: Support mutual recursion in types #6528
Conversation
Signed-Off-By: Jono Prest <[email protected]>
| let translateTypeDeclaration ~config ~outputFileRelative ~recursive ~resolver | ||
| ~typeEnv | ||
| ({typ_attributes; typ_id; typ_loc; typ_manifest; typ_params; typ_type} : | ||
| Typedtree.type_declaration) : CodeItem.typeDeclaration list = | ||
| if !Debug.translation then | ||
| Log_.item "Translate Type Declaration %s\n" (typ_id |> Ident.name); | ||
| if recursive then typeEnv |> TypeEnv.newType ~name:(typ_id |> Ident.name); |
There was a problem hiding this comment.
The issue here was that, a recursive type would not have access to any other mutually recursive types in the type declaration. So instead of checking inline here if it's recursive and adding it to the typeEnv. This happens a step above this function and adds all mutually recursive type ids to the typeEnv before proceeding.
Awesome to see your first contribution, great work! 👏 |
Signed-Off-By: Jono Prest <[email protected]>
JonoPrest
force-pushed
the
fix-gentyp-mutual-recursion
branch
from
a2563b1 to
3fac89e
Compare
Thanks @cristianoc I have updated the changelog and ammended to the commit. |
Thanks very much! |
jscomp/gentype/TranslateSignature.ml
Outdated
| @@ -98,13 +98,17 @@ and translateSignatureItem ~config ~outputFileRelative ~resolver ~typeEnv | |||
| signatureItem : Translation.t = | |||
| match signatureItem with | |||
| | {Typedtree.sig_desc = Typedtree.Tsig_type (recFlag, typeDeclarations)} -> | |||
| let recursive = recFlag = Recursive in | |||
There was a problem hiding this comment.
Right now the treatments of recursive and non-recursive cases are quite far apart, separated by 2 function calls.
Possible refactor: do this work inside translateTypeDeclarations.
Also, the non-recursive case could also be moved inside translateTypeDeclarations and the ~recursive parameter could then be omitted from translateTypeDeclaration.
@JonoPrest what do you think?
There was a problem hiding this comment.
That sounds good @cristianoc.
I'll update it 👍🏼
There was a problem hiding this comment.
@cristianoc I've added a refactor as suggested. I think it's much cleaner 👍🏼
There was a problem hiding this comment.
Sweet. Thanks for the great contribution.
Signed-Off-By: Jono Prest <[email protected]>
Hi there 👋🏼
This is my first PR into the rescript compiler. I've been a rescript user for around 2 years and absolutely love the language and the ecosystem. I would like to contribute more where I can. Please let me know if the contribution format is suitable. I've read the contribution guidlines.
I stumbled upon the issue I made at #6524 and thought I would take a stab at fixing it.
Changes:
Closes #6526