Fix cyclic import/export segfault

[bnoordhuis]

Consider the following two files:

// a.js
import {f} from "b.js"
export {f}

And:

// b.js
import * as a from "a.js"
export function f() {}

Before this commit, it crashed with a nullptr dereference. Throw a "circular reference when looking for export" SyntaxError.

No test because the test suite currently isn't equipped for tests that throw exceptions at import time.

Fixes: #567

[malbarbo]

Is throwing an exception the best approach here? The examples works in both node and deno (with extensions .mjs and ./ path in the import)

[richarddavison]

This is quite complicated. Node, deno, bun etc all handle the case in the issue. However they do not handle this case:

//index.js
import { a } from "./a.js";
console.log(a);

//a.js
import { b } from "./b.js";
export const a = "aloha";
console.log("from a", b);

//b.js
import { a } from "./a.js";
export const b = "bye";
console.log("from b", a);

The difference is when we export an object it should work as we can return the same reference, from b which later get's initialized in a as per Node.js documentation:

When main.js loads a.js, then a.js in turn loads b.js. At that point, b.js tries to load a.js. In order to prevent an infinite loop, an unfinished copy of the a.js exports object is returned to the b.js module. b.js then finishes loading, and its exports object is provided to the a.js module.

More info on this behavior is found here
https://nodejs.org/api/modules.html#cycles

[saghul]

The Node docs are not relevant here, what does the ES spec say?

[richarddavison]

The Node docs are not relevant here, what does the ES spec say?

It's a bit hard to decipher but this seems relevant:
https://tc39.es/ecma262/#figure-module-graph-cycle

Also the issue use case also works in all major browsers

[saghul]

Thanks for the link! Still a bit confusing since your example and the original behave different but seem the same on the surface.

Hum, certainly throwing an error is a better solution than to crash, so there is that.

I haven't touched the import code much, so I'm not sure right off the bat how hard it would be to make this work the way the other platforms do.

It's also somewhat surprising test262 doesn't catch this :-/

[bnoordhuis]

After lots of yak shaving to enable negative tests...

I'm going to land this first in order to fix the segfault (because SyntaxError > segfault) and then implement the proper behavior at some later time, because that's going to be a more elaborate change.

[bnoordhuis]

ffs, the windows buildbots still trip up on output from tests...

The netbsd failure is this;

  tests/test_std.js:234: Error: assertion failed: got |0|, expected |15|

Which is a flake. Also happened regularly on cygwin, see #184.

[bnoordhuis]

Argh.... fixing up the line endings by stubbing out the \r breaks some of the test262 tests. Okay, back to the drawing board.

[bnoordhuis]

netbsd again... "VM is booting" for the last 13 minutes and counting. Everyting else is green though!