-
-
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 all 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 pyright strict diagnostics that are causing many false positives or are just 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]] | ||
|
@@ -158,6 +162,9 @@ def __init__(self, *args: MapOrIterItems[KT, VT], **kw: VT) -> None: | |
if args or kw: | ||
self._update(get_arg(*args), kw, rbof=False) | ||
|
||
# If Python ever adds support for higher-kinded types, `inverse` could use them, e.g. | ||
# def inverse(self: BT[KT, VT]) -> BT[VT, KT]: | ||
# Ref: https://github.com/python/typing/issues/548#issuecomment-621571821 | ||
@property | ||
def inverse(self) -> 'BidictBase[VT, KT]': | ||
"""The inverse of this bidirectional mapping instance.""" | ||
|
@@ -184,7 +191,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 +495,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 # https://github.com/jab/bidict/pull/242#discussion_r824223403 | ||
|
||
#: Used for the copy protocol. | ||
#: *See also* the :mod:`copy` module | ||
|
@@ -528,7 +536,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 |
---|---|---|
@@ -0,0 +1,14 @@ | ||
{ | ||
"include": [ | ||
"bidict", | ||
], | ||
"exclude": [ | ||
"docs", | ||
"tests", | ||
"run_tests.py", | ||
], | ||
"strict": [ | ||
"bidict", | ||
], | ||
"reportUnnecessaryTypeIgnoreComment": "warning", | ||
} |
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