Reparse top level 'await' in modules #39084
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
|
The relevant commits start at 8ab882a |
weswigham
approved these changes
Jun 17, 2020
|
|
||
| // If we parsed this as an external module, it may contain top-level await | ||
| if (isExternalModule(sourceFile) && sourceFile.transformFlags & TransformFlags.ContainsPossibleTopLevelAwait) { | ||
| sourceFile = reparseTopLevelAwait(sourceFile); |
There was a problem hiding this comment.
Hm. Should we do something smarter when we're doing an incremental parse, like only visiting and reparsing the changed subtree?
There was a problem hiding this comment.
The biggest issue with incremental parse is that it depends in part on differences in the context flags, and whether the top-level is an Await context is dependent on whether we treat it as a module. What happens when the changed subtree includes a new export {} that turns the file into a module? What I might do is hold onto a pointer for the non-reparsed source file to pass to incremental parse, and then just handle reparse individually as needed. We might not get as much node reuse inside of expressions, but incremental parse node reuse only applies to a limited set of list contexts (statements, class members, switch clauses, etc.) and is ignored at the expression level.
| if (node && node.transformFlags & TransformFlags.ContainsPossibleTopLevelAwait) { | ||
| if (isExpression(node)) { | ||
| return speculationHelper(() => { | ||
| scanner.setTextPos(node.pos); |
There was a problem hiding this comment.
Would using initializeState and passing in a syntax cursor here allow us to reuse the initially parsed nodes where possible, even in the case of a partial reparse? (Reparse due to context invalidation and incremental reparsing I would think seem similar)
There was a problem hiding this comment.
Possibly. I'm on the fence as to whether I wanted to set original pointers for these nodes. I need to add some incremental parse test cases.
There was a problem hiding this comment.
I considered using incremental parse, but it doesn't apply at the expression level, only at certain list context levels (see #39084 (comment)). I might have been able to leverage it by reparsing the statement itself, but some of the necessary incremental parse machinery isn't currently accessible from within Parser, and would be fairly complex to wire up properly.
|
@weswigham would you mind giving this one more look over now that I've added some additional tests and a few changes? |
|
@typescript-bot perf test |
|
Heya @rbuckton, I've started to run the perf test suite on this PR at 097972f. You can monitor the build here. Update: The results are in! |
|
The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master. |
|
@rbuckton Here they are:Comparison Report - master..39084
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
weswigham
approved these changes
Jun 18, 2020
|
I'll add one. |
cangSDARM
added a commit
to cangSDARM/TypeScript
that referenced
this pull request
Jun 23, 2020
* upstream/master: (58 commits) Variadic tuple types (microsoft#39094) chore: resolve suggestions Expand auto-import to all package.json dependencies (microsoft#38923) inline local functions Update bigint declaration file (microsoft#38526) Update user baselines (microsoft#39077) LEGO: check in for master to temporary branch. Add missing index.ts files to user projects (microsoft#39163) Add reason for a disabled code action (microsoft#37871) Minor fix for assertion predicates (microsoft#38710) Update LKG (microsoft#39173) Reparse top level 'await' in modules (microsoft#39084) change chore: more change chore: resolve review chore: save space fix: lint error test: add test for it chore: make isJsxAttr required chore: revert change in checker ... # Conflicts: # src/compiler/binder.ts # src/compiler/checker.ts # src/compiler/parser.ts # src/compiler/types.ts
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.
This adds support to the parser to re-parse expressions containing
awaitas an identifier after we have parsed a source file and determined it to be a module. Cases covered include:await (x)await <T>(x)await ``await <T>``await[x]await ()- Expression expected at)await (x, )- Expression expected at)await <T, U>(x)->expected at,await .x- Expression expected at.await ?.x- Expression expected at?.await ?.[x]- Expression expected at?.const await = ...- Identifier expected atawaitThis depends on #35282
Fixes #36817