Replace invalid import attributes assertion

[nicolo-ribaudo]

When creating a module script, there is no guarantee yet that the module's dependencies only have valid import attribute keys. Instead of asserting that only type is used, this commit introduces proper validation throwing a SyntaxError.

I was on the fence between SyntaxError and TypeError. ECMA-262 throws a SyntaxError for invalid attributes because it throws a SyntaxError for all the errors that happen before evaluation, but HTML throws a TypeError for invalid values of the "type" attribute. I ended up picking SyntaxError just because it's what ECMA-262 already does.

I found this bug while updating the import attributes implementation in Chromium. I'm not sure if this need explicit implementer interest given that the previous spec was just wrong.

Closes tc39/proposal-import-attributes#144.

---

• At least two implementers are interested (and none opposed):
  • Assumed yes as part of the consensus in TC39 on the import attributes proposal
• Tests are written and can be reviewed and commented upon at:
  • [html] Add test for import declaration errors order web-platform-tests/wpt#41581
• Implementation bugs are filed:
  • Chromium: …
  • Gecko: …
  • WebKit: …
  • Deno (only for timers, structured clone, base64 utils, channel messaging, module resolution, web workers, and web storage): …
  • Node.js (only for timers, structured clone, base64 utils, channel messaging, and module resolution): …
• MDN issue is filed: Not necessary for this level of detail

(See WHATWG Working Mode: Changes for more details.)

---

/infrastructure.html ( diff )
/webappapis.html ( diff )

[domenic]

It seems like this implements (1) from tc39/proposal-import-attributes#144, right? Can you give a bit more detail on the reasoning process there, and in particular how this impacts static vs. dynamic imports?

Hmm no, I guess this is more like (1), but then adding an extra check to skip unnecessary work?

Are there tests you've written that show the choice of (1) vs. (2)?

[domenic]

Editorial touchups in the note, and still trying to make sure I understand the relationship to tc39/proposal-import-attributes#144 and test coverage, but this seems pretty solid.

[nicolo-ribaudo]

Right, this PR does (1).

Consider these three examples:

import "./valid.js";
import "INVALID-URL";

import "./valid.js";
import "./valid2.js" with { type: "invalid-type" };

import "./valid.js";
import "./valid2.js" with { invalid: "invalid-attribute" };

For the first two examples, HTML currently first loads the imported specifiers and the type attribute, and then if it succeeds it passes the module to ECMA-262. This means that the first two example will throw an error before passing the module to ECMA-262.

On the other hand, for the third example HTML does not currently do any pre-validation of import attribute keys before passing the Module Record to ECMA-262, and instead it relies on ECMA-262 to validate them. This means that:

• HTML will pass that module to ECMA-262
• ECMA-262 will start the loading process for ./valid.js
• ECMA-262 will start the loading process for ./valid.js, and it will immediately throw an error because of the invalid attribute

This PR aligns the third case with the other twos, by also validating the attribute keys when validating the type and the specifier. I am not sure if this change can be tested, given that the loading process fails anyway and the only difference is whether or not HostLoadImportedModule("./valid.js") will be called before throwing about the invalid attribute.

This PR does not just introduce that check, but it specifically introduces it as the first check. This aligns the errors priority of static imports with the one of dynamic imports, as mentioned in tc39/proposal-import-attributes#144 (comment). This can currently be tested (web-platform-tests/wpt#41581) because, following what ECMA-262 does, validation of unknown attribute keys throws a SyntaxError. If it was a TypeError, there would be no way to test it (other than somehow checking the error messages).

[nicolo-ribaudo]

I pushed a new test to web-platform-tests/wpt#41581. The first two tests in each file verify the order in which the import declaration checks happen, while the third one verifies that the "unknown attributes check" is present. Before this PR, this code:

import "./valid" with { unknown: "foo" };
import "INVALID";

would throw about the "INVALID" specifier before validating the unknown attribute.

[domenic]

Thanks for the additional explanation, and tests!

[domenic]

For any implementation which already has import attributes or assertions, but fails to implement this edge case correctly (per your tests), please file a bug and link to it in the OP. Otherwise, we can assume this will get done correctly as part of the general import attributes work.

[nicolo-ribaudo]

I am implementing this in Chromium. I'll check what's the status in Webkit and Gecko. Both Deno and Node.js do not support the new semantics yet.