Skip to content
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

Add precise values for enum members where possible #11299

Merged
merged 39 commits into from
Apr 22, 2024

Conversation

erictraut
Copy link
Contributor

@erictraut erictraut commented Jan 22, 2024

This means that typeshed's enums now broadly conform with the proposed changes to the typing spec that are discussed at the following two links:

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

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.

Overall this looks great, thank you!

@@ -21,62 +19,62 @@ class CoerciveIntFlag(IntFlag):
def coerce(cls, value: Self | str | int) -> Self: ...

class WrapMode(CoerciveEnum):
WORD: str
CHAR: str
WORD = ...
Copy link
Member

Choose a reason for hiding this comment

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

You may have answered this already elsewhere, in which case sorry, but how are type checkers supposed to be able to infer the type of WrapMode.WORD.value with something like this? I think mypy is currently able to infer a str here, but I don't think it would be able to with this change.

My preference would be to use the runtime values wherever possible (as you're already doing elsewhere in this PR) but, where it's not possible, to introduce linting rules that enforce annotations for this kind of pattern, e.g.

Suggested change
WORD = ...
WORD: str = ...

Copy link
Member

Choose a reason for hiding this comment

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

Reading the discuss.python.org thread, I see you propose using _value_: str for cases where it's hard for us to provide the exact runtime value in the stub for each member. That will work in most cases, but it won't work for enums with heterogenous data types for their member values, e.g.

class Foo(Enum):
    BAR = 99999999999999999999999999999999999999999999999999999999
    BAZ = "baaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaz"

You might say that that's an antipattern, and I might agree with you, but we should still have a way to express that in the stubs if that's what the runtime is doing. The most important thing in a stub is for us to be accurate w.r.t. to the runtime, even if the runtime is making use of a bad antipattern that we wouldn't necessarily endorse when we were writing our own 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.

@AlexWaygood, if you think that the _value_ proposal is too constraining, please respond to the discuss.python.org thread and make your case there. Most of the participants in that broader thread are probably not following the detailed PR comments, so they won't see your above concerns.

This comment has been minimized.

@JelleZijlstra
Copy link
Member

I took a brief look at the remaining errors. The third-party stubtest run still produces a lot of errors, but most look relatively easy to fix. The stdlib stubtest run just complains about a single enum in ssl that may be doing something unusual.

scroll_left: int
scroll_right: int
scroll_up: int
button8 = ...
Copy link
Member

Choose a reason for hiding this comment

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

This change leads to a regression in how mypy infers the type of the value of this enum member. On main:

% MYPYPATH=stubs/pynput mypy -c "import pynput.mouse._base; reveal_type(pynput.mouse._base.Button.button8.value)" --platform linux
<string>:1: note: Revealed type is "builtins.int"

With this PR:

MYPYPATH=stubs/pynput mypy -c "import pynput.mouse._base; reveal_type(pynput.mouse._base.Button.button8.value)" --platform linux
<string>:1: note: Revealed type is "types.EllipsisType"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd prefer not to wait for a bug fix in mypy 1.10. That will take another several weeks.

Do you know where we can find the values for these enum members? IIRC, it wasn't obvious from the sources. I suppose we could use introspection to determine their values, but I'd need help from someone on a Linux and Windows machine to do this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Here are the values on linux with pynput==1.7.6:

>>> from pynput.mouse import Button
>>> list(Button)
[<Button.unknown: None>, <Button.left: 1>, <Button.middle: 2>, <Button.right: 3>, <Button.scroll_up: 4>, <Button.scroll_down: 5>, <Button.scroll_left: 6>, <Button.scroll_right: 7>, <Button.button8: 8>, <Button.button9: 9>, <Button.button10: 10>, <Button.button11: 11>, <Button.button12: 12>, <Button.button13: 13>, <Button.button14: 14>, <Button.button15: 15>, <Button.button16: 16>, <Button.button17: 17>, <Button.button18: 18>, <Button.button19: 19>, <Button.button20: 20>, <Button.button21: 21>, <Button.button22: 22>, <Button.button23: 23>, <Button.button24: 24>, <Button.button25: 25>, <Button.button26: 26>, <Button.button27: 27>, <Button.button28: 28>, <Button.button29: 29>, <Button.button30: 30>]

The stub is a bit misleading. These are actually defined in pynput.mouse._xorg, not in pynput.mouse._base. There is another, different Button enum in pynput.mouse._base, but that one lacks most of the buttons:

>>> Button.__module__
'pynput.mouse._xorg'
>>> from pynput.mouse._base import Button
>>> Button.__module__
'pynput.mouse._base'
>>> list(Button)
[<Button.unknown: 0>, <Button.left: 1>, <Button.middle: 2>, <Button.right: 3>]
>>> Button.button8
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib/python3.11/enum.py", line 789, in __getattr__
    raise AttributeError(name) from None
AttributeError: button8

Copy link
Member

Choose a reason for hiding this comment

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

I don't see these in the source code at all (https://github.com/moses-palmer/pynput/blob/12acf84dc0f721d91a957da65311497acb664933/lib/pynput/mouse/_base.py#L33), are they added dynamically somewhere?

I tried installing pynput on a Linux server but it fails to import; apparently it requires a running X server. I don't have time right now to try to cajole it, but I might take another look later.

Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer not to wait for a bug fix in mypy 1.10. That will take another several weeks.

Somewhat adjacent to the point you're making, but what does pyright infer for the value of the button8 member here @erictraut? I'm not sure this does qualify as a mypy bug really; I don't know how it could be expected to know that the value of the member really is an int here

Copy link
Member

Choose a reason for hiding this comment

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

Following bd170fa, here's what mypy now infers:

% MYPYPATH=stubs/pynput mypy -c "import pynput.mouse._base; reveal_type(pynput.mouse._base.Button.button8.value)" --platform linux
<string>:1: note: Revealed type is "Literal[0]?"

According to @Akuli's comment in #11299 (comment), that's also incorrect, since the value of the button8 member is 8. Why don't you just use 8 for this member, and 13 for the button13 member, etc., according to the values Akuli listed in #11299 (comment)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I missed @Akuli's post above. I added the linux-specific values. There are still two windows-specific values where the values are still unknown.

Copy link
Collaborator

@Avasam Avasam Apr 14, 2024

Choose a reason for hiding this comment

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

They are tuples under Windows. The x1 and x2 values simply don't exist in the _base.Button enum, but do in _win32.Button:
https://github.com/moses-palmer/pynput/blob/12acf84dc0f721d91a957da65311497acb664933/lib/pynput/mouse/_win32.py#L48-L56

They correspond to "Mouse 4" and "Mouse 5" buttons

>>> from pynput.mouse import Button
>>> Button.left
<Button.left: (4, 2, 0)>
>>> Button.middle
<Button.middle: (64, 32, 0)>
>>> Button.right
<Button.right: (16, 8, 0)>
>>> Button.x1
<Button.x1: (256, 128, 1)>
>>> Button.x2
<Button.x2: (256, 128, 2)>

It seems the pynput stubs is doing the worst of both world between trying to hide implementation details (private modules) and replicating the source (only filling private module for what we need). By filling in _base, but also not mirroring what's actually hapenning.

Anyway for @erictraut 's needs for this PR, I posted above the values on Windows.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, we might want to do a round of review on these stubs to make them follow the implementation more closely, that would make it easier to verify correctness. That's out of scope for this PR, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I appreciate the desire to make these values perfectly match the implementation, but I'd appreciate it if we could unblock this PR (which is itself blocking a bunch of things) and then do a separate pass to refine these types. Are you OK with that?

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.

All these changes look good to me other than https://github.com/python/typeshed/pull/11299/files#r1560806163, which seems like a clear regression in user experience. The last remaining CI error will be fixed by PyCQA/flake8-pyi#479; we can do a hotfix release for that after it's merged, to get CI passing here.

.flake8 Outdated Show resolved Hide resolved

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

f18: int
f19: int
f20: int
alt = 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 appears these values are actually KeyCode object, not ints. I'll try to push a fix.

Copy link
Member

Choose a reason for hiding this comment

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

Oh wait, that wasn't released. In the last release all the values are really 0.

Copy link
Member

Choose a reason for hiding this comment

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

For reference: moses-palmer/pynput@4ee72f9. The last release was in January 2022.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

@JelleZijlstra
Copy link
Member

I would like to merge this, regardless of what happens with the proposed spec change, since the change adds strictly more information to the stubs and as far as I can tell no longer regresses anything for mypy users.

cc @AlexWaygood since you added the "DO NOT MERGE" label.

@AlexWaygood
Copy link
Member

AlexWaygood commented Apr 15, 2024

cc @AlexWaygood since you added the "DO NOT MERGE" label.

I added that ages ago because Eric's PR description seemed to indicate that that was what he wanted (the description still says "DO NOT MERGE" in it now). Happy for it to be removed :-)

I agree that overall this PR improves things greatly for users of these stubs, and we should land a version of it regardless of whether the spec changes land. I'm still unsure about one or two of the specific changes — I'll add inline comments shortly

stubs/fpdf2/fpdf/enums.pyi Outdated Show resolved Hide resolved
Comment on lines +319 to +320
SERVER_AUTH = (129, "serverAuth", "TLS Web Server Authentication", "1.3.6.1.5.5.7.3.2") # pyright: ignore[reportCallIssue]
CLIENT_AUTH = (130, "clientAuth", "TLS Web Client Authentication", "1.3.6.1.5.5.7.3.1") # pyright: ignore[reportCallIssue]
Copy link
Member

Choose a reason for hiding this comment

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

I can't point to a concrete regression here, as this enum seems to be just too complex for mypy both before and after -- it infers the value of the members as Any both on main and on your PR branch. But I can't say that I love this, since at runtime the values for these enums are actually _ASN1Object instances (where the members can be accessed by name as well as index), not tuples:

>>> import ssl
>>> ssl.Purpose.SERVER_AUTH.value.nid
129
>>> ssl.Purpose.SERVER_AUTH.value.shortname
'serverAuth

But I get that the runtime is being somewhat dynamic here and that this is really difficult to represent statically in a stub, so I don't think there's a good solution here. This comment is non-blocking.

Copy link
Contributor

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

@AlexWaygood AlexWaygood changed the title Changed enums to conform with proposed change to typing spec discusse… Add precise values for enum members where possible Apr 22, 2024
@AlexWaygood AlexWaygood merged commit 17f1c46 into python:main Apr 22, 2024
57 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.

6 participants