Fix tkinter ImageSpec #11721

[kbaikov]

Fixes #11721

[kbaikov]

@srittau Do i need to comment Any here too? If so what would be the appropriate comment.

[Akuli]

Thanks for working on this! Tkinter is more tricky than you'd expect :)

After thinking about this again, I'm pretty sure that using Any is not a good idea. The problem is that it doesn't detect the following mistake, which is easy to make and gives a misleading error message at runtime:

import tkinter
import PIL.Image

root = tkinter.Tk()
img = PIL.Image.open("foo.png")
label = tkinter.Label(image=img)

The problem is that you forgot to wrap the PIL image in a PIL.ImageTk image. This is the correct code:

import tkinter
import PIL.Image
import PIL.ImageTk

root = tkinter.Tk()
img = PIL.Image.open("foo.png")
img_tk = PIL.ImageTk.PhotoImage(img)
label = tkinter.Label(root, image=img_tk)

To make type checkers complain about the wrong code, let's use the following protocol:

class _ImageLike(Protocol):
    def width(self) -> int: ...
    def height(self) -> int: ...

This matches tkinter's image classes (PhotoImage and BitmapImage) and PIL's tkinter-compatible class (PIL.ImageTk.PhotoImage), but not a plain PIL image that isn't tkinter compatible. The reason is that PIL has width and height attributes, not methods:

>>> img.width
788
>>> img_tk.width
<bound method PhotoImage.width of <PIL.ImageTk.PhotoImage object at 0x7fb2712e6510>>

Do you want to change this PR, or do you want me to make a new PR with the protocol?

[Akuli]

Apparently this has been a protocol before, but it was changed in #9733. There I said:

I'll probably merge this in a few days, unless someone comes up with a better idea or some reasoning to prefer the width(),height() protocol over fake classes.

And now that PIL ships its own type stubs, that is a good reason to prefer a protocol over fake classes that don't really work anymore.

[github-actions]

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

[kbaikov]

Do you want to change this PR, or do you want me to make a new PR with the protocol?

@Akuli i will do it. Just pushed the fix.
Not sure i fixed everything you mentioned.
Also how do i run the tests only for tkinter and PIL locally for example? Are those stub tests? regression tests? I read the tests/README.md but not sure i got everything from there.

Also do i need to squash my commits, or in this repo you follow the CPython workflow were commits are squashed on merge?

[Akuli]

Your changes look good. Thanks!

regression tests?

I don't think we have a good way to ensure that PIL is installed while running tkinter tests. So I installed PIL and mypy locally and ran mypy over the code from my previous comment. Your branch works great :)

stub tests?

The CI runs stubtest on all stubs (including tkinter), and it's green. That said, stubtest doesn't work very well with tkinter, because it doesn't instantiate any classes, it only imports them. With tkinter, a lot of tricky things happen when you do things at runtime.

Also do i need to squash my commits, or in this repo you follow the CPython workflow were commits are squashed on merge?

You don't need to squash, we use "Squash and merge" :)