1362098 - Module loading errors should include source position information

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P2)

Description

[Jon Coppeard (:jonco)]

There are currently TODOs in HandleResolveFailure and HandleModuleNotFound for this.

Bug 1362097 will provide a way of getting this information.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: bug1362098-import-export-errors

Report source position information for module resolution failures.

The patch adds line and column number properties to module import and export entry objects.  These are required for import entries but optional for export entries because we only use this information to report errors against indirect exports (direct exports are checked at parse time).

These are populated with information from the parser by ModuleBuilder.

They are used when we create an error object in Module.js to provide source information.

Comment 2

[Jon Coppeard (:jonco)]

Attachment: bug1362098-requested-modules

Provide source position information for a module's imports.

The patch adds a new RequestedModuleObject that describes an imported module including the module specifier string and the source line and column.  Previously just a string was used for this purpose.

This information is used by the DOM module loader.  Convenience JS APIs are provided for getting the properties of a RequestedModuleObject.

Comment 3

[Jon Coppeard (:jonco)]

Attachment: bug1362098-script-loader

Update the script loader to set source position information when handling module resolution failures, using the APIs added by the previous patch.

Comment 4

[Jon Coppeard (:jonco)]

Attachment: bug1362098-tests

Add tests to exercise all of the above.

Comment 6

[Till Schneidereit [:till]]

Comment on attachment 8900206 [details] [diff] [review]
bug1362098-import-export-errors

Review of attachment 8900206 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

Comment 7

[Till Schneidereit [:till]]

Comment on attachment 8900208 [details] [diff] [review]
bug1362098-requested-modules

Review of attachment 8900208 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, with or without comments addressed.

::: js/src/builtin/ModuleObject.cpp
@@ +283,5 @@
> +
> +    if (!DefinePropertiesAndFunctions(cx, proto, protoAccessors, nullptr))
> +        return false;
> +
> +    global->setReservedSlot(REQUESTED_MODULE_PROTO, ObjectValue(*proto));

Use initReservedSlot (or initFixedSlot) here to get the isUndefined assert, maybe?

@@ +303,5 @@
> +
> +    RootedRequestedModuleObject self(cx, &obj->as<RequestedModuleObject>());
> +    self->initReservedSlot(ModuleSpecifierSlot, StringValue(moduleSpecifier));
> +    self->initReservedSlot(LineNumberSlot, Int32Value(lineNumber));
> +    self->initReservedSlot(ColumnNumberSlot, Int32Value(columnNumber));

I'd use *FixedSlot for uses like this. However, using *ReservedSlot is certainly more in line with the rest of the Modules code, and it doesn't make much of a difference, I guess.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8900210 [details] [diff] [review]
bug1362098-script-loader

Review of attachment 8900210 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/script/ScriptLoader.cpp
@@ +675,5 @@
>      ModuleScript* ms = aRequest->mModuleScript;
>      nsCOMPtr<nsIURI> uri = ResolveModuleSpecifier(ms, specifier);
>      if (!uri) {
> +      uint32_t lineNumber;
> +      uint32_t columnNumber;

Please init these to some default value for sanity's sake.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1d01918122f
Report source position information for module export resolution failures r=till
https://hg.mozilla.org/integration/mozilla-inbound/rev/a953b8493339
Add source position to requested module information r=till
https://hg.mozilla.org/integration/mozilla-inbound/rev/514a186794df
Report source position for module specfier resolution failure r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d84e2c30a41
Add tests for module error reporting r=bkelly

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e1d01918122f
https://hg.mozilla.org/mozilla-central/rev/a953b8493339
https://hg.mozilla.org/mozilla-central/rev/514a186794df
https://hg.mozilla.org/mozilla-central/rev/2d84e2c30a41