Skip to content

Check for unused pyright: ignore and differentiate from mypy ignores - #9397

Merged
AlexWaygood merged 8 commits into
python:mainfrom
Avasam:pyright-no-unused-pyright-ignore-comments
Dec 28, 2022
Merged

Check for unused pyright: ignore and differentiate from mypy ignores#9397
AlexWaygood merged 8 commits into
python:mainfrom
Avasam:pyright-no-unused-pyright-ignore-comments

Conversation

@Avasam

@AvasamAvasam commented Dec 21, 2022•
edited
Loading

Copy link
Copy Markdown
Collaborator

This is possible since pyright 1.1.283. I had to bump from 1.1.284 because of a regression with comments.
Prevent type: ignore[*] comments from also blanket suppressing all pyright errors.
Added more comments to pyright config.
Fixed new pyright errors

@Avasam
Check for unused pyright: ignore
af804b6
Prevent type: ignore[*] comments from supressing all pyright errors
Added more comments to pyright config
@Avasam
Unused variable
a2e8a20
@AvasamAvasam changed the title Check for unused pyright: ignore and differentiate from mypy ignoresCheck for unused pyright: ignore and differentiate from mypy ignoresDec 21, 2022
JelleZijlstra
JelleZijlstra reviewed Dec 21, 2022
View reviewed changes
Comment threadstdlib/builtins.pyi
@@ -1188,7 +1188,7 @@ class property:
class _NotImplementedType(Any): # type: ignore[misc]
# A little weird, but typing the __call__ as NotImplemented makes the error message

@JelleZijlstraJelleZijlstraDec 21, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Unrelated to your PR but we should probably revisit this and see if it actually makes things better for all type checkers.

@github-actions

This comment has been minimized.

Sign in to view
AlexWaygood
AlexWaygood reviewed Dec 22, 2022
View reviewed changes

@AlexWaygoodAlexWaygood left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks great to me. You've uncovered several cases where it looks like we were accidentally ignoring several pyright errors that we didn't really want to, and I think that shows the value of the change.

I left a few comments below. As well as those: maybe we should add a note to CONTRIBUTING.md somewhere. It might be surprising for new contributors to find out that type: ignore comments don't silence errors emitted by pyright. It might go well in the "Conventions" section, where we already talk about using mypy error codes for type: ignore comments wherever possible.

Comment threadstdlib/collections/__init__.pyi
# but they are not exposed anywhere)
@final
class _odict_keys(dict_keys[_KT_co, _VT_co], Reversible[_KT_co]): # type: ignore[misc]
class _odict_keys(dict_keys[_KT_co, _VT_co], Reversible[_KT_co]): # type: ignore[misc] # pyright: ignore

@AlexWaygoodAlexWaygoodDec 22, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Are we able to add the pyright error codes for these pyright: ignores?

@AvasamAvasamDec 22, 2022•
edited
Loading

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

There is no code!
image

Base class "dict_keys[_KT_co@_odict_keys, _VT_co@_odict_keys]" is marked final and cannot be subclassed Pylance

image

E:\Users\Avasam\Documents\Git\typeshed\stdlib\collections_init_.pyi
E:\Users\Avasam\Documents\Git\typeshed\stdlib\collections_init_.pyi:331:19 - error: Base class "dict_keys[_KT_co@_odict_keys, _VT_co@_odict_keys]" is marked final and cannot be subclassed

@AlexWaygoodAlexWaygoodDec 22, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Oh, strange! In that case, can we leave a comment to that effect? (Same below for the other one)

Suggested change
class_odict_keys(dict_keys[_KT_co, _VT_co], Reversible[_KT_co]): # type: ignore[misc] # pyright: ignore
# pyright doesn't have a specific error code for this error!
class_odict_keys(dict_keys[_KT_co, _VT_co], Reversible[_KT_co]): # type: ignore[misc] # pyright: ignore

Comment threadstubs/redis/redis/client.pyi Outdated
Comment threadstubs/pywin32/win32comext/axdebug/documents.pyi

# error: Cannot determine consistent method resolution order (MRO) for "DebugDocumentText"
class DebugDocumentText(gateways.DebugDocumentInfo, gateways.DebugDocumentText, gateways.DebugDocument): # type: ignore[misc]
class DebugDocumentText(gateways.DebugDocumentInfo, gateways.DebugDocumentText, gateways.DebugDocument): # type: ignore[misc] # pyright: ignore

@AlexWaygoodAlexWaygoodDec 22, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Which pyright error are we ignoring here? Can we add the error code?

@AvasamAvasamDec 22, 2022•
edited
Loading

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Same as above, no code, and if iirc, this does fail at runtime. I had already added a comment to explain the ignore.

Cannot create consistent method ordering Pylance

I could create a feature request with pyright to add a code for "will fail at runtime". But this should rather be fixed upstream (and pywin32 already has accepted a couple fixes I found through type-checking).

And for the other case above, it's kindof a niche type-stub use-case, and is totally fixable by splitting off into another private/internal protocol (which I didn't do to keep changes minimal)

@Avasam@AlexWaygood
Update stubs/redis/redis/client.pyi
70ee787
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

github-actionsBot commented Dec 27, 2022

Copy link
Copy Markdown
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

@Avasam

Avasam commented Dec 27, 2022

Copy link
Copy Markdown
CollaboratorAuthor

f1264a1 (#9397) / https://github.com/python/typeshed/actions/runs/3789522708 Weird stubtest edge-case when ran under Python 3.7 . At least it was an easy fix.

AlexWaygood
AlexWaygood approved these changes Dec 28, 2022
View reviewed changes

@AlexWaygoodAlexWaygood left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Great stuff, thank you!

@AlexWaygood
AlexWaygood merged commit 23ac9bf into python:mainDec 28, 2022
@Avasam
Avasam deleted the pyright-no-unused-pyright-ignore-comments branch December 28, 2022 18:33
Avasam added a commit to Avasam/typeshed that referenced this pull request Jan 30, 2023
@Avasam
Remove redundant reportUnnecessaryTypeIgnoreComment=true sincepython…
da92508
…#9397
Avasam added a commit to Avasam/typeshed that referenced this pull request Jan 30, 2023
@Avasam
Remove redundant reportUnnecessaryTypeIgnoreComment=true sincepython…
138d69e
…#9397
@AvasamAvasam mentioned this pull request Jan 30, 2023
Closed
@AvasamAvasam mentioned this pull request Feb 21, 2023
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JelleZijlstraJelleZijlstraJelleZijlstra left review comments
@AlexWaygoodAlexWaygoodAlexWaygood approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Avasam@JelleZijlstra@AlexWaygood