Fix error propagation through non-iterable unions#3496
Fix error propagation through non-iterable unions#3496Andarist wants to merge 1 commit intomicrosoft:mainfrom
Conversation
| @@ -6093,7 +6093,20 @@ func (c *Checker) getIterationTypesOfIterable(t *Type, use IterationUse, errorNo | |||
|
|
|||
| func (c *Checker) getIterationTypesOfIterableWorker(t *Type, use IterationUse, errorNode *ast.Node, noCache bool) IterationTypes { | |||
| if t.flags&TypeFlagsUnion != 0 { | |||
There was a problem hiding this comment.
Note that Corsa pushed caching entirely to getIterationTypesOfIterable - so the per-constituent results here are not getting cached. That's a preexisting divergence from Strada.
There was a problem hiding this comment.
Pull request overview
Fixes tsgo/tsc parity for error recovery when iterating over union types that are not fully iterable, preventing downstream type errors by recovering the iteration element type to any when any union constituent is non-iterable (issue #3487).
Changes:
- Update iteration-type computation for union types to fail as a whole (and report a single TS2488) when any constituent isn’t iterable.
- Add a new compiler test covering for-of/destructuring/indexing over a non-iterable union and update/add corresponding baselines.
- Update several submodule baseline expectations to reflect unified union-level TS2488 reporting and
anyrecovery.
Show a summary per file
| File | Description |
|---|---|
| internal/checker/checker.go | Changes union iteration-type handling to report a single union-level non-iterable diagnostic and recover iteration types accordingly. |
| testdata/tests/cases/compiler/iterationErrorOverNotIterableUnions1.ts | New repro-style compiler test for non-iterable union iteration/destructuring/indexing behavior. |
| testdata/baselines/reference/compiler/iterationErrorOverNotIterableUnions1.types | New baseline for types output from the added compiler test. |
| testdata/baselines/reference/compiler/iterationErrorOverNotIterableUnions1.symbols | New baseline for symbols output from the added compiler test. |
| testdata/baselines/reference/compiler/iterationErrorOverNotIterableUnions1.js | New baseline for JS emit output from the added compiler test. |
| testdata/baselines/reference/compiler/iterationErrorOverNotIterableUnions1.errors.txt | New baseline for diagnostics output from the added compiler test. |
| testdata/baselines/reference/submodule/conformance/destructuringArrayBindingPatternAndAssignment4(target=es2015).types.diff | Removes now-unneeded diff baseline due to updated parity. |
| testdata/baselines/reference/submodule/conformance/destructuringArrayBindingPatternAndAssignment4(target=es2015).types | Updates expected recovery type for destructuring from invalid iterable union. |
| testdata/baselines/reference/submodule/conformance/destructuringArrayBindingPatternAndAssignment4(target=es2015).errors.txt.diff | Removes now-unneeded diff baseline due to updated parity. |
| testdata/baselines/reference/submodule/conformance/destructuringArrayBindingPatternAndAssignment4(target=es2015).errors.txt | Updates expected TS2488 text to be union-level. |
| testdata/baselines/reference/submodule/conformance/ES5For-ofTypeCheck9(target=es2015).types.diff | Removes now-unneeded diff baseline due to updated parity. |
| testdata/baselines/reference/submodule/conformance/ES5For-ofTypeCheck9(target=es2015).types | Updates expected recovery type in invalid for-of over union. |
| testdata/baselines/reference/submodule/conformance/ES5For-ofTypeCheck9(target=es2015).errors.txt.diff | Removes now-unneeded diff baseline due to updated parity. |
| testdata/baselines/reference/submodule/conformance/ES5For-ofTypeCheck9(target=es2015).errors.txt | Updates expected TS2488 to be union-level instead of per-constituent. |
| testdata/baselines/reference/submodule/conformance/ES5For-ofTypeCheck7(target=es2015).types.diff | Removes now-unneeded diff baseline due to updated parity. |
| testdata/baselines/reference/submodule/conformance/ES5For-ofTypeCheck7(target=es2015).types | Updates expected recovery type in invalid for-of over union. |
| testdata/baselines/reference/submodule/conformance/ES5For-ofTypeCheck7(target=es2015).errors.txt.diff | Removes now-unneeded diff baseline due to updated parity. |
| testdata/baselines/reference/submodule/conformance/ES5For-ofTypeCheck7(target=es2015).errors.txt | Updates expected TS2488 to be union-level instead of per-constituent. |
| testdata/baselines/reference/submodule/conformance/ES5For-of30(target=es2015).errors.txt.diff | Removes now-unneeded diff baseline due to updated parity. |
| testdata/baselines/reference/submodule/conformance/ES5For-of30(target=es2015).errors.txt | Updates expected TS2488 text to be union-level. |
| testdata/baselines/reference/submodule/compiler/noImplicitAnyLoopCrash(target=es2015).errors.txt.diff | Removes now-unneeded diff baseline due to updated parity. |
| testdata/baselines/reference/submodule/compiler/noImplicitAnyLoopCrash(target=es2015).errors.txt | Updates expected TS2488 text to be union-level. |
Copilot's findings
- Files reviewed: 22/22 changed files
- Comments generated: 2
| allIterationTypes := make([]IterationTypes, 0, len(t.Types())) | ||
| for _, constituent := range t.Types() { | ||
| iterationTypes := c.getIterationTypesOfIterableWorker(constituent, use, nil, noCache) | ||
| if !iterationTypes.hasTypes() { | ||
| if errorNode != nil { | ||
| c.addDeferredDiagnostic(func() { | ||
| c.reportTypeNotIterableError(errorNode, t, use&IterationUseAllowsAsyncIterablesFlag != 0) | ||
| }) | ||
| } | ||
| return IterationTypes{} |
There was a problem hiding this comment.
The union-handling change here is meant to fix cases where the primary TS2488 diagnostic is suppressed (e.g. // @ts-ignore on a for...of/destructuring/spread), but downstream errors should still not be produced because the iterated element should recover to any. The added test exercises the non-suppressed case, but doesn’t cover the suppressed-diagnostic scenario from issue #3487; please add a minimal test with // @ts-ignore on the for...of line and assert there are no downstream errors in the loop body (and ideally no errors at all for that statement).
| for (const ignoredItem of data) { | ||
| ignoredItem.b; |
There was a problem hiding this comment.
The identifier ignoredItem reads as if this loop is using // @ts-ignore, but there is no ignore comment here. Consider either adding the intended // @ts-ignore to match the bug report scenario, or renaming the variable so the test intent is clearer.
| for (const ignoredItem of data) { | |
| ignoredItem.b; | |
| for (const otherItem of data) { | |
| otherItem.b; |
fixes #3487