import("hermes-parser") changes process.exitCode

[fisker]

Bug Description

• I have run gradle clean and confirmed this bug does not occur with JSC

Hermes version: 0.10.1
React Native version (if any):
OS version (if any): Windows
Platform (most likely one of arm64-v8a, armeabi-v7a, x86, x86_64): x86_64

Steps To Reproduce

code example:

process.exitCode = 11;
await import("hermes-parser");
console.log(process.exitCode);
// Logs `0` instead of `11`

The Expected Behavior

Should not change process.exitCode.

Possible related to #942

[tmikov]

This seems to be a side effect of the Emscripten glue code. A quick examination of HermesParserWASM.js narrows it down to this:

  quit_ = (status, toThrow) => {
    if (keepRuntimeAlive()) {
      process["exitCode"] = status;
      throw toThrow;
    }
    logExceptionOnExit(toThrow);
    process["exit"](status);
  };

I am not really sure what the correct approach here is. The whole Emscripten glue situation feels like a bit of a mess. This is definitely fixable, but somebody will have to really dig down and figure it out.

[pieterv]

Hey @fisker, i had a look into this issue. Its a bit tricky since the code that has this behavior is generated by emscripten so not something we have direct control over. I haven't been able to find a emscripten setting which disables this behavior, see the code below:

Emscripten code: https://github.com/emscripten-core/emscripten/blob/3c822ef6ba6b3be58b73fd8dde4422538fdbe3dc/src/shell.js#LL260C31-L260C31
Settings: https://github.com/emscripten-core/emscripten/blob/main/src/settings.js
Current settings found here: https://github.com/facebook/hermes/blob/main/tools/hermes-parser/CMakeLists.txt

There are some options thought:

1. emscripten allows you to override specific functions, the way this works is by creating a global that emscripten reads and overrides its own functions with, e.g. so the following code before requiring HermesParserWASM fixes the issue:

global.hermes_parser_wasm = {};
global.hermes_parser_wasm.quit = (status) => {
  if (status !== 0) {
    process.exitCode = status;
  }
};

The downside is this uses globals which won't work with mjs files (as i understand it?).

@fisker i assume you are looking at this to be able to include it in prettier v3? (i would love to see this happen so happy to help out with this work btw).

2. i could move the emscripten generated code so it only gets called via the parse function (not as a side effect of requiring "hermes-parser"), then at the end of that function i could reset the exitCode to the code before the call if its non zero. This might be better anyway since since its not great "hermes-parser" has these side effects.

@fisker any preference? Ill try giving 2) a go.

[tmikov]

@pieterv can this be addressed using -s MODULARIZE=1 and then passing options like this?

[pieterv]

Nice! I tested this and its working, MODULARIZE=1 also has the nice sideeffect of not initializing the module on import, so module sideeffects can be deferred. Ill put up the change shortly.

[fisker]

@pieterv Thank you for looking into this. Actually this is not really an issue for Prettier anymore, since we only need the visitorKeys.

The following

- import { VisitorKeys as flowVisitorKeys } from "hermes-eslint";
+ import flowVisitorKeys from "hermes-parser/dist/generated/ESTreeVisitorKeys.js";

already work for us. https://github.com/prettier/prettier/pull/14756/files#diff-26f745c3d0a653bf61c52f9adfd251e9a3fc619ec20ab1792b95e7296266edcaL3

I thought it worth to raise an issue since it took me a while to locate the problem. 😄

[pieterv]

@fisker Oh nice, glad you were able to get unblocked!

I was wondering, is size still an issue with the prettier V3 architecture? I know this was a concern for adding hermes parser as a core parser in the past (prettier/prettier#13818), i would love to get it in at some point since we actually patch prettier in Meta to use hermes-parser since its about x10 faster than the Flow parser.

[fisker]

Unfortunately yes, people has been complainted. We can try to support it after we fix prettier/prettier#13912. No promises.

[pieterv]

Unfortunately yes, people has been complainted

Weird i wonder why people care. The module system would be nice though! If size is a limitation maybe it would be ok to swap flow-parser for hermes-parser? flow-parser is 768 kB and hermes-parser 934 kB, so not too far off, i might be able to get hermes-parser closer to the flow size. They have equivalent features, however hermes parser doesn't need special support for Flow type comments.