BUG: Module exporting undeclared name should be a syntax error

[rhuanjl]

Another bug I found whilst working on #5759 though I do not currently have a fix planned for this.

Per
https://tc39.github.io/ecma262/#sec-module-semantics-static-semantics-early-errors
It is a Syntax Error if any element of the ExportedBindings of
ModuleItemList does not also occur in either the VarDeclaredNames of
ModuleItemList, or the LexicallyDeclaredNames of ModuleItemList.

POC:

//start.js
import {Fun} from "./other.js";
print(Fun);

//other.js
export {anything as Fun};

Note the same repros if instead of "anything" you use any other undeclared name OR use a global like Number or Array or if you don't alias "anything".

Output with ch: undefined, jsc and v8 throw Syntax Errors
(Can't do an eshost repro at the moment as eshost doesn't yet interact with ch's module flag)

This causes numerous test262 failures.

[rhuanjl]

I've been thinking about how to fix this - it seems it needs to involve adding a step at the end of parsing a module that re-checks any names exported via export {namesHere} without a from clause to confirm if they were locally defined and if not seek back to the relevant line and raise a syntax error.

[boingoing]

Seems like something we should be able to do at name binding. We do push a pidref to 'anything' when we export it, right? When we bind names, we could throw a SyntaxError if we find a referenced name with no declaration.

[rhuanjl]

Would that permit a valid case like this:

export {foo};
var foo = 5;

Whilst catching the invalid cases I put in the first post?

[pleath]

Yes, it would. @boingoing is referring to the way the parser binds names to declarations. The "push" records the fact that there was a reference to the given name in the current scope. Binding takes place when parsing of the current scope is completed. So the reference can lexically precede or follow the declaration.

[rhuanjl]

Thanks @pleath that makes sense.

I was going to make a PR to fix this but see it's now assigned to you @boingoing want me to give it a go still or have you got it?

[boingoing]

@rhuanjl, feel free to take a stab at it if you're motivated. I haven't looked-into the fix yet. We can help if you need some in that area of the code. If you're not feeling it I can take this one on, as well. 👍

[rhuanjl]

I had a go at trying to do this but didn't get far, unless I'm missing something currently no pid ref is pushed for an exported name unless the export is also variable declaration (in which case it can't possibly hit this error).

And I couldn't quite work out how to push a pid ref for the name and have it be accessible in the right place without making it into a variable declaration which would obviously be wrong.

[pleath]

You're right, no pid ref is pushed in this case. It looks like we implemented it without attempting to bind the exported name to a declaration. Either we missed this requirement in the spec or implemented an earlier spec that didn't contain the requirement.

What you'll want to do is call PushPidRef from Parser::AddModuleImportOrExportEntry, passing in the IdentPtr of the local name, and store the resulting PidRefStack* on the export record. Binding will fill in the Symbol* on the PidRefStack. When we later resolve the export entries, a null Symbol* will indicate an undeclared name. Note that this has to be done whether buildAST is false or true, so the caller of AddModuleImportOrExportEntry will have to make the call in both cases.

Does all that make sense?