Skip to content

Enable pyright for all Python 3 stubs #5597

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 7 commits into from
Jun 9, 2021
Merged

Conversation

srittau
Copy link
Collaborator

@srittau srittau commented Jun 9, 2021

Disable rules reportUnknownParameterType, reportUnknownVariableType, and reportUnknownMemberType.

Unknown parameter types are preferred over Any annotations for
incomplete stubs. Especially larger stubs are expected to be
incomplete for some time and it would be a shame to lose the
other pyright warnings for those stubs.

Cc @jakebailey

srittau added 2 commits June 9, 2021 10:03
Unknown parameter types are preferred over Any annotations for
incomplete stubs. Especially larger stubs are expected to be
incomplete for some time and it would be a shame to lose the
other pyright warnings for those stubs.
Fix problems with tkinter
@github-actions

This comment has been minimized.

@srittau
Copy link
Collaborator Author

srittau commented Jun 9, 2021

I'll have a huge patch incoming that fixes all problems in the currently disabled stubs.

Fix pyright problems
@srittau srittau changed the title pyright: disable reportUnknownParameterType Enable pyright for all Python 3 stubs Jun 9, 2021
@@ -2,7 +2,11 @@ import sys
from _typeshed import ReadableBuffer
from typing import Callable, Optional, Union

from cryptography.hazmat.primitives.asymmetric.ec2 import EllipticCurve, EllipticCurvePrivateKey, EllipticCurvePublicKey
from cryptography.hazmat.primitives.asymmetric.ec2 import ( # type: ignore
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why does this need # type: ignore?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because pyright doesn't find this import (probably because it's "hidden" in stubs/cryptography). This is something we should look into, but I ignored it for this PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

from cryptography.hazmat.primitives.asymmetric.dsa import DSAPrivateKey, DSAPublicKey
from cryptography.hazmat.primitives.asymmetric.ec import EllipticCurvePrivateKey, EllipticCurvePublicKey
from cryptography.hazmat.primitives.asymmetric.ed25519 import Ed25519PublicKey
from cryptography.hazmat.primitives.asymmetric.rsa import RSAPrivateKey, RSAPublicKey

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm a bit confused about why these were here in the first place, since this doesn't seem to be re-exporting.

@github-actions

This comment has been minimized.

3 similar comments
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions
Copy link
Contributor

github-actions bot commented Jun 9, 2021

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

@@ -40,8 +40,8 @@ class Any(google___protobuf___message___Message, google___protobuf___internal___

def __init__(self,
*,
type_url : typing___Optional[typing___Text] = None,
value : typing___Optional[builtin___bytes] = None,
type_url : typing___Optional[typing___Text] = ...,
Copy link
Member

Choose a reason for hiding this comment

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

Aren't these stubs autogenerated? We'll need to update the generation script.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

IIRC, they originally were, but since then there have been manual updates.

Copy link
Member

Choose a reason for hiding this comment

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

OK, then ideally we should blacken them too. I'll merge this now and we can deal with the fallout if we try to update the generated stubs.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

They have been blackened back during the Great Blackening. (Great Blackout?) I don't know how they are generated, maybe the script is part of the protobuf repository?

Copy link
Member

Choose a reason for hiding this comment

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

Looks like it's scripts/generate_proto_stubs.sh.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

D'oh!

@JelleZijlstra JelleZijlstra merged commit 7115807 into python:master Jun 9, 2021
@srittau srittau deleted the pyright branch June 9, 2021 14:15
@jakebailey
Copy link
Contributor

@erictraut

I'm not so sure it's a good idea to be disabling every unknown rule... These filter into other users and mean that strict users are going to start seeing errors in their code due to missing types in typeshed.

@srittau
Copy link
Collaborator Author

srittau commented Jun 9, 2021

But wouldn't that be a problem anyway when users start using these stubs? I am not sure what mypy does, but it seems not to apply the same strictness rules to stubs as it does to user code.

@erictraut
Copy link
Contributor

I agree with Jake that these rules should not be disabled. I would prefer to leave a library in the "exclude" section until it fully meets these requirements. Otherwise we're lowering the bar for all stubs, which is heading in the wrong direction.

@jakebailey
Copy link
Contributor

I'm not sure what you mean. Not having users see unknown when they use stubs from places like the standard lib is the goal; now we have no way to prevent this like we did before the CI checks (which meant Eric would only find them when he pulled and ran against his team's codebase).

@srittau
Copy link
Collaborator Author

srittau commented Jun 9, 2021

Partly-annotated stubs are a large part of typeshed and will remain this way for the foreseeable future. We want to lower the bar to stubs submission and don't expect users to fully annotate stubs that gets submitted. In fact, we very much prefer only partly annotated stubs over no stubs and I expect the number of partly annotated stubs to grow as typeshed grows. For example, as far as I understand, @JukkaL plans to submit a huge bunch of auto-generated stubs at some point.

Re-enabling these options would mean that pyright would not check the majority of the stubs in typeshed in the future.

@srittau
Copy link
Collaborator Author

srittau commented Jun 9, 2021

One thing I can see, though, is enabling these option for the stdlib and "known-complete" third-party stubs. This would be useful to avoid regressions.

@erictraut
Copy link
Contributor

Ensuring that the most commonly-used stubs (especially the stdlib stubs) are complete and usable in the strictest type checking modes is way more important than checking the less-frequently used stubs.

It sounds like we might want to consider ways to enable specific rules for specific subdirectories. That would be a pretty significant feature to implement, but it's something we could consider.

@erictraut
Copy link
Contributor

Here's an idea. Pyright allows you to specify a config file by path. By default it looks for a config file called "pyrightconfig.json" in the root directory, but you can specify a different config file. We could define two separate config files and run two separate passes. The first pass would use stricter settings and would exclude libraries that are not complete, the second pass would use laxer settings and would include all libraries. Thoughts?

@srittau
Copy link
Collaborator Author

srittau commented Jun 9, 2021

Using two configs for "known-good" and other stubs sounds like the best solution. But we should probably move this discussion out of this PR into a separate issue.

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.

5 participants