-
Notifications
You must be signed in to change notification settings - Fork 292
Allow type checkers to ignore specific error codes #2153
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
Open
davidhalter
wants to merge
10
commits into
python:main
Choose a base branch
from
davidhalter:zuban-ignore
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
f4a919a
Allow type: ignore error codes filtering
davidhalter 6ce8087
Try to improve the wording for the spec
davidhalter 5cbcac4
Rephrase a bit of the spec
davidhalter 6856262
Merge remote-tracking branch 'origin/main' into zuban-ignore
davidhalter ffc8e7c
Avoid specifying how whitespace after # type: ignore works
davidhalter a0bc8ad
Update conformance/tests/directives_type_ignore.py
davidhalter 614bf60
Update docs/spec/directives.rst
davidhalter ca781d2
Rerun the tests
davidhalter d37d815
Change the error code to a random error code and allow the error to h…
davidhalter a6b55e4
Update conformance/tests/directives_type_ignore.py
davidhalter File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,11 +1,6 @@ | ||
| conformance_automated = "Fail" | ||
| conformant = "Partial" | ||
| notes = """ | ||
| Treats `# type: ignore[error-code]` as only ignoring errors that match the error code `error-code`. | ||
| """ | ||
| conformance_automated = "Pass" | ||
| errors_diff = """ | ||
| Line 14: Unexpected errors ['directives_type_ignore.py:14:10: error[invalid-assignment] Object of type `Literal[""]` is not assignable to `int`'] | ||
| """ | ||
| output = """ | ||
| directives_type_ignore.py:14:10: error[invalid-assignment] Object of type `Literal[""]` is not assignable to `int` | ||
| directives_type_ignore.py:16:10: error[invalid-assignment] Object of type `Literal[""]` is not assignable to `int` | ||
| """ |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,12 +1,7 @@ | ||
| conformant = "Partial" | ||
| notes = """ | ||
| Does not honor "# type: ignore" comment if comment includes additional text. | ||
| """ | ||
| conformance_automated = "Fail" | ||
| conformance_automated = "Pass" | ||
| errors_diff = """ | ||
| Line 14: Unexpected errors ['directives_type_ignore.py:14: error: Incompatible types in assignment (expression has type "str", variable has type "int") [assignment]'] | ||
| """ | ||
| output = """ | ||
| directives_type_ignore.py:14: error: Incompatible types in assignment (expression has type "str", variable has type "int") [assignment] | ||
| directives_type_ignore.py:14: note: Error code "assignment" not covered by "type: ignore" comment | ||
| directives_type_ignore.py:16: error: Incompatible types in assignment (expression has type "str", variable has type "int") [assignment] | ||
| directives_type_ignore.py:16: note: Error code "assignment" not covered by "type: ignore" comment | ||
| """ |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about also documenting that multiple error codes can be given (comma-separated), and unrecognized error codes should be ignored, if a type checker supports ignoring errors based on specific error code, as @ilevkivskyi suggested here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there any other opinions on this? I'm pretty open to changes here.
I personally think that the comma separated form doesn't need to be specified explicitly.
Does this mean that Mypy would need to ignore the error code "undefined"? Currently it adds errors at least when using
--warn-unused-ignores: https://mypy-play.net/?mypy=latest&python=3.12&flags=warn-unused-ignores&gist=27ebc9511f193a9f348b0235412cbcc9There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally think that the approach outlined by @ilevkivskyi is the right one for multi-type-checker interoperability (much better than requiring totally separate ignore comments for each type checker), and I would be happy to fully specify it. But that would be a specification change that puts pyright and pyrefly out of conformance. If we did that, then I think the fulI behavior, including multiple comma separated codes, should be specified. I guess maybe we need feedback at least from pyrefly here: is the "always blanket ignore" behavior pyrefly's UX preference, or just something that was done to satisfy the previous conformance suite requirements?
But I think as long as the specification here is looser, as currently written (to accommodate both the pyright/pyrefly behavior and the mypy/zuban/ty behavior), it's not really important to document the multiple-error-codes handling, since effectively all this spec is now saying is that type checkers can do whatever they want with the information inside the brackets.
(Either way, I don't think the spec should _require_that type checkers be silent about unknown codes; it should be OK to warn users about them, as mypy does with
--warn-unused-ignores. This is better behavior for users who aren't using multiple type checkers. Ideally users in multi-type-checker scenarios can turn off that warning if they don't want it.)I'm happy with the current state of this PR as a first step, at least.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with the current wording in this PR.
As for @ilevkivskyi's suggestion - I personally like it, but I'd like to run this by some other pyrefly devs who have thought a lot more than I have about configuration and adoption, to make sure this isn't incompatible with our tooling in some way that I'm missing.