Fix WindowType and WindowFlags integer operations #145

bluebird75 · 2021-03-26T14:53:54Z

Before the fix, the test would report:

windowflags.py:10: error: Incompatible types in assignment (expression has type "int", variable has type "WindowType")
windowflags.py:18: error: Incompatible types in assignment (expression has type "int", variable has type "WindowFlags")
windowflags.py:19: error: Incompatible types in assignment (expression has type "int", variable has type "WindowFlags")
windowflags.py:20: error: Incompatible types in assignment (expression has type "int", variable has type "WindowFlags")
windowflags.py:21: error: Incompatible types in assignment (expression has type "int", variable has type "WindowFlags")
windowflags.py:22: error: Incompatible types in assignment (expression has type "int", variable has type "WindowFlags")
windowflags.py:27: error: Unsupported operand types for + ("WindowFlags" and "WindowType")
windowflags.py:27: error: Incompatible types in assignment (expression has type "int", variable has type "WindowFlags")
windowflags.py:28: error: Unsupported operand types for - ("WindowFlags" and "WindowType")
windowflags.py:28: error: Incompatible types in assignment (expression has type "int", variable has type "WindowFlags")
windowflags.py:29: error: Unsupported operand types for | ("WindowFlags" and "WindowType")
windowflags.py:29: error: Incompatible types in assignment (expression has type "int", variable has type "WindowFlags")
windowflags.py:30: error: Unsupported operand types for & ("WindowFlags" and "WindowType")
windowflags.py:30: error: Incompatible types in assignment (expression has type "int", variable has type "WindowFlags")
windowflags.py:31: error: Unsupported operand types for ^ ("WindowFlags" and "WindowType")
windowflags.py:31: error: Incompatible types in assignment (expression has type "int", variable has type "WindowFlags")
windowflags.py:34: error: Incompatible types in assignment (expression has type "int", variable has type "WindowFlags")
windowflags.py:34: error: Unsupported operand types for + ("WindowType" and "WindowFlags")
windowflags.py:35: error: Incompatible types in assignment (expression has type "int", variable has type "WindowFlags")
windowflags.py:35: error: Unsupported operand types for - ("WindowType" and "WindowFlags")
windowflags.py:36: error: Incompatible types in assignment (expression has type "int", variable has type "WindowFlags")
windowflags.py:36: error: Unsupported operand types for | ("WindowType" and "WindowFlags")
windowflags.py:37: error: Incompatible types in assignment (expression has type "int", variable has type "WindowFlags")
windowflags.py:37: error: Unsupported operand types for & ("WindowType" and "WindowFlags")
windowflags.py:38: error: Incompatible types in assignment (expression has type "int", variable has type "WindowFlags")
windowflags.py:38: error: Unsupported operand types for ^ ("WindowType" and "WindowFlags")
windowflags.py:41: error: Unsupported left operand type for + ("WindowFlags")
windowflags.py:42: error: Unsupported left operand type for - ("WindowFlags")
windowflags.py:43: error: Unsupported left operand type for | ("WindowFlags")
windowflags.py:44: error: Unsupported left operand type for & ("WindowFlags")
windowflags.py:45: error: Unsupported left operand type for ^ ("WindowFlags")

Before the fix, the test would report: windowflags.py:10: error: Incompatible types in assignment (expression has type "int", variable has type "WindowType") windowflags.py:18: error: Incompatible types in assignment (expression has type "int", variable has type "WindowFlags") windowflags.py:19: error: Incompatible types in assignment (expression has type "int", variable has type "WindowFlags") windowflags.py:20: error: Incompatible types in assignment (expression has type "int", variable has type "WindowFlags") windowflags.py:21: error: Incompatible types in assignment (expression has type "int", variable has type "WindowFlags") windowflags.py:22: error: Incompatible types in assignment (expression has type "int", variable has type "WindowFlags") windowflags.py:27: error: Unsupported operand types for + ("WindowFlags" and "WindowType") windowflags.py:27: error: Incompatible types in assignment (expression has type "int", variable has type "WindowFlags") windowflags.py:28: error: Unsupported operand types for - ("WindowFlags" and "WindowType") windowflags.py:28: error: Incompatible types in assignment (expression has type "int", variable has type "WindowFlags") windowflags.py:29: error: Unsupported operand types for | ("WindowFlags" and "WindowType") windowflags.py:29: error: Incompatible types in assignment (expression has type "int", variable has type "WindowFlags") windowflags.py:30: error: Unsupported operand types for & ("WindowFlags" and "WindowType") windowflags.py:30: error: Incompatible types in assignment (expression has type "int", variable has type "WindowFlags") windowflags.py:31: error: Unsupported operand types for ^ ("WindowFlags" and "WindowType") windowflags.py:31: error: Incompatible types in assignment (expression has type "int", variable has type "WindowFlags") windowflags.py:34: error: Incompatible types in assignment (expression has type "int", variable has type "WindowFlags") windowflags.py:34: error: Unsupported operand types for + ("WindowType" and "WindowFlags") windowflags.py:35: error: Incompatible types in assignment (expression has type "int", variable has type "WindowFlags") windowflags.py:35: error: Unsupported operand types for - ("WindowType" and "WindowFlags") windowflags.py:36: error: Incompatible types in assignment (expression has type "int", variable has type "WindowFlags") windowflags.py:36: error: Unsupported operand types for | ("WindowType" and "WindowFlags") windowflags.py:37: error: Incompatible types in assignment (expression has type "int", variable has type "WindowFlags") windowflags.py:37: error: Unsupported operand types for & ("WindowType" and "WindowFlags") windowflags.py:38: error: Incompatible types in assignment (expression has type "int", variable has type "WindowFlags") windowflags.py:38: error: Unsupported operand types for ^ ("WindowType" and "WindowFlags") windowflags.py:41: error: Unsupported left operand type for + ("WindowFlags") windowflags.py:42: error: Unsupported left operand type for - ("WindowFlags") windowflags.py:43: error: Unsupported left operand type for | ("WindowFlags") windowflags.py:44: error: Unsupported left operand type for & ("WindowFlags") windowflags.py:45: error: Unsupported left operand type for ^ ("WindowFlags")

bluebird75 · 2021-03-26T14:54:57Z

fixes #142

BryceBeagle · 2021-03-26T15:13:47Z

Looks like these changes cause the tests to fail in CI.

Also can you add an entry to the CHANGELOG.md? We should probably document PR workflow somewhere...

…t annotaions

bluebird75 · 2021-03-26T22:05:44Z

This was more difficult than I expected, I had to cover all cases to make sure it works correctly. Let's see what CI says.

PyQt5-stubs/QtCore.pyi

bluebird75 · 2021-03-27T09:53:03Z

I just re-read carefully the Qt documentation about QFlags ( https://doc.qt.io/qt-5/qflags.html#details ) . The following points are interesting :

If you try to pass a value from another enum or just a plain integer other than 0, the compiler will report an error. If you need to cast integer values to flags in a untyped fashion, you can use the explicit QFlags constructor as cast operator.

Then makes me think that combinations of QFlags with int is discouraged and maybe, I should raise a TypeError there just like in C++. Looking at the operator implemented in QFlags for int, I see :

operator & and &= both supports int (like I did)
operator | |= ^ ^= do not support int at all (unlike what I did)
no addition or substraction is definied (like I did)

So, we could be theorically faithful to QFlags by removing int support in OR and XOR operators, or we could just reflect the PyQt reality that it works fine also it is discouraged. I lean toward the first case (limiting int support) because it provides more typesafety in using those QFlags.

BryceBeagle · 2021-03-27T14:26:54Z

So, we could be theorically faithful to QFlags by removing int support in OR and XOR operators, or we could just reflect the PyQt reality that it works fine also it is discouraged. I lean toward the first case (limiting int support) because it provides more typesafety in using those QFlags.

I think so far we've been maintaining these stubs to be true to the PyQt implementation so far, so I'd be a bit hesitant to start making them opinionated. Maybe we could reach out to Phil on the mailing list to see if he'd want to make PyQt itself better reflect Qt's design? He's already been changing a lot of things about the Enums/Flags in PyQt6; maybe he'd be open to making this change too.

altendky

@BryceBeagle, had you gone over this in any detail? Definitely no need for us both to do so.

But yeah, this is all a horrible mess having three different 'interchangeable' types. The reality of the library, that is, not this PR. The PR is appreciated. :]

altendky · 2021-03-27T17:51:35Z

tests/windowflags.py

+try:
+    windowFlagsTest = windowFlags1 + windowFlags2		# type: ignore
+    assert False
+except TypeError:
+    pass


I think this would work.

with pytest.raises(TypeError): windowFlagsTest = windowFlags1 + windowFlags2

I ran it without py.test because it would not work very well on my computer. I am not sure how much you have tested all the flow on Windows but I was not interested in debugging it. Anyway, none of the other tests rely on py.test so this was as good as it gets.

Thanks for the explanation. I've only done #123 for Windows so far... :[

bluebird75 · 2021-03-27T20:16:37Z

PyQt5-stubs/QtCore.pyi

@@ -2376,6 +2376,20 @@ class Qt(sip.simplewrapper):
    WindowActive = ...  # type: Qt.WindowState

    class WindowType(int):
+        def __init__(self, f: typing.Union['Qt.WindowType', int]) -> None: ...
+        def __int__(self) -> int: ...
+        def __invert__(self) -> 'Qt.WindowFlags': ...


Actually, this is wrong. I need to ditch away all the WindowType modifications because they break the Liskov principle : if you inherit int, overloaded method should return something which is int compatible, so clearlyQt.WindowFlags which does not inherit int is not a good candidate. This applies to all the methods below.

I guess we'll have to do just like in C++, where WindowType operations are not correctly checked because they become int after an operation.

That's why I initially ditched the int inheritance.

bluebird75 · 2021-03-27T20:19:40Z

PyQt5-stubs/QtCore.pyi

+        def __rxor__(self, other: int) -> 'Qt.WindowFlags': ...
+        def __radd__ (self, other: int) -> 'Qt.WindowFlags': ...
+        def __rsub__ (self, other: int) -> 'Qt.WindowFlags': ...
+


Same comment as above, this all has to go.

bluebird75 · 2021-03-27T20:23:10Z

I'll try to reach to Phil Thompson but there a decision to be made here anyway :-) . I like it how it is now as far as I am concerned.

altendky · 2021-03-27T22:14:39Z

@bluebird75, I see you left a couple comments saying things need to change and then one saying you like it how it is. CI is failing so something needs to change somewhere. I haven't yet looked to figure out where. We certainly do have plenty of ignores strewn about for places where accurately representing what PyQt does results in Mypy complaining that we are doing bad things. You can see the # type: ignore[misc] over in https://github.com/python-qt-tools/PyQt5-stubs/pull/109/files, for example. I know there are a lot of ignores specifically for Liskov violations. As mentioned, so far we have focused on accuracy over driving users towards a certain 'style'.

Let us know if you want to make some changes or if you'd like us to jump in and help figure this out.

…dowtype-operations

BryceBeagle · 2021-03-29T19:57:04Z

@BryceBeagle, had you gone over this in any detail? Definitely no need for us both to do so.

No, I haven't had a chance to look into this in detail. I've just been making some high-level comments/observations. 👍

Breaking Liskov subsitution is OK because that's what PyQt5 does

…bclass

bluebird75 · 2021-04-19T21:00:06Z

finally, ready to be merged !

altendky · 2021-08-13T15:34:06Z

Are you still working on this or would you like me to see if I can help with the failures?

bluebird75 · 2021-08-13T16:17:56Z

I would like to get this one done, because on the principle, it's a much more appropriate way to describe a QFlag based class. Assuming we get it to work here, all I need to do on other qflag based classes is change the inheritance.

I'll give it a short attempt on fixing it and ask for help if I am unsuccessful.

bluebird75 · 2021-08-13T16:28:10Z

The CI fails on the mypy check but it passes fine on my computer :

d:\work\pyqt5-stubs\PyQt5-stubs>mypy --show-error-codes  PyQt5-stubs
Success: no issues found in 46 source files

d:\work\pyqt5-stubs\PyQt5-stubs>mypy --version
mypy 0.812

Which brings two questions :

which version of mypy is good enough for pyqt5-stubs verification ? I see that the requirements has pinned down a very specific commit.
the failure is about returning an invariant type where mypy would expect a covariant (as argument) or a contra variant (as a return value). While this is a fine approach in general, in this specific case, returning an invariant is actually correct because the API is really set in stone. This is probably why this error has been removed with later versions of mypy.

I can probably fix it by using co/contra variants but I would like to be settled on the mypy version first.

bluebird75 · 2021-08-13T16:32:26Z

OK. One clear problem is that I did not setup my local requirements per requirements\develop.in . If I do that, I am sure that I will use the expected version of mypy.

Still, it would be a good idea to have a CI run with the latest mypy to make sure there are no differences for users using the most recent version.

bluebird75 · 2021-08-13T21:01:03Z

I tried to setup the equivalent of the CI on my computer using the classical :

pip install -r requirements\develop.txt

This mandates mypy==0.641 and pytest==4.0.1 . However, according to the tox.ini file, the requirements are actually mypy @ git+https://github.com/python/mypy@538d36481526135c44b90383663eaa177cfc32e3 pytest .

I guess tox.ini has the final word on this and I should install these versions ?

altendky · 2021-08-16T15:45:45Z

I'm not going to do a "complete" fixup of the requirements structure, but I am updating them over in #161. The mypy reference in tox was not great and is being removed. It was for some stubtest fix that wasn't released at that point in time. It shouldn't have been left like that without a note explaining it. In general, I try to keep things working with latest versions of dependencies.

Aside, I have had various issues where I can't recreate the CI results locally. It's been awhile so I forget how much I did figure out about that but I know some days I've just given up and gone off what CI says. This is certainly not a great state of affairs but I haven't yet prioritized addressing that.

altendky · 2021-09-10T15:25:45Z

Welp, I gave up on the reviews for #161 and merged it. I'll take the liberty to handle the catch up merge conflicts to avoid back and forth here. Just be sure to pull my changes.

bluebird75 · 2021-11-12T12:47:34Z

This PR has been superseded by #153 .

Closing.

Full testing on WindowType, WindowFlags and int combinations + correc…

542d527

…t annotaions

BryceBeagle reviewed Mar 26, 2021

View reviewed changes

PyQt5-stubs/QtCore.pyi Outdated Show resolved Hide resolved

Update changelog

ba5b984

Make WindowType inherit int again (revert a previous change)

b328383

altendky reviewed Mar 27, 2021

View reviewed changes

bluebird75 commented Mar 27, 2021

View reviewed changes

bluebird75 added 6 commits March 29, 2021 14:43

Ignore the errors reported from breaking Liskov principle

e1c67d4

add one more test for windowFlags usage

1ad195a

Merge remote-tracking branch 'origin/master' into fix-windowflags-win…

aee419b

…dowtype-operations

WindowType as working cross verfication with mypy and py.test

53c1105

WindowFlags operations are verified by both mypy and pytest

eebb2cb

Also test assignment operator

af26357

bluebird75 added 5 commits April 19, 2021 14:33

Fix CI: overload not needed, breaking Liskov is OK here

8f1d77f

Breaking Liskov subsitution is OK because that's what PyQt5 does

Fix CI

64d3934

Make stubtest happy: there is no need for a __init__ method on int su…

77af902

…bclass

Fix CI: functions are not needed

e277af3

Make mypy happy

8a93e15

pfremy added 2 commits April 25, 2021 00:01

Make test for WindowFlags as generic as possible

f8052b0

Use QFlags typing generics to describe WindowFlags and WindowType

40525d8

pfremy and others added 2 commits April 28, 2021 15:49

Use typing.Protocol instead of Generic, it is more appropriate

155fced

Merge branch 'master' into fix-windowflags-windowtype-operations

5c1b159

bluebird75 added 3 commits August 19, 2021 21:27

try with a more recent version of mypy

b3a8eb9

Create tox.ini

43666d1

Update tox.ini

525eb9c

Merge branch 'master' into fix-windowflags-windowtype-operations

17e268f

bluebird75 closed this Nov 12, 2021

bluebird75 deleted the fix-windowflags-windowtype-operations branch November 12, 2021 12:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix WindowType and WindowFlags integer operations #145

Fix WindowType and WindowFlags integer operations #145

bluebird75 commented Mar 26, 2021 •

edited by altendky

Loading

bluebird75 commented Mar 26, 2021

BryceBeagle commented Mar 26, 2021

bluebird75 commented Mar 26, 2021

bluebird75 commented Mar 27, 2021

BryceBeagle commented Mar 27, 2021

altendky left a comment

altendky Mar 27, 2021

bluebird75 Mar 27, 2021

altendky Mar 27, 2021

bluebird75 Mar 27, 2021

bluebird75 Mar 27, 2021

bluebird75 Mar 27, 2021

bluebird75 commented Mar 27, 2021

altendky commented Mar 27, 2021

BryceBeagle commented Mar 29, 2021

bluebird75 commented Apr 19, 2021

altendky commented Aug 13, 2021

bluebird75 commented Aug 13, 2021

bluebird75 commented Aug 13, 2021

bluebird75 commented Aug 13, 2021

bluebird75 commented Aug 13, 2021

altendky commented Aug 16, 2021

altendky commented Sep 10, 2021

bluebird75 commented Nov 12, 2021

Fix WindowType and WindowFlags integer operations #145

Fix WindowType and WindowFlags integer operations #145

Conversation

bluebird75 commented Mar 26, 2021 • edited by altendky Loading

bluebird75 commented Mar 26, 2021

BryceBeagle commented Mar 26, 2021

bluebird75 commented Mar 26, 2021

bluebird75 commented Mar 27, 2021

BryceBeagle commented Mar 27, 2021

altendky left a comment

Choose a reason for hiding this comment

altendky Mar 27, 2021

Choose a reason for hiding this comment

bluebird75 Mar 27, 2021

Choose a reason for hiding this comment

altendky Mar 27, 2021

Choose a reason for hiding this comment

bluebird75 Mar 27, 2021

Choose a reason for hiding this comment

bluebird75 Mar 27, 2021

Choose a reason for hiding this comment

bluebird75 Mar 27, 2021

Choose a reason for hiding this comment

bluebird75 commented Mar 27, 2021

altendky commented Mar 27, 2021

BryceBeagle commented Mar 29, 2021

bluebird75 commented Apr 19, 2021

altendky commented Aug 13, 2021

bluebird75 commented Aug 13, 2021

bluebird75 commented Aug 13, 2021

bluebird75 commented Aug 13, 2021

bluebird75 commented Aug 13, 2021

altendky commented Aug 16, 2021

altendky commented Sep 10, 2021

bluebird75 commented Nov 12, 2021

bluebird75 commented Mar 26, 2021 •

edited by altendky

Loading