-
-
Notifications
You must be signed in to change notification settings - Fork 63
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
Use Pyright in CI #242
Use Pyright in CI #242
Changes from 5 commits
5d76ab7
7129938
fde6470
2105419
649a4c2
452b027
d9f9da1
5956a19
fb9d971
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,6 +28,10 @@ | |
from ._typing import KT, VT, MISSING, OKT, OVT, IterItems, MapOrIterItems | ||
|
||
|
||
# Disable some pyright strict diagnostics that are not helpful in this file: | ||
# pyright: reportPrivateUsage=false, reportUnknownArgumentType=false, reportUnknownMemberType=false, reportUnknownVariableType=false, reportUnnecessaryIsInstance=false | ||
|
||
|
||
OldKV = t.Tuple[OKT[KT], OVT[VT]] | ||
DedupResult = t.Optional[OldKV[KT, VT]] | ||
Write = t.List[t.Callable[[], None]] | ||
|
@@ -184,7 +188,8 @@ def inverse(self) -> 'BidictBase[VT, KT]': | |
self._invweak: 't.Optional[weakref.ReferenceType[BidictBase[VT, KT]]]' = None | ||
# Also store a weak reference back to `instance` on its inverse instance, so that | ||
# the second `.inverse` access in `bi.inverse.inverse` hits the cached weakref. | ||
inv._inv, inv._invweak = None, weakref.ref(self) | ||
inv._inv = None | ||
inv._invweak = weakref.ref(self) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The code before this change is semantically equivalent to the code after this change. But before this change, pyright (but not mypy) flagged this line, as follows: /Users/jab/src/bidict/bidict/_base.py:187:23 - error: Cannot assign member "_invweak" for type "BidictBase[VT@BidictBase, KT@BidictBase]"
Expression of type "ref" cannot be assigned to member "_invweak" of class "BidictBase[VT@BidictBase, KT@BidictBase]"
Type "ref" cannot be assigned to type "ReferenceType[BidictBase[KT@BidictBase, VT@BidictBase]] | None"
TypeVar "_T@ReferenceType" is invariant
Type "Self@BidictBase[KT@BidictBase, VT@BidictBase]" cannot be assigned to type "BidictBase[KT@BidictBase, VT@BidictBase]"
Type cannot be assigned to type "None" (reportGeneralTypeIssues) Questions:
I'm willing to make this change just to appease pyright, but it has two minor drawbacks:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Are you sure? That's the opposite of what I'd expect, because with the first variant you shouldn't pay the cost of creating an intermediate tuple. I tried some quick benchmarks and also found that the separate assignments are faster. It's a pretty marginal difference, though, so I wouldn't make a decision based on performance unless it's highly performance critical code. As for the pyright error, I suspect it's a limitation in pyright's bidirectional type inference. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It's the opposite of what I'd expect too, but when I saw this as
For sure. (When I said "minor", this is what I should have said.:) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
(Sorry for going off on this tangent, but now I'm interested :) ) I tried code like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is intended behavior in pyright, and it is related to bidirectional type inference. Pyright applies bidirectional type inference for assignments if the LHS expression has a declared type. The expression The other problem here is that you have declared The problem is that the I don't know why mypy accepts the assignment in this case without emitting an error. I don't think it technically should, but I guess it's a gray area. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the tip about the Self type, I hadn't seen that! I just tested it though and it doesn't currently work with generics. At least, if you try to do something like the following, you get an error about Self not supporting indexing: class BidictBase(Mapping[KT, VT]):
def inverse(self) -> Self[VT, KT]:
... Without that, I'm not sure how useful Self would be here, even if bidict did only have to support Python 3.11+. |
||
# In e.g. `bidict().inverse.inverse`, this design ensures that a strong reference | ||
# back to the original instance is retained before its refcount drops to zero, | ||
# avoiding an unintended potential deallocation. | ||
|
@@ -487,7 +492,7 @@ def _init_from(self, other: MapOrIterItems[KT, VT]) -> None: | |
# If other is a bidict, use its existing backing inverse mapping, otherwise | ||
# other could be a generator that's now exhausted, so invert self._fwdm on the fly. | ||
inv = other.inverse if isinstance(other, BidictBase) else inverted(self._fwdm) | ||
self._invm.update(inv) | ||
self._invm.update(inv) # pyright: ignore | ||
|
||
#: Used for the copy protocol. | ||
#: *See also* the :mod:`copy` module | ||
|
@@ -528,7 +533,7 @@ def __reduce__(self) -> t.Tuple[t.Any, ...]: | |
# somewhere in sys.modules that pickle can discover. | ||
should_invert = isinstance(self, GeneratedBidictInverse) | ||
cls, init_from = (self._inv_cls, self.inverse) if should_invert else (self.__class__, self) | ||
return self._from_other, (cls, dict(init_from), should_invert) # type: ignore [call-overload] # https://github.com/python/mypy/issues/4975 | ||
return self._from_other, (cls, dict(init_from), should_invert) # type: ignore [call-overload] | ||
|
||
|
||
# See BidictBase._set_reversed() above. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -105,12 +105,10 @@ def clear(self) -> None: | |
self._fwdm.clear() | ||
self._invm.clear() | ||
|
||
@t.overload | ||
def pop(self, __key: KT, __default: DT) -> t.Union[VT, DT]: ... | ||
@t.overload | ||
@t.overload # type: ignore [override] | ||
def pop(self, __key: KT) -> VT: ... | ||
@t.overload | ||
def pop(self, __key: KT, __default: t.Union[VT, DT] = ...) -> t.Union[VT, DT]: ... | ||
def pop(self, __key: KT, __default: DT) -> t.Union[VT, DT]: ... | ||
|
||
def pop(self, key: KT, default: ODT[DT] = MISSING) -> t.Union[VT, DT]: | ||
"""*x.pop(k[, d]) → v* | ||
|
@@ -137,7 +135,7 @@ def popitem(self) -> t.Tuple[KT, VT]: | |
del self._invm[val] | ||
return key, val | ||
|
||
@t.overload | ||
@t.overload # type: ignore [override] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just noticed a new version of mypy was released 2 days ago, so upgraded it in this PR in case it would reduce the number of places where mypy and pyright are disagreeing. But it ended up adding a new place they're disagreeing, namely this line: without this 'type: ignore' comment, mypy errors as follows: bidict/_bidict.py: note: In member "update" of class "MutableBidict":
bidict/_bidict.py:138:6: error: Signature of "update" incompatible with
supertype "MutableMapping" [override]
@t.overload
^
bidict/_bidict.py:138:6: note: Superclass:
bidict/_bidict.py:138:6: note: @overload
bidict/_bidict.py:138:6: note: def update(self, SupportsKeysAndGetItem[KT, VT], **kwargs: VT) -> None
bidict/_bidict.py:138:6: note: @overload
bidict/_bidict.py:138:6: note: def update(self, Iterable[Tuple[KT, VT]], **kwargs: VT) -> None
bidict/_bidict.py:138:6: note: @overload
bidict/_bidict.py:138:6: note: def update(self, **kwargs: VT) -> None
bidict/_bidict.py:138:6: note: Subclass:
bidict/_bidict.py:138:6: note: @overload
bidict/_bidict.py:138:6: note: def update(self, Mapping[KT, VT], **kw: VT) -> None
bidict/_bidict.py:138:6: note: @overload
bidict/_bidict.py:138:6: note: def update(self, Iterable[Tuple[KT, VT]], **kw: VT) -> None
bidict/_bidict.py:138:6: note: @overload
bidict/_bidict.py:138:6: note: def update(self, **kw: VT) -> None
Found 1 error in 1 file (checked 14 source files) (This is the same "override" error we're seeing in #242 (comment), which pyright was similarly not flagging, though that case still looks like a false positive bug in mypy. In this case, it looks like mypy is correct and pyright is giving a false negative: Strange that mypy only just started flagging this in v0.940, and was not flagging it in v0.931, given that this change was introduced in typeshed in August 2020: python/typeshed@c065982 In any case, I don't think it's worth changing my code to use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For completeness, here's a sketch of the kind of changes that would be required to support non-Mapping show/hide diffdiff --git a/bidict/_typing.py b/bidict/_typing.py
index 8e3ec60..315b225 100644
--- a/bidict/_typing.py
+++ b/bidict/_typing.py
@@ -10,11 +10,17 @@
import typing as t
from enum import Enum
+if t.TYPE_CHECKING:
+ from _typeshed import SupportsKeysAndGetItem
+else:
+ SupportsKeysAndGetItem = t.Mapping
+
KT = t.TypeVar('KT')
VT = t.TypeVar('VT')
IterItems = t.Iterable[t.Tuple[KT, VT]]
MapOrIterItems = t.Union[t.Mapping[KT, VT], IterItems[KT, VT]]
+MapLikeOrIterItems = t.Union[SupportsKeysAndGetItem[KT, VT], MapOrIterItems[KT, VT]]
diff --git a/bidict/_iter.py b/bidict/_iter.py
index 1e7dd5a..957db83 100644
--- a/bidict/_iter.py
+++ b/bidict/_iter.py
@@ -10,15 +10,21 @@
from operator import itemgetter
import typing as t
-from ._typing import KT, VT, IterItems, MapOrIterItems
+from ._typing import KT, VT, IterItems, MapLikeOrIterItems
-def iteritems_mapping_or_iterable(arg: MapOrIterItems[KT, VT]) -> IterItems[KT, VT]:
+def iteritems_mapping_or_iterable(arg: MapLikeOrIterItems[KT, VT]) -> IterItems[KT, VT]:
"""Yield the items in *arg* based on whether it's a mapping."""
- yield from arg.items() if isinstance(arg, t.Mapping) else arg # pyright: ignore
+ if isinstance(arg, t.Mapping):
+ items = arg.items() # pyright: ignore
+ elif hasattr(arg, 'keys'):
+ items = ((k, arg[k]) for k in arg.keys()) # type: ignore
+ else:
+ items = arg # type: ignore
+ return iter(items) # pyright: ignore
diff --git a/bidict/_bidict.py b/bidict/_bidict.py
index f713f0b..a18341e 100644
--- a/bidict/_bidict.py
+++ b/bidict/_bidict.py
@@ -19,7 +19,7 @@ import typing as t
from ._abc import MutableBidirectionalMapping
from ._base import BidictBase, get_arg
from ._dup import OnDup, ON_DUP_RAISE, ON_DUP_DROP_OLD
-from ._typing import KT, VT, DT, ODT, MISSING, IterItems, MapOrIterItems
+from ._typing import KT, VT, DT, ODT, MISSING, IterItems, MapLikeOrIterItems, SupportsKeysAndGetItem
class MutableBidict(BidictBase[KT, VT], MutableBidirectionalMapping[KT, VT]):
@@ -135,26 +135,26 @@ class MutableBidict(BidictBase[KT, VT], MutableBidirectionalMapping[KT, VT]):
del self._invm[val]
return key, val
- @t.overload # type: ignore [override]
- def update(self, __m: t.Mapping[KT, VT], **kw: VT) -> None: ...
+ @t.overload
+ def update(self, __m: SupportsKeysAndGetItem[KT, VT], **kw: VT) -> None: ...
@t.overload
def update(self, __i: IterItems[KT, VT], **kw: VT) -> None: ...
@t.overload
def update(self, **kw: VT) -> None: ...
- def update(self, *args: MapOrIterItems[KT, VT], **kw: VT) -> None:
+ def update(self, *args: MapLikeOrIterItems[KT, VT], **kw: VT) -> None:
"""Like calling :meth:`putall` with *self.on_dup* passed for *on_dup*."""
if args or kw:
self._update(get_arg(*args), kw) |
||
def update(self, __m: t.Mapping[KT, VT], **kw: VT) -> None: ... | ||
@t.overload | ||
def update(self, __i: IterItems[KT, VT], **kw: VT) -> None: ... | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -98,7 +98,7 @@ def new_last_node(self) -> Node: | |
"""Create and return a new terminal node.""" | ||
old_last = self.prv | ||
new_last = Node(old_last, self) | ||
old_last.nxt = self.prv = new_last | ||
old_last.nxt = self.prv = new_last # pyright: ignore # https://github.com/microsoft/pyright/issues/3172 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This bug appears to not be fully fixed in pyright 1.1.229. Without this suppression comment, pyright 1.1.229 still flags this line with: /Users/jab/src/bidict/bidict/_orderedbase.py:101:29 - error: "prv" is not specified in __slots__ (reportGeneralTypeIssues) I left a comment on the associated issue. In the meantime, great to now be able to use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (hit "enter" too soon) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (For completeness, the correct link to the issue tracking this bug is microsoft/pyright#3183) |
||
return new_last | ||
|
||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
{ | ||
"include": [ | ||
"bidict", | ||
], | ||
"exclude": [ | ||
"docs", | ||
"tests", | ||
"run_tests.py", | ||
], | ||
"strict": [ | ||
"bidict", | ||
], | ||
"reportUnnecessaryTypeIgnoreComment": "warning", | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,5 @@ | ||
pre-commit | ||
pyright | ||
pylint | ||
# pylint runs against tests/*.py which import pytest and hypothesis | ||
pytest | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More just noting for the record than to take away from more important discussion:
I tried following the docs at https://github.com/microsoft/pyright/blob/main/docs/ci-integration.md#running-pyright-as-a-pre-commit-hook but found:
pass_filenames: false
, or else node would just hangadditional_dependencies: ['[email protected]']
because it would duplicate the pyright version fromrequirements/lint.txt
, and thelocal
environment gets set up with therequirements/lint.txt
dependencies already