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

playground: make jsoo_main compile #538

Merged
merged 33 commits into from
Jun 5, 2023
Merged

playground: make jsoo_main compile #538

merged 33 commits into from
Jun 5, 2023

Conversation

jchavarri
Copy link
Member

@jchavarri jchavarri commented Apr 7, 2023

A humble start, just make Jsoo_main compile in a jsoo executable. As this part of the code is never exercised in ci, it had rotten quite a bit.

Now, it can be loaded from node, but instantly triggers a cryptic runtime error the moment you try to compile anything.

Repro:

opam install js_of_ocaml
dune build jscomp/main/jsoo_main.bc.js
node
Welcome to Node.js v17.8.0.
Type ".help" for more information.
> require('./_build/default/jscomp/main/jsoo_main.bc.js');
{}
> ocaml
{ compile: [Function (anonymous)], version: '0.3.2' }
> ocaml.compile("let t = 1");
Fatal error: exception Melange_compiler_libs.Env.Error(_)

TODO

Not sure if all should be part of this PR, or some parts can be deferred.

  • Don't hardcode BS_BROWSER: not sure how to set env var only for a dune executable
  • Investigate warning while compiling: Warning: integer overflow: integer 0x100000000 (4294967296) truncated to 0x0 (0); the generated code might be incorrect.
  • Compilation errors: properly handle the case when error_of_exn returns Some
  • Add the compilation of Jsoo_main to ci to prevent future rot
  • Investigate why building dune build --profile=prod jscomp/main/jsoo_main.bc.js crashes (maybe related to BS_BROWSER)
  • Make both melange.ppx and the "internal" ppx apply to the input code

@jchavarri
Copy link
Member Author

jchavarri commented May 6, 2023

Managed to print the internals of the cryptic Fatal error: exception Melange_compiler_libs.Env.Error(_) error by updating melange compiler libs:

Lookup_error: Unbound_module: Stdlib

So it seems for some reason the Melange stdlib is not being "linked" in the resulting bc.js file? I tried adding melange to the list of libraries in the executable here:

(libraries core melange-compiler-libs melange_ppx)

But still get the same error.

@hhugo
Copy link
Contributor

hhugo commented May 6, 2023

Managed to print the internals of the cryptic Fatal error: exception Melange_compiler_libs.Env.Error(_) error by updating melange compiler libs:

Lookup_error: Unbound_module: Stdlib

So it seems for some reason the Melange stdlib is not being "linked" in the resulting bc.js file? I tried adding melange to the list of libraries in the executable here:

(libraries core melange-compiler-libs melange_ppx)

But still get the same error.

I don't think this mean that the stdlib was not linked in, but rather that the CMI of the stdlib was not found by the type checker

@hhugo
Copy link
Contributor

hhugo commented May 6, 2023

Look at Persistent_env.Persistent_signature.load := Bs_cmi_load.load_cmi. it seems to be trying to load cmis using filesystem calls

@jchavarri
Copy link
Member Author

@hhugo thanks for the pointers! I started remembering now that I have to set up a process where the cmis and cmjs for the libraries used are placed somewhere, so they can be loaded in the static "pseudo" file system that jsoo uses at runtime.

I found the relevant part of the process in rescript repo, where js_of_ocaml build-fs is called here.

I understand that for the pseudo file system to work, besides preparing the cm*s like mentioned above, the jsoo build of melange should ignore the Persistent_signature.load mutation right? Something like this in Bs_conditional_initial.setup_env:

  #ifdef BS_BROWSER
  #else
    Persistent_env.Persistent_signature.load := Bs_cmi_load.load_cmi;
  #endif

@jchavarri
Copy link
Member Author

I got over the Stdlib errors by loading the Stdlib cmis in the pseudo file system 🎉 In the end, I didn't have to use ifdef BS_BROWSER to avoid the Persistent_env.Persistent_signature.load override 🤔

There is now some error the second time the compiler is invoked that I will investigate next:

$ make dev # build melange stdlib cmijs
$ dune build jscomp/main/jsoo_main.bc.js # build js version of the compiler
$ find _build/default/jscomp/stdlib-412/.stdlib.objs/melange -name "*.cmi" -or -name "*.cmj" | xargs js_of_ocaml build-fs -o stdlib.js
$ node
Welcome to Node.js v18.16.0.
Type ".help" for more information.
> require('./_build/default/jscomp/main/jsoo_main.bc.js');
{}
> require('./stdlib.js');
{}
> ocaml.compile("let t = 1");
{
  js_code: '// Generated by Melange\n' +
    "'use strict';\n" +
    '\n' +
    '\n' +
    'var t = 1;\n' +
    '\n' +
    'exports.t = t;\n' +
    '/* No side effect */\n'
}
> ocaml.compile("let t = 1");
{ js_error_msg: 'Invalid_argument("Ext_list.fold_right2")' }

@hhugo
Copy link
Contributor

hhugo commented May 10, 2023

I got over the Stdlib errors by loading the Stdlib cmis in the pseudo file system tada In the end, I didn't have to use ifdef BS_BROWSER to avoid the Persistent_env.Persistent_signature.load override thinking

There is now some error the second time the compiler is invoked that I will investigate next:

$ make dev # build melange stdlib cmijs
$ dune build jscomp/main/jsoo_main.bc.js # build js version of the compiler
$ find _build/default/jscomp/stdlib-412/.stdlib.objs/melange -name "*.cmi" -or -name "*.cmj" | xargs js_of_ocaml build-fs -o stdlib.js
$ node
Welcome to Node.js v18.16.0.
Type ".help" for more information.
> require('./_build/default/jscomp/main/jsoo_main.bc.js');
{}
> require('./stdlib.js');
{}
> ocaml.compile("let t = 1");
{
  js_code: '// Generated by Melange\n' +
    "'use strict';\n" +
    '\n' +
    '\n' +
    'var t = 1;\n' +
    '\n' +
    'exports.t = t;\n' +
    '/* No side effect */\n'
}
> ocaml.compile("let t = 1");
{ js_error_msg: 'Invalid_argument("Ext_list.fold_right2")' }

There is probably some reset function to call in between compilation.

@jchavarri
Copy link
Member Author

I could track it down to this line, where the lambda exports and the "meta exports" differ in length. Noticed there is something missing in the reset function, will open a PR in melange-compiler-libs.

@anmonteiro
Copy link
Member

Upgraded compiler libs in #591 so you should get them if you rebase.

jchavarri added 2 commits May 31, 2023 07:06
* main:
  test(rescript-syntax): fix rescript-syntax test (#592)
  fix: restore the old error message
  melange ppx: fatal error when parsing %raw with cb
  chore: update melange-compiler-libs (#591)
  GA(deps): bump cachix/install-nix-action from 20 to 21 (#589)
@jchavarri
Copy link
Member Author

I think the PR is in a state where it could be reviewed, before adding more functionality to the playground build, like Reason syntax or custom libraries cmi/js.

The only item from the todo list in the description that I couldn't find a fix for are the warnings. I saw there a line in the melange runtime lib that has a value 0x100000000 but I tried updating it and jsoo would still show the same warning. It doesn't seem to me that the jsoo playground depends on the melange runtime at all, so I am probably looking in the wrong place.

Otherwise, the following works:

$ make playground
$ make dev # build melange stdlib cmijs
$ find _build/default/jscomp/stdlib-412/.stdlib.objs/melange -name "*.cmi" -or -name "*.cmj" | xargs js_of_ocaml build-fs -o stdlib.js
$ node
Welcome to Node.js v18.16.0.
Type ".help" for more information.
> require('./_build/default/jscomp/main/jsoo_main.bc.js');
{}
> require('./stdlib.js');
{}
> ocaml.compile("let t = 1");
{
  js_code: '// Generated by Melange\n' +
    "'use strict';\n" +
    '\n' +
    '\n' +
    'var t = 1;\n' +
    '\n' +
    'exports.t = t;\n' +
    '/* No side effect */\n'
}
> ocaml.compile("[%%bs.raw \"var a = 1\"]");
{
  js_code: '// Generated by Melange\n' +
    "'use strict';\n" +
    '\n' +
    '\n' +
    'var a = 1\n' +
    ';\n' +
    '\n' +
    '/*  Not a pure module */\n'
}
> ocaml.compile(`type person\nexternal john : person = "john" [@@bs.module "MySchool"]`);
{
  js_code: '// Generated by Melange\n' +
    "/* This output is empty. Its source's type definitions, externals and/or unused code got optimized away. */\n"
}

let ast = Ppx_entry.rewrite_implementation ast in
let ast = Js_implementation.Ppx_entry.rewrite_implementation ast in
let ast =
Melange_ppxlib_ast.Of_ppxlib.copy_structure
Copy link
Member

Choose a reason for hiding this comment

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

this changed a bit in #604

Copy link
Member Author

Choose a reason for hiding this comment

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

I am not sure what is the right way to convert these asts now, using a lot of magic... 529df6a

@anmonteiro
Copy link
Member

there's a bit of a conflict with #615, you should be good just moving your files to bin/ when rebasing.

@jchavarri
Copy link
Member Author

@anmonteiro All the review comments were tackled. I mainly have doubts about the changes required after #604, there is too much magic but that seems to be the way these conversions are done now, and I am not sure how to do it otherwise.

@jchavarri jchavarri requested a review from anmonteiro June 5, 2023 13:33
@jchavarri
Copy link
Member Author

Ah, I also added some test for the playground to avoid having to run all these commands in Node manually every time anything changes. I couldn't find a way to have this test run with all the other tests, as the only way to set an env var for the jsoo_main.bc.js executable is by setting it from the profile, which means the new test can't be run with all the others, as profiles are global.

bin/jsoo_main.ml Outdated
let converted =
Convert.copy_structure (Ppxlib.Driver.map_structure ppxlib_ast)
in
(Obj.magic converted : Parsetree.structure)
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 pretty sure this is wrong. You probably copied it from melc.ml.

This is applying the PPXes and converting the preprocessed AST to the current switch version. Also Parsetree.structure isn't what you think it is because you opened Melange_compiler_libs globally.

In the playground, we just need this AST version to be the Melange_compiler_libs parsetree, I think? so we might not need to make the copy at all?

Copy link
Member

Choose a reason for hiding this comment

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

I just pushed 53dc2dc fixing this

Copy link
Member Author

@jchavarri jchavarri Jun 6, 2023

Choose a reason for hiding this comment

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

You probably copied it from melc.ml.

Yes.

In the playground, we just need this AST version to be the Melange_compiler_libs parsetree, I think? so we might not need to make the copy at all?

Just curious, I noticed you left the copy_structurecalls, why does ppxlib need them?

Copy link
Member

Choose a reason for hiding this comment

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

Ppxlib has its own version of the AST. So we need to convert from the melange version to its internal AST version and back. Right now that’s 4.14 so we technically don’t need these. But it’s better to avoid bugs when ppxlib bumps AST versions.

@anmonteiro
Copy link
Member

The Obj.magic situation is a bit unfortunate, because we version our own Parsetree, which is technically the same as the current ppxlib AST.

I haven't found a way to abstract over these that I'm happy with, so I've been repeating slight variations of the same code (now present in 3 places IIRC). I'll clean it up soon.

@anmonteiro
Copy link
Member

Let's do this.

@anmonteiro anmonteiro merged commit 0dbce3a into main Jun 5, 2023
@anmonteiro anmonteiro deleted the playground-1 branch June 5, 2023 23:43
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.

3 participants