Reparse top level 'await' in modules#39084
Uh oh!
There was an error while loading. Please reload this page.
Conversation
rbuckton commented Jun 16, 2020
The relevant commits start at 8ab882a |
weswigham left a comment
weswigham left a comment There was a problem hiding this comment.
Looks good, just some small questions/comments.
Also: are there any tests for the decorators, eg, @await and @(await whatever) and/or @await(whatever)?
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| // 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.
Uh oh!
There was an error while loading. Please reload this page.
rbuckton commented Jun 18, 2020
@weswigham would you mind giving this one more look over now that I've added some additional tests and a few changes? |
rbuckton commented Jun 18, 2020
@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! |
typescript-bot commented Jun 18, 2020
The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master. |
typescript-bot commented Jun 18, 2020
@rbuckton Here they are:Comparison Report - master..39084
System
Hosts
Scenarios
| |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
weswigham left a comment
weswigham left a comment There was a problem hiding this comment.
Looks good; would it also be worth having an incremental test of adding/removing the possibly-await-thing, while the script/moduleness remains the same?
rbuckton commented Jun 18, 2020
I'll add one. |
* 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
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