Skip to content

Report iteration errors on all error nodes #1061

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Jun 5, 2025
Merged

Report iteration errors on all error nodes #1061

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8272017
Report iteration errors on all error nodes
Andarist Jun 5, 2025
c9f7b9e
fmt
Andarist Jun 5, 2025
a5a4449
include `nodeId` in the cache key insead
Andarist Jun 5, 2025
4bb5800
Revert "include `nodeId` in the cache key insead"
Andarist Jun 5, 2025
1dfc0da
Merge branch 'main' into fix/repeat-iteration-errors
Andarist Jun 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 15 additions & 8 deletions internal/checker/checker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -470,7 +470,6 @@ const (
IterationUseSpreadFlag IterationUse = 1 << 5
IterationUseDestructuringFlag IterationUse = 1 << 6
IterationUsePossiblyOutOfBounds IterationUse = 1 << 7
IterationUseReportError IterationUse = 1 << 8
// Spread, Destructuring, Array element assignment
IterationUseElement = IterationUseAllowsSyncIterablesFlag
IterationUseSpread = IterationUseAllowsSyncIterablesFlag | IterationUseSpreadFlag
Expand All @@ -481,7 +480,7 @@ const (
IterationUseAsyncYieldStar = IterationUseAllowsSyncIterablesFlag | IterationUseAllowsAsyncIterablesFlag | IterationUseYieldStarFlag
IterationUseGeneratorReturnType = IterationUseAllowsSyncIterablesFlag
IterationUseAsyncGeneratorReturnType = IterationUseAllowsAsyncIterablesFlag
IterationUseCacheFlags = IterationUseAllowsSyncIterablesFlag | IterationUseAllowsAsyncIterablesFlag | IterationUseForOfFlag | IterationUseReportError
IterationUseCacheFlags = IterationUseAllowsSyncIterablesFlag | IterationUseAllowsAsyncIterablesFlag | IterationUseForOfFlag
)

type IterationTypes struct {
Expand Down Expand Up @@ -5882,18 +5881,26 @@ func (c *Checker) getIterationTypesOfIterable(t *Type, use IterationUse, errorNo
if IsTypeAny(t) {
return IterationTypes{c.anyType, c.anyType, c.anyType}
}
key := IterationTypesKey{typeId: t.id, use: use&IterationUseCacheFlags | core.IfElse(errorNode != nil, IterationUseReportError, 0)}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was this even concurrent-checking safe? wouldn't we, potentially, get a different set of errors with multiple error nodes given each concurrent checker could report the error for a different firt encountered errorNode?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, yes, I agree, though I believe it's handled because we spawn a specific number of checkers and then round robin them, and then they all walk their files in order, so it's always consistent.

But, I think we should probably change it, since this affects the editor and would change if we allow custom checker counts.

key := IterationTypesKey{typeId: t.id, use: use&IterationUseCacheFlags}
// If we are reporting errors and encounter a cached `noIterationTypes`, we should ignore the cached value and continue as if nothing was cached.
// In addition, we should not cache any new results for this call.
noCache := false
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alternatively, I could use nodeId as part of the IterationTypesKey to get the same results

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a feeling that we might want to do that, just because we can hit this path through regular checker methods externally and then repeatedly tack more diagnostics onto the list.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, won't this be like one entry for every single loop, yield, etc, that we check?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both options seem bad, kinda...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I guess the way you had it originally was more similar to the original code, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, it was more similar - and it only was re-retrieving the iteration types when the cached value had no types. So it seems it should be faster than including the nodeId in the cache key

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, then I guess go back, sorry 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed out a revert going back to the noCache variant

if cached, ok := c.iterationTypesCache[key]; ok {
return cached
if errorNode == nil || cached.hasTypes() {
return cached
}
noCache = true
}
result := c.getIterationTypesOfIterableWorker(t, use, errorNode, noCache)
if !noCache {
c.iterationTypesCache[key] = result
}
result := c.getIterationTypesOfIterableWorker(t, use, errorNode)
c.iterationTypesCache[key] = result
return result
}

func (c *Checker) getIterationTypesOfIterableWorker(t *Type, use IterationUse, errorNode *ast.Node) IterationTypes {
func (c *Checker) getIterationTypesOfIterableWorker(t *Type, use IterationUse, errorNode *ast.Node, noCache bool) IterationTypes {
if t.flags&TypeFlagsUnion != 0 {
return c.combineIterationTypes(core.Map(t.Types(), func(t *Type) IterationTypes { return c.getIterationTypesOfIterableWorker(t, use, errorNode) }))
return c.combineIterationTypes(core.Map(t.Types(), func(t *Type) IterationTypes { return c.getIterationTypesOfIterableWorker(t, use, errorNode, noCache) }))
}
if use&IterationUseAllowsAsyncIterablesFlag != 0 {
iterationTypes := c.getIterationTypesOfIterableFast(t, c.asyncIterationTypesResolver)
Expand Down
5 changes: 4 additions & 1 deletion testdata/baselines/reference/submodule/compiler/omittedExpressionForOfLoop.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
omittedExpressionForOfLoop.ts(1,19): error TS2304: Cannot find name 'doesNotExist'.
omittedExpressionForOfLoop.ts(4,19): error TS18050: The value 'undefined' cannot be used here.
omittedExpressionForOfLoop.ts(7,12): error TS2488: Type 'never' must have a '[Symbol.iterator]()' method that returns an iterator.
omittedExpressionForOfLoop.ts(10,12): error TS2488: Type 'never' must have a '[Symbol.iterator]()' method that returns an iterator.


==== omittedExpressionForOfLoop.ts (3 errors) ====
==== omittedExpressionForOfLoop.ts (4 errors) ====
for (const [,] of doesNotExist) {
~~~~~~~~~~~~
!!! error TS2304: Cannot find name 'doesNotExist'.
Expand All @@ -20,4 +21,6 @@ omittedExpressionForOfLoop.ts(7,12): error TS2488: Type 'never' must have a '[Sy
}

for (const [] of []) {
~~
!!! error TS2488: Type 'never' must have a '[Symbol.iterator]()' method that returns an iterator.
}
23 changes: 0 additions & 23 deletions testdata/baselines/reference/submodule/compiler/omittedExpressionForOfLoop.errors.txt.diff
View file
Open in desktop

This file was deleted.

8 changes: 6 additions & 2 deletions testdata/baselines/reference/submodule/conformance/for-of16.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
for-of16.ts(8,11): error TS2488: Type 'MyStringIterator' must have a '[Symbol.iterator]()' method that returns an iterator.
for-of16.ts(10,11): error TS2488: Type 'MyStringIterator' must have a '[Symbol.iterator]()' method that returns an iterator.


==== for-of16.ts (1 errors) ====
==== for-of16.ts (2 errors) ====
class MyStringIterator {
[Symbol.iterator]() {
return this;
Expand All @@ -14,4 +15,7 @@ for-of16.ts(8,11): error TS2488: Type 'MyStringIterator' must have a '[Symbol.it
!!! error TS2488: Type 'MyStringIterator' must have a '[Symbol.iterator]()' method that returns an iterator.
!!! related TS2489 for-of16.ts:8:11: An iterator must have a 'next()' method.

for (v of new MyStringIterator) { } // Should still fail (related errors should still be shown even though type is cached).
for (v of new MyStringIterator) { } // Should still fail (related errors should still be shown even though type is cached).
~~~~~~~~~~~~~~~~~~~~
!!! error TS2488: Type 'MyStringIterator' must have a '[Symbol.iterator]()' method that returns an iterator.
!!! related TS2489 for-of16.ts:10:11: An iterator must have a 'next()' method.
21 changes: 0 additions & 21 deletions testdata/baselines/reference/submodule/conformance/for-of16.errors.txt.diff
View file
Open in desktop

This file was deleted.

6 changes: 5 additions & 1 deletion testdata/baselines/reference/submodule/conformance/types.forAwait.es2018.2.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,10 @@ types.forAwait.es2018.2.ts(8,21): error TS2504: Type '{}' must have a '[Symbol.a
types.forAwait.es2018.2.ts(10,16): error TS2322: Type 'number' is not assignable to type 'string'.
types.forAwait.es2018.2.ts(12,16): error TS2322: Type 'number' is not assignable to type 'string'.
types.forAwait.es2018.2.ts(14,21): error TS2488: Type 'AsyncIterable<number>' must have a '[Symbol.iterator]()' method that returns an iterator.
types.forAwait.es2018.2.ts(16,15): error TS2488: Type 'AsyncIterable<number>' must have a '[Symbol.iterator]()' method that returns an iterator.


==== types.forAwait.es2018.2.ts (5 errors) ====
==== types.forAwait.es2018.2.ts (6 errors) ====
declare const asyncIterable: AsyncIterable<number>;
declare const iterable: Iterable<number>;
async function f() {
Expand Down Expand Up @@ -33,6 +34,9 @@ types.forAwait.es2018.2.ts(14,21): error TS2488: Type 'AsyncIterable<number>' mu
!!! related TS2773 types.forAwait.es2018.2.ts:14:21: Did you forget to use 'await'?
}
for (y of asyncIterable) {
~~~~~~~~~~~~~
!!! error TS2488: Type 'AsyncIterable<number>' must have a '[Symbol.iterator]()' method that returns an iterator.
!!! related TS2773 types.forAwait.es2018.2.ts:16:15: Did you forget to use 'await'?
}
}

26 changes: 0 additions & 26 deletions testdata/baselines/reference/submodule/conformance/types.forAwait.es2018.2.errors.txt.diff
View file
Open in desktop

This file was deleted.

Loading