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

Explain why we subclass ImageQt with Any #9461

Closed
wants to merge 1 commit into from

Conversation

Avasam
Copy link
Collaborator

@Avasam Avasam commented Jan 5, 2023

These are way too complex, with 4 different possible sources (2 deprecated)
And we don't want to force the user to install PyQt or Pyside when they may not even use it.

Fixes 6 subclassing errors (the same error in Pillow, D3DShot, python-xlib, fpdf2, PyScreeze, and PyAutoGUI stubs)

Ref: #9491

@github-actions
Copy link
Contributor

github-actions bot commented Jan 5, 2023

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

Copy link
Collaborator

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

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

Maybe this should be a protocol?

@Avasam
Copy link
Collaborator Author

Avasam commented Jan 5, 2023

Maybe this should be a protocol?

I thought about it, but ImageQt is used as a return type, so it'd have to completely match QtImage from all 4 implementations. I did a quick try and it got messy real quick with thousands of lines copied from PyQt6.

Protocol could probably be simplified by using Any when it starts referencing other types (so at least we have all methods and members). But would a protocol still work as a return type?

@AlexWaygood
Copy link
Member

Protocol could probably be simplified by using Any when it starts referencing other types (so at least we have all methods and members). But would a protocol still work as a return type?

Protocols can be used as a return type! I agree with @hauntsaninja that we can probably at least try to roughly describe the shape of the superclass here.

@AlexWaygood AlexWaygood changed the title Explain why we sublass ImageQt with Any Explain why we subclass ImageQt with Any Jan 5, 2023
@Avasam
Copy link
Collaborator Author

Avasam commented Jan 5, 2023

I meant that I'm concerned about the variance:

from typing import Protocol

class Abc:
    foo: str
    
class AbcProtocol(Abc, Protocol): ...  # type: ignore[misc]

reveal_type(AbcProtocol().foo). # str

class Abcd:
    foo: str
   
abcd: Abcd
abcd = AbcProtocol()  # Incompatible types in assignment (
abcproto: AbcProtocol = Abcd()

@hauntsaninja
Copy link
Collaborator

I'm not sure I follow, wouldn't users just use the QImage type hint?
(Or is it a case where users do actually know what type they're getting back and e.g. do nominal checking or the cases where the types' APIs are non-overlapping are actually important to users?)

@Avasam
Copy link
Collaborator Author

Avasam commented Jan 7, 2023

Here's a minimal example in PyQt6. This is what we need to support (without actually having to install PyQt6, PyQt5, PySide6 or PySide2 when installing types-pillow). Would using a protocol for ImageQt's base work thanks to duck typing? And how complex would that protocol have to be? (QImages have lots of methods, members and bases).

from PyQt6 import QtGui
from PIL import Image, ImageQt
data: memoryview  # Some graphics data from elsewhere in the code
pil_image: Image.Image  # Some pillow image from elsewhere in the code

def show_qimage(image: QtGui.QImage):
    # Show the image someplace in the GUI
    pass

# With PyQt directly
qimage = QtGui.QImage(data, 640, 480, 640 * 4, QtGui.QImage.Format.Format_RGBA8888)
show_qimage(qimage)

# using the pillow wrapper instead
qimage = ImageQt.ImageQt(pil_image)
show_qimage(qimage) # Argument of type "ImageQt" cannot be assigned to parameter "image" of type "QImage" in function "show_qimage" "ImageQt" is incompatible with "QImage"

Would using a protocol for ImageQt's base work thanks to duck typing?

Edit: Don't think so

class A:
    def test(self, foo: str) -> int: ...


class ProtocolA(Protocol):
    def test(self, foo: str) -> int: ...


protoA = cast(ProtocolA, A())

a: A = protoA  # Expression of type "ProtocolA" cannot be assigned to declared type "A"  "ProtocolA" is incompatible with "A"

@Avasam
Copy link
Collaborator Author

Avasam commented Jan 11, 2023

@AlexWaygood , I don't see an "easy" solution for this (or any good solution at all). Since type-checkers can't have a conditional type based on if a module exists or not. (without having to check for a full ImportError, which can't be done statically, I feel like checking for a module's existence could be a beneficial proposition)

Can I move this to #9491 ?

@AlexWaygood
Copy link
Member

@AlexWaygood , I don't see an "easy" solution for this (or any good solution at all). Since type-checkers can't have a conditional type based on if a module exists or not. (without having to check for a full ImportError, which can't be done statically, I feel like checking for a module's existence could be a beneficial proposition)

Can I move this to #9491 ?

I'm fine with that. It's not like we're introducing a regression here, after all -- we're already sbuclassing Any in the stub. If anybody wants to invest some time into working out a more principled solution, they're always welcome to do so at a later date!

@Avasam
Copy link
Collaborator Author

Avasam commented Jan 12, 2023

Closing in favor of #9491

@Avasam Avasam closed this Jan 12, 2023
@Avasam Avasam deleted the ImageQt-any-subclassing branch February 29, 2024 00:44
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