Skip to content

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

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

Conversation

Avasam
Copy link
Collaborator

@Avasam Avasam commented Dec 21, 2022

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

Prevent type: ignore[*] comments from supressing all pyright errors
Added more comments to pyright config
@Avasam Avasam changed the title Check for unused pyright: ignore and differentiate from mypy ignores Check for unused pyright: ignore and differentiate from mypy ignores Dec 21, 2022
@@ -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
Copy link
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.

Copy link
Member

@AlexWaygood AlexWaygood left a comment

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.

@@ -328,15 +328,15 @@ class _OrderedDictValuesView(ValuesView[_VT_co], Reversible[_VT_co]):
# (At runtime, these are called `odict_keys`, `odict_items` and `odict_values`,
# 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
Copy link
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?

Copy link
Collaborator Author

@Avasam Avasam Dec 22, 2022

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

Copy link
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

@@ -14,7 +14,7 @@ class DebugDocumentProvider(gateways.DebugDocumentProvider):
def GetDocument(self): ...

# 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
Copy link
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?

Copy link
Collaborator Author

@Avasam Avasam Dec 22, 2022

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)

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@Avasam Avasam requested a review from AlexWaygood December 27, 2022 19:21
@github-actions

This comment has been minimized.

@github-actions
Copy link
Contributor

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

@Avasam
Copy link
Collaborator Author

Avasam commented Dec 27, 2022

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.

Copy link
Member

@AlexWaygood AlexWaygood left a comment

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:main Dec 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 added a commit to Avasam/typeshed that referenced this pull request Jan 30, 2023
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.

3 participants