Report iteration errors on all error nodes

[Andarist]

No description provided.

[Andarist]

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

[jakebailey]

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.

[Andarist]

done

[jakebailey]

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

[jakebailey]

Both options seem bad, kinda...

[jakebailey]

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

[Andarist]

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

[jakebailey]

Okay, then I guess go back, sorry 😄

[Andarist]

I pushed out a revert going back to the noCache variant

[Andarist]

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?

[jakebailey]

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.

[Copilot]

Pull Request Overview

This PR enhances error reporting for iteration constructs by ensuring errors are reported on all relevant nodes rather than only the first occurrence.

• Adjusts the caching logic in getIterationTypesOfIterable to bypass cache when reporting errors.
• Removes the IterationUseReportError flag and replaces it with a noCache mechanism.
• Updates baseline snapshots in test data to include the additional expected error lines.

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

File	Description
internal/checker/checker.go	Modified iteration-type caching and error-report logic
testdata/baselines/reference/submodule/conformance/types.forAwait.es2018.2.errors.txt(.diff)	Updated expected errors to include additional iteration errors
testdata/baselines/reference/submodule/conformance/for-of16.errors.txt(.diff)	Updated expected errors to include additional iteration errors
testdata/baselines/reference/submodule/compiler/omittedExpressionForOfLoop.errors.txt(.diff)	Updated expected errors to include additional iteration errors

Comments suppressed due to low confidence (4)

internal/checker/checker.go:5884

• [nitpick] Consider adding a comment above this cache key definition to explain why the errorNode flag was removed from the key and how the noCache logic affects caching behavior, improving code clarity.

key := IterationTypesKey{typeId: t.id, use: use & IterationUseCacheFlags}

internal/checker/checker.go:5887

• [nitpick] The variable name noCache is ambiguous; consider renaming it to skipCache or disableCache to more clearly convey its purpose.

noCache := false

internal/checker/checker.go:5901

• [nitpick] The new noCache parameter in this function signature should be documented in a comment or the method docstring to explain when it is used and its effect on caching.

func (c *Checker) getIterationTypesOfIterableWorker(t *Type, use IterationUse, errorNode *ast.Node, noCache bool) IterationTypes {

internal/checker/checker.go:5882

• Consider adding unit tests for the scenario where an errorNode is provided and the cached iteration types have no types, to ensure the no-cache path correctly reports all errors on multiple nodes.

func (c *Checker) getIterationTypesOfIterable(t *Type, use IterationUse, errorNode *ast.Node) IterationTypes {