Module handling fixes and improvements

[lahma]

• tests are no using normal engine execution without extra host
• engine overloads for Evaluate and Execute to allow supplying script type

[lahma]

@christianrondeau I made some changes for test running in this PR and it surfaced some problems, 85 failing tests. I was able to fix some and now it's down to 59. I also implemented the dynamic import part.

If you have time at some point could you have a quick look at this branch? There seems to be some problems with export resolution logic at least, shown by test case like language/module-code/namespace/internals/define-own-property.js where indirect/renamed handling isn't registered correctly to module namespace exports.

[lahma]

I think there's something fishy going on with module resolution/registration w.r.t. to exported name vs local name, also seems that resolution doesn't follow spec by always using ExportedName but if does some fallbacks...

[christianrondeau]

I'm not surprised, there were a few things that I was didn't expect to see working without any actual implementation but actually did work. Since a lot of code was already there, I think some code worked "by chance", e.g. since the importing module has access to the exported scope if the name was bound, I didn't actually need to do much in the actual import/export statements. I'll run the tests and review your changes as soon as I can. For now for some reasons the tests just won't start (they spin and won't start, probably something on my end) but I'll reach out when I have both running tests and time to look at them :D

[lahma]

Great, thank you. No hurry and I can try to contribute too but I think you have already gone through the digesting of the resolution logic so might be easier for you.

[christianrondeau]

I think global wasn't implemented and many tests rely on it. Also it looks like this specific error might not actually be thrown all the way up, I'm going to take a deeper look now. How should I contribute back in the branch? For now I'll branch out your branch and manually ping you if I have something worthwhile?

[christianrondeau]

Aaaah also I see you added engine.Execute() for the module, but you cannot execute a module directly, you have to import it. I'm not sure if/how we can throw a clearer error when trying to export in a non-module context. We can circle back to this once the tests are green.

[christianrondeau]

I see you added a few methods to Engine.Modules.cs, to allow running Execute directly on a module. Those methods currently break self-referencing and cyclic referencing, and IMHO add confusion between "ImportModule" overloads and "Execute". I wrote a few pages of text explaining why in the previous PR. For example, if you import the same module twice, it should give you the same live bindings, since from the C# code you are actually "importing" the ecmascript module's bindings.

Are you OK with me re-renaming some things and removing some of your overloads? Once it's green we can either rename all overloads if you want (Execute will cause conflicts though) or re-open the discussion about mixing modules and scripts.

[lahma]

Sure, go agenda. Overloads were mostly meant to allow running regular code in module context , allowing imports to work.

[christianrondeau]

Cool, I'm untangling a few threads (I'm starting with self-evaluation not behaving correctly, I'll reach out once I made sense of this)

• Module was only added to the cache after it was evaluated, which is a problem
• New Execute overloads did not eagerly cache the executing "module"
• Error line number is lost during Evaluate (not sure what the solution is yet, the line is in lost in JsModule: capability.Reject.Call(Undefined, new[] { result.Value });)
• The previous problem makes it harder to figure out what I get a "TypeError: null" when writing to a property of an imported module object (this is where I'm at, I'll have to continue later)

[lahma]

I think Execute as concept would be nice for end users, that would be their entry point allowing regular JS script code.

In hindsight Evaluate makes less sense as it always returns the module namespace.

Thanks for looking into this.

[christianrondeau]

I have a few more green tests, you can track my work here: https://github.com/christianrondeau/jint/tree/module-improvements

About Execute, I understand why it seems to make sense at first glance (I wanted to do that first), but it's actually a rabbit hole. Modules do not behave in the same way as scripts at all, and they don't get evaluated in the same way.

• If you "import" a module twice, it will only be evaluated once; therefore, executing the same code twice will do nothing on the second run.
• Imported variable bindings are created before evaluation, so conflicts with existing variables would have to be dealt with.
• Modules must be strict, so non-strict scripts would have to either "ignore" that or throw.
• A module must have an identity to deal with cyclic dependencies, so the "script" would need to actually have module properties (but it cannot be a JsModule without breaking a lot of the current behavior)
• An option would be for an importing "script" to re-implement the "import" statement to actually create bindings in the script on the fly rather than receiving them from the environment.
• An importing "script" would only allow using absolute paths, or we would need to introduce a new concept of "base path".

It can be done, but I'm afraid it will be much harder to navigate rather than saying, like node does, "you either make a script OR you make a module".

If we only talk about semantics (i.e. you want to use .Execute because it's discoverable), the problem I had when I tried was dealing with specifiers. For example, you can Execute(string code), but for a module, you need Execute(string specifier, string code) and it becomes hard to see what is what. In a way, this vision of Execute is like sugar for AddModule + ImportModule, but I'm wondering whether that sugar really is useful, since you won't need modules if you only plan on having one C#-driven module. The big difference with doing node my-module.mjs (which feels like "execute") is that in C#, once the module code has been evaluated and returns, you're back into the C# context, with the module still "live". Everything falls perfectly in place once you consider the C# code as "importing the module", rather than "executing the module".

The only overload of Execute that would make sense to me would be "Execute(specifier)", but because it conflicts with "Execute(code)" that already exists, you'd need "ExecuteModule(specifier)" which does exactly the same as "ImportModule(specifier)" but without returning the namespace. I'm not sure it's better than simply doing ImportModule.

Another alternative that would require more changes but could make sense, would be to separate Engine and have ModuleEngine. That could work because then you'd have a single run of Execute, after which the whole ModuleEngine would be disposed. For example, "var engine = new ModuleEngine(...); engine.Execute(filename);". But I'm not sure if that's actually very useful.

Some instances of that discussion about "importing" v.s. "executing":

• Support ECMAScript modules (export/import statements, modules definition) #1054 (comment)
• Support ECMAScript modules (export/import statements, modules definition) #1054 (comment) (same question, different answer)
• Support ECMAScript modules (export/import statements, modules definition) #1054 (comment)
• Support ECMAScript modules (export/import statements, modules definition) #1054 (comment)

[lahma]

Thank you for the clarifications, I know you have gone back and forth with the design and the problems a great many times more than I have.

We should probably hide everything that does not make sense, but also try to allow come use cases that feel sound. I think one of them is to use modules in a script, but I guess that has to wait for another day as it's a can of worms like you've described.

In short:

As a user, I would like to use some functionality from module libraries I have found and run them as part of the script.

So I think this boils down to the test case I tried to make, but fails to crystalize the idea as it's in module context and cannot return the value I would want to, like Evaluate. Think formatName as PDF generator library/module for example.

const string script = @"
   import { formatName } from './modules/format-name.js'
   return formatName('John', 'Doe'); 
";

var result = engine.Evalute(script);   

If this were module code, I would give it an ID/specifier Guid.NewGuid().ToString(), I don't need to refer to this code, I just want to run it.

[lahma]

And you can also create a new PR replacing this one if you want to get the test automation results, you can also squash my commits to yours if you want to, I think the most important part is to use the default Test262 testing infrastructure for running.

[lahma]

This might actually be possible by using dynamic import (the function call), it doesn't need to be in module context when called:

There is also a function-like dynamic import(), which does not require scripts of type="module".

So just allowing current module be null in JintImportExpression and ensuring that the call works as expected should suffice for the "normal" use case. The spec says that importing module can be null for HostImportModuleDynamically. So regular scripts would just use this function call syntax.

[christianrondeau]

Aaaah indeed dynamic import avoids most of the variable binding problems by returning a promise rather than touching the lexical context. However you wouldn't be able to do the example you gave, it would be something like (afaik we don't have await):

const string script = @"
   var result;
   import('./modules/format-name.js').then(formatter =>
    result = formatter.formatName('John', 'Doe');
  );
  return result;
";

var result = engine.Evalute(script);

Without await you would have to set a global variable and run another Evaluate

Otherwise, for doing something like you suggested previous, we only need to define a run a module, but that module would still be outside of the context. Henceforth using something like engine.RunModule(...) (the sugar for AddModule + ImportModule) could be one option, another option would be to flag the whole Engine in "module mode", which would make most of the methods throw, some like .Get() would actually do namespace.Get behind the scene, and .Execute would create and run a dynamically-named module (most methods would have an if/else in that case, which is why could end up with a ModuleEngine and a ScriptEngine for example).

Anyway for now I'll try and make tests pass, and I'll do a new PR. I can do a PR right away if you prefer.

[lahma]

Let's put await require as preferred solution to backlog and not worry about it now. You can decide when you feel like PR'ing, I'll close this one in favor of that.

[christianrondeau]

Will do. By the way, I noticed the tests tend to run quite slow because the file system is fully scanned and every file is read even when running a single test. I'd like to implement lazy loading of file content during the tests listing, so debugging is less painful. Do you have objections if I do that in the PR? I can do it and not commit it if you prefer keeping the PR clean.

[lahma]

Maybe we should wait for #1039 , it changes the test execution altogether. Maybe there will still be problems, but the PR has the current goal of dynamically loading current test fixtures from GitHub and generating specific test cases.

[christianrondeau]

I'll keep my changes locally then (it pretty much simply defers loading the code to when the test is actually ran, I'll keep the file locally if eventually you're interested, I'll share the link along with the PR but separately)

Edit: I'll just revert, because file content is used to group tests, my change would also modify how tests are listed, I'll just manually return in Test262Test.csm otherwise it sometimes takes almost a minute to just start the tests sometimes :|

Edit 2: It would also be nice to have reliable tests ordering (not using ConcurrentBag) so I can always see the "next test to check" being the first of the failed tests list.

[christianrondeau]

My head hurts :| export-expname-from-binding-string.js is just crazy hard to follow... I think I might have to review some more of the existing code that I built on top of and try to make it closer to the specifications, wish me luck! :D

Edit: Progress! (Sorry ;) )

[christianrondeau]

I am making some fairly good progress (mostly figuring out that some of the specifications are in notes rather than in the specs themselves, and that node.js doesn't follow ecmascript specifications to the letter...), down to 31 failing tests :)

[christianrondeau]

Just to keep you in the loop, some tests fail with recursive dependencies and other weird edge cases, but they seem to match the specifications as far as I can tell... So I am doing a refactor that splits "JsModule" into "ModuleRecord", "CyclicModuleRecord" and "SourceTextModuleRecord" so it's easier to follow specifications methods. Therefore... again another pretty big PR incoming. Up to now except a few edge cases that I did fix, I'm wondering if we have to implement recursive dependencies at all though. For now I'll see how far I can get with at least refactoring into something that can be compared with ecmascript's specs, and if I still don't find the problem, I'll submit a PR and ask for help.

[lahma]

Sounds good. There's also option to accept "good enough" and utilize skipped.json for items that cause more trouble than it's worth. Until someone reaaaally needs it and is allow willing to create a PR for such edge case 😉

[christianrondeau]

There's a pretty good chance that I use this, there are edge cases that are just there to hurt people I think! But I'd like to figure out at least why recursive modules are in the test cases while the specs seem to assert against it. I'm missing a piece of the puzzle.

[lahma]

there are edge cases that are just there to hurt people I think!

don't get me started with my generators PR where there's yield inside class constructor 😉

[christianrondeau]

Hahaha they are cruel ;) But hey, recursive cyclic imports work on my branch now! <3 The problem was if (_dfsIndex > _dfsAncestorIndex) instead of if (_dfsAncestorIndex > _dfsIndex) from the original code :| It's a matter of pride now ;)

[christianrondeau]

Moved to #1102 (this one can be closed, let me know if you prefer that I make a PR on a branch in this repo instead)