Closed Bug 1362098 Opened 4 years ago Closed 4 years ago

Module loading errors should include source position information


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




Tracking Status
firefox57 --- fixed


(Reporter: jonco, Assigned: jonco)




(4 files)

There are currently TODOs in HandleResolveFailure and HandleModuleNotFound for this.

Bug 1362097 will provide a way of getting this information.
Priority: -- → P2
Assignee: nobody → jcoppeard
Summary: Errors thrown by HostResolveImportedModule should include source position information → Module loading errors should include source position information
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.
Attachment #8900206 - Flags: review?(till)
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.
Attachment #8900208 - Flags: review?(till)
Update the script loader to set source position information when handling module resolution failures, using the APIs added by the previous patch.
Attachment #8900210 - Flags: review?(bkelly)
Attached patch bug1362098-testsSplinter Review
Add tests to exercise all of the above.
Attachment #8900212 - Flags: review?(bkelly)
Duplicate of this bug: 1362097
Comment on attachment 8900206 [details] [diff] [review]

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

Attachment #8900206 - Flags: review?(till) → review+
Comment on attachment 8900208 [details] [diff] [review]

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.
Attachment #8900208 - Flags: review?(till) → review+
Comment on attachment 8900210 [details] [diff] [review]

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.
Attachment #8900210 - Flags: review?(bkelly) → review+
Attachment #8900212 - Flags: review?(bkelly) → review+
Pushed by
Report source position information for module export resolution failures r=till
Add source position to requested module information r=till
Report source position for module specfier resolution failure r=bkelly
Add tests for module error reporting r=bkelly
You need to log in before you can comment on or make changes to this bug.