Module support fixes and improvements

[christianrondeau]

Based on #1097 but in a new PR to facilitate collaboration, as proposed

Not all tests are green yet, I might skip some but I have a few questions. If you're motivated, a review at this point would not be a bad idea. I'll make a separate comment for one of my questions.

[christianrondeau]

@lahma I have one test that I'm not sure how to deal with. Check out the test instn-iee-bndng-cls.js. It imports instn-iee-bndng-cls_FIXTURE.js which contains this code:

try {
  A;
} catch (error) {
  results.push(error.name, typeof A);
}

The problem here is that typeof A currently throws with ReferenceError but based on the test, it should instead return undefined but only when referenced with typeof. In JintUnaryExpression for TypeOf there's _argument.Evaluate(context); which throws but shouldn't. The cheap option is to add a try/catch  in JintUnaryExpression so specifically ReferenceError simply returns undefined instead, but the specs doesn't catch errors, and it does say to throw... so I feel there's something off in that path. Node behaves like this (e.g. typeof AnythingHere returns undefined as the test says).

This is unrelated to modules but it breaks multiple tests. If you want to take a look, I think it's pretty legit to be able to do typeof SomeType to check if it has been imported or not by other scripts.

[christianrondeau]

Another "unrelated" problem, instn-local-bndng-cls.js checks for bindings before they are initialized, e.g.:

// This should throw a ReferenceError but it does not
typeof MyClass;
class MyClass {}

I think one part of the problem is that the ScriptWalker in HoistingScope does not actually check for Nodes.ClassDeclaration, only FunctionDeclaration (and they don't share any base class), I'll probably try to add that and see if that helps, but why the previous case isn't supposed to throw is beyond me for now...

And if you like generators, instn-local-bndng-gen.js might trigger some ideas too :)

At the moment I'm investing all of my time on this, but I don't think I'll be able to keep this up much longer. I'll try tomorrow to identify a "skip list" with reasons so that we can merge what's currently done and split the different issues (some of which I'll try to work on when I can).

[lahma]

I can investigate the typeof case this evening, I'll report back when I'm wiser.

[lahma]

I can also fix the hoisting scope, I think I found the problem like you already analyzed, wrong handling. Probably will fix against main.

[lahma]

@christianrondeau can you rebase against main and check if any of your problems has gone away? I can work on next specific issues which you want to report/help with.

[lahma]

It seems that if you remove the SetValue additions in JintAssignmentExpression it's down to 7 failing tests.

[christianrondeau]

Rebased and that helped! There are only two things remaining as far as I can tell, I can probably figure them out but I feel like you might know. If you don't, let me know and I'll take a look, but otherwise I think the PR will be ready for review after those!

typeof on a variable before initialization should return undefined

• language\module-code(sourceFile: language/module-code/instn-iee-bndng-cls.js)
• language\module-code(sourceFile: language/module-code/instn-iee-bndng-const.js)
• language\module-code(sourceFile: language/module-code/instn-iee-bndng-fun.js)
• language\module-code(sourceFile: language/module-code/instn-iee-bndng-let.js)
• language\module-code(sourceFile: language/module-code/instn-iee-bndng-var.js)

Explained here: #1102 (comment)

Generators in modules

• language\module-code(sourceFile: language/module-code/instn-local-bndng-export-gen.js)
• language\module-code(sourceFile: language/module-code/instn-local-bndng-gen.js)

I didn't check them in details since I don't know how generators work in the first place :)

[lahma]

Generators in modules

These can be ignored (skipped.json). Generators haven't been implemented in main so I wouldn't expect them not to work in modules either. I guess they are missing generators feature flag.

I tried latest commit and I still think the SetValue additions (which look a bit scary) can be removed, failed test count won't change.

[lahma]

@christianrondeau I'll have a stab at the remaining issues unless you are currently working on it, can't promise timeline though.

[christianrondeau]

I'll remove the SetValue additions. That came from https://262.ecma-international.org/5.1/#sec-11.13.1 (the note contained specifications, I also found it weird that they were outside of the actual implementation text but it fixed some tests), but I guess another fix down the line solved that same problem since you're right, removing that code doesn't flag any new tests... 🤷 Code removed! I also added the generator tests to skipped.json, which means only the typeof issue remains!

[lahma]

Great! Beware of using the 5.1 spec though, some older code in Jint is based on that thus the old spec will have the items and clarifications, but all new code is using the "living spec" so they might be in conflict (though working at the moment together).

[christianrondeau]

And yes I'll let you check for typeof, in case you intuitively know what to do about it. If not, let me know and I'll investigate.

[christianrondeau]

Oh and about typeof, I did take a quick look to see why your commit 298ff59ac0017ae8fbf8802883f8fa0377d6e6bf didn't fix it, there's still v = engine.GetValue(rf, true); which fails. I feel like the "correct" fix would be to either try/catch there or to return an abrupt completion result instead of throwing, since accessing the binding itself isn't obvious from that specific place in the code.

[christianrondeau]

Actually no, otherwise that wouldn't throw: forget this, that would throw in _argument.Evaluate(context) but I still find weird to add a try/catch that's not in the specs

typeof (function() { throw "oops"; }())

And there's no catch either in the specs.

I also checked and IsUnresolvableReference doesn't seem to be doing binding checks for ObjectEnvironmentRecord.

There's something that I'm missing in the specs...

[lahma]

I'm also lost here. Tried to debug and understand, no big victories yet. Based on the test code:

export { A as B } from './instn-iee-bndng-fun.js';

// Taken together, the following two assertions demonstrate that there is no
// entry in the environment record for ImportName:
export const results = [];
try {
  A;
} catch (error) {
  results.push(error.name, typeof A);
}

A should throw reference error because it should not be initialized in this context, only in caller context. But then using typeof against it should return a Reference that has undefined as base - should in-progress module be undefined for its exports or something? This might boil down to module environment record returning whether is has binding or not. But again, I'm at loss.

My first thought was that everything should be exported lazily, but then again the results is used here so it should be visible in module record.

I assume that typeof is good, but the logic creating a reference might be lacking...

[christianrondeau]

Yep...

1. Typeof should call GetValue: https://tc39.es/ecma262/#sec-typeof-operator-runtime-semantics-evaluation
2. GetValue should call GetBindingValue: https://tc39.es/ecma262/#sec-getvalue
3. GetBindingValue always throws if the value is uninitialized https://tc39.es/ecma262/#sec-module-environment-records-getbindingvalue-n-s

Looking at another implementation:

1. https://github.com/engine262/engine262/blob/063418d65006290c4f4ba60b685e0793638dfa99/src/runtime-semantics/UnaryExpression.mjs
2. https://github.com/engine262/engine262/blob/063418d65006290c4f4ba60b685e0793638dfa99/src/abstract-ops/reference-operations.mjs#L57
3. https://github.com/engine262/engine262/blob/063418d65006290c4f4ba60b685e0793638dfa99/src/environment.mjs#L798

Same thing... and they do have the same test: https://github.com/tc39/test262/blob/991a38932aefeac2300b4c52b573597107467406/test/language/module-code/instn-iee-bndng-cls_FIXTURE.js so one option would be to clone and debug that same test with their code, see where it follows a different path... I won't be able to do that right away, but that would certainly be one way to figure it out.

[christianrondeau]

So, I was able to debug using engine262, and in the case of typeof A;, the reference.base is unresolvable... since I have everything setup I'll try and figure out why the reference is not unresolvable on our end and we should be good to go :)

[christianrondeau]

I think I got it. There's a bogus binding for A. export { A as B } should not create a binding for B, and here it throws with A because it's actually trying to bind A in the parent module... in other words, typeof is fine.

[christianrondeau]

VICTORY! The problem was actually something stupid I did in the original PR (I had no idea what I was doing). All tests are green 🎉

I also fixed dynamic import (import('module')), to be honest it's a little bit hackish because it directly calls ImportModule, which waits for the module to be fully resolved. It should instead return the Evaluate() promise, but since we don't have any await or real async continuations, the behavior would end up exactly the same so I added a note and kept it like this.

I think we're good for a review and merge at this point (there are certainly improvements but at least main will be compliant)

[lahma]

Great work! Thanks for pushing this trough with your valuable insights. I threw in last tweak commit and now everything looks good to my eye.

[christianrondeau]

Woot! Now the next logical step will be to support async/await but that will be for another time :D Thanks for your help!

[lahma]

Before async/await I'm afraid we need to get #824 done. I has constructs (or tries to have) for resuming on contexts etc. Also moving closer to spec and has hooks for places where async should also be considered. It's a chore...

[christianrondeau]

I was specifically talking about File.ReadAllText in DefaultModuleLoader, which would already make the whole call tree async (i.e. return tasks), not the await within JavaScript (though that would probably be a good thing). The main reason being that doing async file loading (io pipes) rather than blocking threads would be much better under volume.

I took a quick look at the impact, and it changes a lot of methods to Task<>.

I think the cleaner way to deal with that would be to indeed return a task in the IModuleLoader, but have the task added to a list in Engine that'll be awaited in ImportModule only. That would also be more in line with how Node works... anyway food for thought, this is also out of scope for this specific PR.

[lahma]

Yup, native Task with .NET can kill performance in interpretation side as it's viral, so wouldn't want awaits in normal code flow (executing statements etc). Just a heads up, there's also the Gitter channel if you want free-form brain-storming.