Restore import defer = parsing
#61837
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
typescript-bot
added
the
For Uncommitted Bug
|
This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise. |
1 similar comment
|
This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise. |
There was a problem hiding this comment.
Pull Request Overview
This PR restores the parsing of the "defer" identifier in import equals declarations and enhances the test coverage for various scenarios involving "defer" in import statements.
- Adds new test cases for valid and invalid "defer" usage in various module systems.
- Updates baseline outputs to reflect the corrections.
- Refines the parser logic in src/compiler/parser.ts to correctly distinguish "defer" as a phase modifier.
Reviewed Changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tests/cases/conformance/importDefer/*.ts | Added tests for "defer" usage in import equals and import declarations. |
| tests/baselines/reference/* | Updated baselines reflecting the new parsing behavior. |
| src/compiler/parser.ts | Modified the condition for recognizing "defer" to improve parsing accuracy. |
Comments suppressed due to low confidence (2)
src/compiler/parser.ts:8396
- [nitpick] The compound condition for detecting a deferred import is complex; adding an inline comment or refactoring using intermediate variables could improve code clarity.
identifier?.escapedText === "defer" && (token() === SyntaxKind.FromKeyword ? !lookAhead(nextTokenIsStringLiteral) : token() !== SyntaxKind.CommaToken && token() !== SyntaxKind.EqualsToken)
src/compiler/parser.ts:8396
- [nitpick] Consider caching the result of token() in a local variable before the condition to ensure consistency and potentially reduce function calls.
identifier?.escapedText === "defer" && (token() === SyntaxKind.FromKeyword ? !lookAhead(nextTokenIsStringLiteral) : token() !== SyntaxKind.CommaToken && token() !== SyntaxKind.EqualsToken)
|
@typescript-bot test it |
|
Hey @jakebailey, the results of running the DT tests are ready. Everything looks the same! |
|
@jakebailey Here are the results of running the user tests with tsc comparing Everything looks good! |
|
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
jakebailey
reviewed
Jun 9, 2025
| @@ -8392,7 +8392,10 @@ namespace Parser { | |||
| phaseModifier = SyntaxKind.TypeKeyword; | |||
| identifier = isIdentifier() ? parseIdentifier() : undefined; | |||
| } | |||
| else if (identifier?.escapedText === "defer" && token() !== SyntaxKind.FromKeyword) { | |||
| else if ( | |||
There was a problem hiding this comment.
Why isn't this just the same as what's done for type above? You can't mix type and defer, so I would have expected both to parse in the same fashion.
There was a problem hiding this comment.
The difference is that the type parsing considers type as a modifier in import type foo =, while the defer parsing doesn't in import defer foo =.
Do you prefer if I unify the path, but then add a "nice" error for import defer foo = rather than just complaining about the unexpected foo/=?
There was a problem hiding this comment.
Daniel pointed out there's a difference between them in other ways like import type foo, { bar } from "..." is not legal as it's ambiguous whether or not bar is type or not. So, all good.
There was a problem hiding this comment.
Well technically we could still unify parsing for them and report that type case as an "early error" while still building the proper AST.
(I'm happy to both keep the code as-is or to update it)
There was a problem hiding this comment.
After thinking more, technically neither allow named bindings or defaults. I think this is okay as-is. I'll make a second PR to add more tests.
|
@jakebailey Here are the results of running the top 400 repos with tsc comparing Everything looks good! |
DanielRosenwasser
approved these changes
Jun 9, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #60757 (comment)
I also added a bunch more tests for when using
deferas a binding identifier in ES import declarations. Note thatimport defer from from "foo"is invalid, but I added a test to verify that it reports the same error as forimport defer foo from "foo".cc @jakebailey