Module piercing

[Protryon]

This has a minor reducing impact on size for large module collections, however, this is mostly due to variable renaming to resolve conflicts. We will need DCE to get the benefits out of this. The old loader is still available, working, and testing.

[bakkot]

An idea (which we may or may not find valuable enough to pursue):

Test262 has some coverage of module semantics. We might consider adding those tests to this project - i.e., for each test which contains an import or export, compile it with this project, tack on the necessary harness files, and then run it in a JS engine to ensure the asserts all hold.

Thoughts?

[Protryon]

Fixed

[bakkot]

var declarations in a module are not global. They are in scripts. As such, I believe this should all be wrapped up in an IIFE:

(function(){
  "use strict";
  var d = "d";
  var c = "c" + d;
  var b = "b" + d;
  var result = "a" + b + c;
})();

This also removes the need for the ThisUndefiningReducer, because this is undefined in IIFEs with strict bodies.

[Protryon]

@bakkot The test262 module integration seems like a good idea, will implement.

[Protryon]

This currently does pass all tests, however, this requires shapesecurity/shift-java#216 and shapesecurity/shift-java#214 to properly parse and early error all tests.

[bakkot]

@Protryon the test262 tests would be a lot more valuable if we actually executed the resulting bundle, rather than just compiling it.

[Protryon]

@bakkot Ready for review

[bakkot]

Is there any way to the nice short output a previous version of this PR had? I know it's not correct in edge cases, but sometimes that's an acceptable tradeoff given the overhead here.

The short output being something like

(function(){
  "use strict";
  var d = "d";
  var c = "c" + d;
  var b = "b" + d;
  var result = "a" + b + c;
  return { result: result };
})();

[Protryon]

Added a dangerLevel options, an enum which can be SAFE (previous operation), BALANCED (pretty much what you posted for example, but asserts imports are read-only), and DANGEROUS (what we had before, and exactly what you posted for example, with the object being predeclared before returning).

[bakkot]

Nice! Do you think it would be possible to statically identify some subset of programs where a theoretically dangerous level is in fact safe? For example, if there's no cycles, no import *, and no attempt to write to an imported binding, does that make it the BALANCED level actually 100% safe? If so, a good followup (not necessarily part of this PR) would be to automatically choose a more efficient strategy when it's safe to do so.

[Protryon]

If there are no cycles/self-imports, it's safe to assume there are no TDZ issues, however BALANCED also currently removes the Symbol.toStringTag entry on the module full-namespace import. This would require no occurrences of import * as X (as opposed to import *), or require us to assert that X[Symbol.toStringTag] is never accessed, which can be done in some cases (say only StaticMemberExpressions were the only accesses of the namespace reference ).

DANGEROUS mode could be identified by enabling it and having a different read-detection response. For example, if there is a possible read, throw a bundle-time error rather than inserting a lambda with a TypeError throw statement.

[bakkot]

would require no occurrences of import * as X

Sorry, yeah, that's what I meant.

say only StaticMemberExpressions were the only accesses of the namespace reference

And exported module does not export any (non-arrow) functions which refer to this or eval, which can also give you a reference to the namespace object. Probably this analysis is more trouble than it's worth, but the basic "there are no namespace exports" one seems doable.

For example, if there is a possible read

Do you mean a possible write, rather than a possible read?

---

I guess any such analysis also needs the assumption that each exported binding is written to exactly once, right? (Or I guess, to be precise, that there are no writes to exported bindings inside of closures; multiple writes at the top level would be acceptable.) Otherwise the whole "live binding" complication comes into play. Still seems doable though.

[Protryon]

Yes, I mean't possible write. The exported binding can be written as much as desired from the exporting class, however, the imported binding in that module or others is unwritable (as opposed to immutable, as any changes from the underlying module will be immediately reflected in the imported binding). EDIT: As such, the analysis is quite easy: look for an AssignmentTarget that references an import-specifier-defined binding. No more analysis is needed to detect a TypeError will be thrown when executed, assuming no dynamic scope overriding declarations via eval is going on.

At present, DANGEROUS mode just turns off this TypeError generation. The advantage to this is, more or less, nothing. If this is present in a script, the purpose of succeeding in compilation is only to throw a TypeError immediately, as would normally happen in such a circumstance. Moving this behavior to a compile time error is reasonable, for say, BALANCED mode, rather than ignoring it as in DANGEROUS mode.

[michaelficarra]

Maybe you'll want to include an error string that refers to the unresolved binding?

[michaelficarra]

Can we keep one that doesn't require BundlerOptions and just uses some reasonable default?

[michaelficarra]

I think we should make this public. It'd be annoying to have to start from one of NODE_OPTIONS or SPEC_OPTIONS.