CommonJS module loader is error-prone

[Melab]

Graal.js's CommonJS module loader implementation is naive. A module's source code is prepared in exactly the same manner that Node.js used to do it and could suffer from the same problems.

Here is the relevant section from CommonJSRequireBuiltin.java

// Read the file.
Source source = sourceFromPath(modulePath.toString(), realm);
TruffleString filenameBuiltin = Strings.fromJavaString(normalizedPath.toString());
if (modulePath.getParent() == null && !modulePath.exists()) {
throw fail(moduleIdentifier);
}
// Create `require` and other builtins for this module.
String dirnameBuiltin = modulePath.getParent() == null ? "." : modulePath.getParent().getAbsoluteFile().normalize().toString();
JSObject exportsBuiltin = createExportsBuiltin(realm);
JSObject moduleBuiltin = createModuleBuiltin(realm, exportsBuiltin, filenameBuiltin);
JSObject requireBuiltin = createRequireBuiltin(realm, moduleBuiltin, filenameBuiltin);
JSObject env = JSOrdinary.create(getContext(), getRealm());
JSObject.set(env, Strings.ENV_PROPERTY_NAME, JSOrdinary.create(getContext(), getRealm()));
// Parse the module
CharSequence characters = MODULE_PREAMBLE + source.getCharacters() + MODULE_END;
Source moduleSources = Source.newBuilder(source).content(characters).mimeType(JavaScriptLanguage.TEXT_MIME_TYPE).build();

ContextifyContext::CompileFunction was created to fix this in Node and is used by vm.compileFunction. Some other implementations of CommonJS, such as jvm-npm, use Function to create the module function.

[Melab]

I just verified this with this module code:

})();

console.log("This shouldn't work!");

(function () {

Calling require on the above module results in the message being printed.

[Melab]

This module code demonstrates the line/column offsets being incorrect in a stack trace:

throw Error("Is the stack trace correct?");

Output in JS launcher:

Error: Is the stack trace correct?
    at <js> :anonymous(/usr/local/lib/node_modules/throw_error/index.js:1:67-106)
    at <js> :program(<shell>:1:1:0-21)

The column offset is off by the length of MODULE_PREAMBLE, or 62 bytes.

[oubidar-Abderrahim]

Hi, Thank you for reporting this issue, we will take a look into it and get back to you

[wirthi]

Hi,

I'll ask @eleinadani to have a look at your problem; maybe there is a simple way to improve our loader.

In general, though, you will be right: it is a "naive" implementation that tries to mimic what Node is doing, but is certainly far from perfect.

-- Christian

[eleinadani]

We pushed a fix for this scenario here.

[Melab]

@eleinadani I meant to add this comment here a few days ago, but I was delayed and I just noticed you closed this issue.

A function should be added internally which behaves like Node's ContextifyContext::CompileFunction. For Graal.js's purposes, such a function would not need to be as complex and should take code representing a valid function body, a list of parameter names (that the created function would take), a source name that would be seen in stack traces, a column offset, and a line offset. The offsets would ve used to modify the offsets that appear in stack traces. If possible, a JavaScript binding for it should be added to the Debug or Polyglot builtins.

[Melab]

@wirthi @oubidar-Abderrahim Please see the above comment.