Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add tests for Source Phase Imports #3980

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

legendecas
Copy link
Member

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also add a test to verify that the import source declaration syntax is supported:

// test.js
import source x from "x";
import "./ensure-linking-error_FIXTURE.js";

throws in the resolution phase and not in the parse phase (e.g. https://github.com/tc39/test262/blob/main/INTERPRETING.md#negative)

where that fixture file is https://github.com/tc39/test262/blob/main/test/language/module-code/import-attributes/ensure-linking-error_FIXTURE.js

--

We should also test that Source Text Module Records do not have a source representation:

assertRejects(import("./a-js-file_FIXTURE.js"), ReferenceError);
// negative:
// - phase: resolution
// - type: ReferenceError
import source s from "./a-js-file_FIXTURE.js";

@nicolo-ribaudo
Copy link
Member

cc @guybedford

@legendecas legendecas force-pushed the module-source branch 3 times, most recently from c97be5d to 139d854 Compare January 9, 2024 08:04
Copy link

@guybedford guybedford left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Appreciated for your patience with the review. Finally had a look through and it's seeming really thorough to me.

Perhaps we should move abstract-module-source and its prototype tests to test/built-ins?

The invalid and valid tests could also include a test that import.source cannot be assigned. See some of the import.meta tests for examples. We could also ensure import.source.x, typeof import.source etc remains invalid.

I would note that we are still aiming to specify import.source for ECMAScript SourceText module records, so that reducing the number of tests that would need to change / be removed when that happens would be useful. Rather than the suite of these failures in the catch tests I think just a few would suffice if they will be changed anyway.

Finally for the static import syntax tests, it might be beneficial to also include some of the valid cases of the source and from identifier names being imported for the source import phase per the various discussions we had around these syntax cases.

test/language/module-code/abstract-module-source/length.js Outdated Show resolved Hide resolved
@legendecas
Copy link
Member Author

@guybedford thanks for the suggestions, updated!

@legendecas
Copy link
Member Author

This PR doesn't include import-attributes with source phase imports. Once it is normatively specified, the test can be added as well.


$DONOTEVALUATE();

import source x from '';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this first one?


$DONOTEVALUATE();

import source source from '';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no requirement that an empty specifier will fail resolving. Can we use something like from '<do not resolve>', and explicitly document it in https://github.com/tc39/test262/blob/main/INTERPRETING.md?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants