Skip to content

Always error on an attempt to use await as an identifier in modules #55503

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.

Sign up for GitHub

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

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

Always error on an attempt to use await as an identifier in modules #55503

wants to merge 1 commit into from

Conversation

Andarist
Copy link
Contributor

fixes #8559

@typescript-bot typescript-bot added the For Backlog Bug PRs that fix a backlog bug label Aug 24, 2023
Andarist
Andarist commented Aug 24, 2023
View reviewed changes
src/compiler/binder.ts
else if (node.flags & NodeFlags.AwaitContext) {
file.bindDiagnostics.push(createDiagnosticForNode(node, Diagnostics.Identifier_expected_0_is_a_reserved_word_that_cannot_be_used_here, declarationNameToString(node)));
}
else if (originalKeywordKind === SyntaxKind.AwaitKeyword && (node.flags & NodeFlags.AwaitContext || isExternalModule(file))) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps this would deserve a new diagnostic. The other one (with node.flags & NodeFlags.AwaitContext) didn't have a specific diagnostic meant for it so I skipped adding one here as well.

I think though that it could be nice to add a specific diagnostic here (and maybe add a specific one for that other case as well while at it). If you agree - should this be a complete new diagnostic code or could the existing code be reused? Probably a new one makes more sense.

Copy link
Member

Choose a reason for hiding this comment

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

I believe technically this is allowed in non-strict CJS, but I'd rather not make weird exceptions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, it only technically has to be disallowed in modules 😉 Isn't allowing it in TS files with script targets a weird exception already? 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not allowed in non-strict CJS, it's always allowed there. The actual rule is that this is disallowed in modules - not that it's disallowed in strict mode 🤷

@DanielRosenwasser
Copy link
Member

@typescript-bot pack this
@typescript-bot test this
@typescript-bot test top100
@typescript-bot user test this
@typescript-bot run dt

@typescript-bot
Copy link
Collaborator

typescript-bot commented Aug 24, 2023 •
edited
Loading

Heya @DanielRosenwasser, I've started to run the tarball bundle task on this PR at 1c1316e. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Aug 24, 2023 •
edited
Loading

Heya @DanielRosenwasser, I've started to run the diff-based user code test suite on this PR at 1c1316e. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Aug 24, 2023 •
edited
Loading

Heya @DanielRosenwasser, I've started to run the diff-based top-repos suite on this PR at 1c1316e. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Aug 24, 2023 •
edited
Loading

Heya @DanielRosenwasser, I've started to run the parallelized Definitely Typed test suite on this PR at 1c1316e. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Aug 24, 2023 •
edited
Loading

Hey @DanielRosenwasser, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/156921/artifacts?artifactName=tgz&fileId=439A1D3DBB7F02D7502DF1AD0CB1AECB34BE68C66FFFB909AA8AC3D794ABD83C02&fileName=/typescript-5.3.0-insiders.20230824.tgz"
    }
}

and then running npm install.

There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/[email protected]".;

@typescript-bot
Copy link
Collaborator

@DanielRosenwasser Here are the results of running the user test suite comparing main and refs/pull/55503/merge:

There were infrastructure failures potentially unrelated to your change:

  • 1 instance of "Unknown failure"
  • 2 instances of "Package install failed"

Otherwise...

Something interesting changed - please have a look.

Details

rxjs-src

/mnt/ts_downloads/rxjs-src/build.sh

  • [NEW] error TS2428: All declarations of 'WeakMap' must have identical type parameters.
    • /home/vsts/work/1/s/typescript-55503/lib/lib.es2015.collection.d.ts(62,11)
    • /home/vsts/work/1/s/typescript-55503/lib/lib.es2015.iterable.d.ts(162,11)
    • /home/vsts/work/1/s/typescript-55503/lib/lib.es2015.symbol.wellknown.d.ts(140,11)
    • /home/vsts/work/1/s/typescript-55503/lib/lib.es2015.collection.d.ts(62,11)
    • /home/vsts/work/1/s/typescript-55503/lib/lib.es2015.iterable.d.ts(162,11)
    • /home/vsts/work/1/s/typescript-55503/lib/lib.es2015.symbol.wellknown.d.ts(140,11)
    • /home/vsts/work/1/s/typescript-55503/lib/lib.es2015.collection.d.ts(62,11)
    • /home/vsts/work/1/s/typescript-55503/lib/lib.es2015.iterable.d.ts(162,11)
    • /home/vsts/work/1/s/typescript-55503/lib/lib.es2015.symbol.wellknown.d.ts(140,11)
    • /home/vsts/work/1/s/typescript-55503/lib/lib.es2015.collection.d.ts(62,11)
    • /home/vsts/work/1/s/typescript-55503/lib/lib.es2015.iterable.d.ts(162,11)
    • /home/vsts/work/1/s/typescript-55503/lib/lib.es2015.symbol.wellknown.d.ts(140,11)
    • /home/vsts/work/1/s/typescript-55503/lib/lib.es2015.collection.d.ts(62,11)
    • /home/vsts/work/1/s/typescript-55503/lib/lib.es2015.iterable.d.ts(162,11)
    • /home/vsts/work/1/s/typescript-55503/lib/lib.es2015.symbol.wellknown.d.ts(140,11)
  • [MISSING] error TS2428: All declarations of 'WeakMap' must have identical type parameters.
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.collection.d.ts(62,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.iterable.d.ts(162,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.symbol.wellknown.d.ts(140,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.collection.d.ts(62,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.iterable.d.ts(162,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.symbol.wellknown.d.ts(140,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.collection.d.ts(62,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.iterable.d.ts(162,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.symbol.wellknown.d.ts(140,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.collection.d.ts(62,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.iterable.d.ts(162,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.symbol.wellknown.d.ts(140,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.collection.d.ts(62,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.iterable.d.ts(162,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.symbol.wellknown.d.ts(140,11)

@typescript-bot
Copy link
Collaborator

@DanielRosenwasser Here are the results of running the top-repos suite comparing main and refs/pull/55503/merge:

Everything looks good!

@typescript-bot
Copy link
Collaborator

Hey @DanielRosenwasser, it looks like the DT test run failed. Please check the log for more details.
You can check the log here.

@sandersn sandersn requested a review from navya9singh September 7, 2023 22:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DanielRosenwasser DanielRosenwasser DanielRosenwasser left review comments

@navya9singh navya9singh Awaiting requested review from navya9singh

At least 1 approving review is required to merge this pull request.

Assignees

@navya9singh navya9singh

Labels
For Backlog Bug PRs that fix a backlog bug
Projects
PR Backlog
Status: Waiting on reviewers
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ES6 modules cannot use "await" as an identifier
4 participants
@Andarist @DanielRosenwasser @typescript-bot @navya9singh