Skip to content

More pywin32 stub completion #9308

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 14 commits into from
Dec 5, 2022
Merged

Conversation

Avasam
Copy link
Collaborator

@Avasam Avasam commented Dec 1, 2022

Completed based on usage of the following libraries in mypy_primer:

  • apprise
  • comtypes

Libraries that I use which also uses pywin32:

  • PyWinCtl
  • pywin32 itself (probably the biggest user of its own stubs)

As well as some of the most popular libraries that use both pywin32 and mypy (all over 1k stars on github):

  • certbot
  • anki
  • flexget
  • monkey
  • twisted
  • salt

Completed based on usage of the following libraries in mypy_primer:
- apprise
- comtypes
As well as some of the most popular libraries that use both pywin32 and mypy (all over 1k stars on github):
- certbot
- anki
- flexget
- monkey
- twisted
- salt
@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.

I've highlighted a couple of places where it looks like you maybe wanted to make parameters positional-only, but you only added a single leading underscore instead of two :)

@Avasam
Copy link
Collaborator Author

Avasam commented Dec 2, 2022

I've highlighted a couple of places where it looks like you maybe wanted to make parameters positional-only, but you only added a single leading underscore instead of two :)

sigh, I keep doing this and not spotting it. Can't wait for / to be supported. Thanks!

@Avasam
Copy link
Collaborator Author

Avasam commented Dec 3, 2022

I've updated a bit more by running the stub against pywin32's own code. Helped find a few issues with C modules.

@github-actions

This comment has been minimized.

@AlexWaygood
Copy link
Member

Not sure why mypy_primer is being so slow today — I filed hauntsaninja/mypy_primer#62

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@AlexWaygood
Copy link
Member

What's with the changes to distutils, pickle and importlib you made in 49d6722? Shouldn't they be a separate PR(s)? 🧐

@github-actions

This comment has been minimized.

@Avasam
Copy link
Collaborator Author

Avasam commented Dec 3, 2022

What's with the changes to distutils, pickle and importlib you made in 49d6722? Shouldn't they be a separate PR(s)? 🧐

They're all related to fixing type-checking errors in pywin32. But yes, I'll submit them separately.

@github-actions

This comment has been minimized.

@Avasam Avasam requested a review from AlexWaygood December 5, 2022 05:02
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.

Two more things I noticed, sorry!

Comment on lines 9 to 17
class _DispatchCreateClass(Protocol):
def __init__(
self,
IDispatch: str | PyIDispatchType | _GoodDispatchTypes | PyIUnknownType,
olerepr: build.DispatchItem | build.LazyDispatchItem,
userName: str | None = ...,
UnicodeToString: None = ...,
lazydata: Incomplete | None = ...,
): ...
Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately, I don't think this works the way you want it to :( __init__ methods in protocols are an edge case that are kind of weird and somewhat confusing, both at runtime and in how they're treated by static type checkers. The runtime behaviour is a whole other discussion, so let's stick to the static aspects here ;)

Type checkers don't regard __init__ methods as part of the interface that a user declares when inheriting from typing.Protocol. That means that they won't emit an error on code like the following, which you might expect them to flag:

from typing import Protocol

class Foo(Protocol):
    def __init__(self, arg: str) -> None: ...

class Bar:
    def __init__(self, arg1: int, arg2: bytes) -> None: ...

def expects_foo(x: Foo) -> None:
    pass

expects_foo(Bar(42, b"bar"))

The reasons why they won't emit an error on code like that has to do with the Liskov Substitution Principle, which is generally considered only to apply to objects that have already been constructed -- the constructor methods of objects are considered outside of the purview of the LSP. Some further reading on this, if you're interested:

Copy link
Collaborator Author

@Avasam Avasam Dec 5, 2022

Choose a reason for hiding this comment

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

image
image
image

But I also tried reimplementing it using Callback Protocols and it seems to be working just as well. So I can use that if you prefer:

_T_co = TypeVar("_T_co", covariant=True)
_T = TypeVar("_T")

class _DispatchCreateClass(Protocol[_T_co]):
    @staticmethod
    def __call__(
        IDispatch: str | PyIDispatchType | _GoodDispatchTypes | PyIUnknownType,
        olerepr: build.DispatchItem | build.LazyDispatchItem,
        userName: str | None = ...,
        UnicodeToString: None = ...,
        lazydata: Incomplete | None = ...,
    ) -> _T_co: ...

@overload
def Dispatch(
    IDispatch: str | PyIDispatchType | _GoodDispatchTypes | PyIUnknownType,
    userName: str | None,
    createClass: _DispatchCreateClass[_T],
    typeinfo: _win32typing.PyITypeInfo | None = ...,
    UnicodeToString: None = ...,
    clsctx: int = ...,
) -> _T: ...
# [...]

Sidenote, the parameter createClass of the first overload should not be | None either way. Fixing that as well.

Copy link
Collaborator Author

@Avasam Avasam Dec 5, 2022

Choose a reason for hiding this comment

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

If I change the type of a parameter, mypy won't catch it with __init__. So Callback protocols it is.

Fun fact, pyright did catch it however:
image

@AlexWaygood
Copy link
Member

They're all related to fixing type-checking errors in pywin32. But yes, I'll submit them separately.

Ah, I wasn't sure! But yeah, better to do them in a follow-up PR -- thanks!

@github-actions

This comment has been minimized.

@Avasam Avasam requested a review from AlexWaygood December 5, 2022 20:26
@github-actions
Copy link
Contributor

github-actions bot commented Dec 5, 2022

Diff from mypy_primer, showing the effect of this PR on open source code:

comtypes (https://github.com/enthought/comtypes)
- comtypes/test/test_dispinterface.py:21: error: Cannot find implementation or library stub for module named "win32com.client.gencache"  [import]

Comment on lines +12 to +20
class _DispatchCreateClass(Protocol[_T_co]):
@staticmethod
def __call__(
IDispatch: str | PyIDispatchType | _GoodDispatchTypes | PyIUnknownType,
olerepr: build.DispatchItem | build.LazyDispatchItem,
userName: str | None = ...,
UnicodeToString: None = ...,
lazydata: Incomplete | None = ...,
) -> _T_co: ...
Copy link
Member

Choose a reason for hiding this comment

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

I'm still a little uncertain that this is doing what you want it to, but I don't want to hold up this PR any longer if you're confident it works for your purposes :)

(And we can always fix it later if there is a problem.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks! It definitely seems to do what I what it to. At the very least it acts the way I expect.

@AlexWaygood AlexWaygood merged commit 0c3cf8f into python:main Dec 5, 2022
@Avasam Avasam deleted the pywin32-mypy-primer branch December 5, 2022 23:58
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.

2 participants