Fixed an issue with broken await using declarations in for of loops
#56466
Conversation
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. |
src/compiler/checker.ts
Outdated
| @@ -49476,7 +49476,6 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |||
| return true; | |||
| } | |||
| } | |||
| return false; | |||
There was a problem hiding this comment.
That's the core of the fix for what was reported. for await loops never had a chance to reach checkGrammarVariableDeclarationList call in this function. Perhaps there are some other subtle issues in this function? Shouldn't the rule of thumb be here only to use return true as an early return?
There was a problem hiding this comment.
I am not convinced that this is anything but a bug.
src/compiler/transformers/esnext.ts
Outdated
| @@ -302,9 +303,9 @@ export function transformESNext(context: TransformationContext): (x: SourceFile | |||
| // before handing the shallow transformation back to the visitor for an in-depth transformation. | |||
| const forInitializer = node.initializer; | |||
| Debug.assertNode(forInitializer, isUsingVariableDeclarationList); | |||
| Debug.assert(forInitializer.declarations.length === 1, "ForInitializer may only have one declaration"); | |||
| Debug.assert(forInitializer.declarations.length <= 1, "ForInitializer may only have one declaration"); | |||
There was a problem hiding this comment.
That was a crash on those broken empty declaration lists produced by the added test cases.
Other broken inputs are somewhat handled by transformers. For example:
// input
for (const x of)
// es5 output
for (var _i = 0, _1 = ; _i < _1.length; _i++) {
var x = _1[_i];
;
}It's not that this particular output is problem-free... it's broken but at least the transform/emit doesn't crash.
src/compiler/transformers/esnext.ts
Outdated
|
|
||
| const forDecl = forInitializer.declarations[0]; | ||
| const forDecl = firstOrUndefined(forInitializer.declarations) || factory.createVariableDeclaration(factory.createUniqueName("x")); |
There was a problem hiding this comment.
I also wonder if this whole case shouldn't be a parse error and if it shouldn't be partially handled by createMissingNode and/or similar mechanisms
There was a problem hiding this comment.
Probably? If you throw stuff into https://jakebailey.dev/esbuild-playground/ or something do your cases error more?
There was a problem hiding this comment.
The errors are now roughly equivalent after my change. However, IIUC, esbuild and Babel treat this as a syntax error - whereas TS currently treats this as a grammar error (in TS we end up with sourceFile.parseDiagnostics.length === 0). The same is true about some other existing cases like:
for (var of of) { }Note that my understanding of syntax/grammar errors is somewhat superficial so maybe I'm just misinterpreting something here.
There was a problem hiding this comment.
Yeah, we sorta treat grammar errors as just "more parser errors", though I would personally prefer if stopped doing these kinds of things in the checker.
There was a problem hiding this comment.
But, yeah, is there a fix which instead makes a missing node instead? That feels better given the debug assert above.
There was a problem hiding this comment.
But, yeah, is there a fix which instead makes a missing node instead?
Possibly. I only learned about createMissingNode after I opened this PR - so I would have to explore that route and recheck how it's used in the codebase and if that would be a good fit here.
There was a problem hiding this comment.
I rechecked this and this .declarations is already isMissingList. That's a concept only really known to the parser though right now - I could move a util for checking that to some shared module but I don't think it's worth it.
It just seems to me that this transform here has been written with extra invariants that are not present in some other very similar situations:
// @target: es5
for (var of a) { }This is also a for-of loop but the es2015 transform doesn't require this to have exactly one declaration, see here. It just gracefully grabs the first one or creates a temp variable (so basically what I'm doing here).
I think it would just be best to follow the es2015 transform here, I'll adjust the code to use the same way of creating a temp variable and gonna remove this invariant entirely.
…-declarations-in-for-of
* upstream/main: (37 commits) Added NoTruncation flag to completions (microsoft#58719) Clone node to remove location even when it has been modified if needed (microsoft#58706) Properly account for `this` argument in intersection apparent type caching (microsoft#58677) Fix: Include Values of Script Extensions for Unicode Property Value Expressions in Regular Expressions (microsoft#58615) In `reScanSlashToken` use `charCodeChecked` not `codePointChecked` (microsoft#58727) Shorten error spans for errors reported on constructor declarations (microsoft#58061) Mark file as skips typechecking if it contains ts-nocheck (microsoft#58593) Fixed an issue with broken `await using` declarations in `for of` loops (microsoft#56466) Do not expand type references in keyof and index access (microsoft#58715) Improve the performance of isolatedDeclarations quickfix (microsoft#58722) Unwrap `NoInfer` types when narrowing (microsoft#58292) Recover from type reuse errors by falling back to inferred type printing (microsoft#58720) Fixing self import (microsoft#58718) Enable JS emit for noCheck and noCheck for transpileModule (microsoft#58364) Revert PR 55371 (microsoft#58702) Update dependencies (microsoft#58639) Fix baselines after PR 58621 (microsoft#58705) Do not infer `yield*` type from contextual `TReturn` (microsoft#58621) `await using` normative changes (microsoft#58624) Handling statements from a known source file (microsoft#58679) ...
fixes what was reported by @evanw here: #55558 (comment)
cc @rbuckton