Skip to content

Port "Error on too many parameters for iterator method" #1059

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.

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 2 commits into from
Jun 6, 2025

Conversation

Andarist
Copy link
Contributor

@Andarist Andarist commented Jun 5, 2025

@Copilot Copilot AI review requested due to automatic review settings June 5, 2025 05:26
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR ports the error reporting for too many parameters in iterator methods to a new error code, updating error messages in both synchronous and asynchronous iterator tests while enhancing the iteration type resolution in checker.go.

  • Update error messages in iterator and async iterator error baselines
  • Introduce new checked iterable type resolvers and adjust signature filtering in the checker
  • Update baseline files to reflect the new error codes and messages

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file
File Description
testdata/baselines/reference/submodule/compiler/iteratorExtraParameters.errors.txt.diff Updated error messages from TS2488 to TS2322 for iterator extra parameters
testdata/baselines/reference/submodule/compiler/iteratorExtraParameters.errors.txt Updated baseline error messages for iterator methods
testdata/baselines/reference/submodule/compiler/asyncIteratorExtraParameters.errors.txt.diff Updated error messages from TS2504 to TS2322 for async iterator extra parameters
testdata/baselines/reference/submodule/compiler/asyncIteratorExtraParameters.errors.txt Updated baseline error messages for async iterator methods
internal/checker/checker.go Added new checked iterable type resolvers and refined signature filtering logic
Comments suppressed due to low confidence (1)

internal/checker/checker.go:6055

  • [nitpick] Consider adding a comment to explain why signatures with a minimum argument count of 0 are filtered as valid. This can help maintain the clarity of the iteration type resolution logic for future maintainers.
allSignatures := c.getSignaturesOfType(methodType, SignatureKindCall)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is still different for 2 reasons:

  • diagnostics/error output containers don't support skipLogging (yet?)
  • there is no logic to ignore cached values for IterationTypes{} (aka noIterationTypes) and thus repeated errors are not reported. I'll fix this in a followup PR

Copy link
Member

Choose a reason for hiding this comment

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

For the first thing, I'm pretty sure this is a place where we're differing because the code wasn't ported but redone from scratch. So I don't think that's a simple fix.

Copy link
Member

Choose a reason for hiding this comment

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

I think the PR needs a rebase after #1061, yes?

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

validSignatures := core.Filter(allSignatures, func(sig *Signature) bool {
return c.getMinArgumentCount(sig) == 0
})
if len(validSignatures) != 0 {
Copy link
Member

Choose a reason for hiding this comment

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

It feels suspicious that this condition is flipped from the original code...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's just because the actual branches here are flipped, this allowed me to nearly fall through to the IterationTypes{} case in the case when valid signatures are missing

Copy link
Member

@jakebailey jakebailey left a comment

Choose a reason for hiding this comment

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

Seems correct modulo the larger errors.

@jakebailey jakebailey added this pull request to the merge queue Jun 6, 2025
Merged via the queue into microsoft:main with commit 22f2c74 Jun 6, 2025
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants