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

Use Pyright in CI #242

Merged
merged 9 commits into from
Mar 20, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .devcontainer/devcontainer.json
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@
},
// Set *default* container specific settings.json values on container create.
"settings": {
// Enable pylance's pyright integration (disabled by default):
"python.analysis.typeCheckingMode": "basic",
"python.defaultInterpreterPath": "/usr/local/bin/python",
"python.linting.enabled": true,
"python.linting.mypyEnabled": true,
Expand Down
13 changes: 9 additions & 4 deletions .pre-commit-config.yaml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
repos:
- repo: https://github.com/pre-commit/pre-commit-hooks
rev: v4.0.1
rev: v4.1.0
hooks:
- id: trailing-whitespace
- id: end-of-file-fixer
Expand All @@ -16,7 +16,7 @@ repos:
- id: codespell

- repo: https://github.com/pre-commit/mirrors-mypy
rev: v0.931
rev: v0.941
hooks:
- id: mypy
exclude: setup.py|docs/conf.py|tests
Expand All @@ -28,13 +28,18 @@ repos:
exclude: setup.py|docs/conf.py|tests

- repo: https://github.com/pycqa/flake8
rev: 3.9.2
rev: 4.0.1
hooks:
- id: flake8

# https://pre-commit.com/#repository-local-hooks
- repo: local
hooks:
- id: pyright
name: pyright
entry: pyright
language: node
types: [python]
- id: pylint
name: pylint
entry: pylint
Expand All @@ -43,7 +48,7 @@ repos:
exclude: setup.py|run_tests.py|docs/conf.py
Copy link
Owner Author

@jab jab Mar 10, 2022

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:

  • I had to remove pass_filenames: false, or else node would just hang
  • I removed additional_dependencies: ['[email protected]'] because it would duplicate the pyright version from requirements/lint.txt, and the local environment gets set up with the requirements/lint.txt dependencies already


- repo: https://github.com/Lucas-C/pre-commit-hooks
rev: v1.1.10
rev: v1.1.13
hooks:
- id: forbid-crlf
- id: remove-crlf
Expand Down
14 changes: 11 additions & 3 deletions bidict/_base.py
Original file line number Diff line number Diff line change
Expand Up @@ -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]]
Expand Down Expand Up @@ -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."""
Expand All @@ -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)
Copy link
Owner Author

Choose a reason for hiding this comment

The 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:

  1. Is this a bug in pyright? (This is with the latest version, 1.1.228.)
  2. Is there a better way to appease pyright here?

I'm willing to make this change just to appease pyright, but it has two minor drawbacks:

  • In general, x = 1; y = 2; z = 3 # ... is slower than x, y, z = 1, 2, 3
  • In cases like this, where the code is just carrying out exactly what the comment before it explains (and which is the inverse of the two (more verbosely-written) assignments that precede it), the single-assignment version that was previously written is slightly preferable, since it more quickly gets to the point that the comment just explained (and more clearly implements the comment in its entirety).

Choose a reason for hiding this comment

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

In general, x = 1; y = 2; z = 3 # ... is slower than x, y, z = 1, 2, 3

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.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Are you sure? That's the opposite of what I'd expect...

It's the opposite of what I'd expect too, but when I saw this as #11 in https://youtu.be/YjHsOrOOSuI?t=1216 and reproduced it, it stuck with me ever since. Interesting you're not reproducing it now! I wonder if this changed in more recent versions of Python.

I wouldn't make a decision based on performance unless it's highly performance critical code.

For sure. (When I said "minor", this is what I should have said.:)

Choose a reason for hiding this comment

The 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 #11 in https://youtu.be/YjHsOrOOSuI?t=1216 and reproduced it, it stuck with me ever since. Interesting you're not reproducing it now! I wonder if this changed in more recent versions of Python.

(Sorry for going off on this tangent, but now I'm interested :) )

I tried code like def f(): x1, x2 = 1, 2; return x1, x2 for different numbers of variables (all on 3.6). Unpacking is faster at n = 10, but slower at n = 3 and n = 100. I think the issue is that (1, 2) gets turned into a single constant by the compiler, and at n = 10 that apparently makes the whole thing cheaper than repeated assignment would be. But at n = 100 the compiler gives up on generating a single constant tuple and builds it at runtime instead, so repeated assignment is faster again. But in real code you're probably not just assigning a tuple of constant ints, so it's pretty unlikely that you'd run into a situation where it's faster in practice.

Copy link

@erictraut erictraut Mar 10, 2022

Choose a reason for hiding this comment

The 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 inv._invweak does have a declared type, but the expression inv._inv, inv._invweak does not. So bidirectional type inference is not applied when you use tuple packing/unpacking.

The other problem here is that you have declared self._invweak to have the type Optional[weakref.ReferenceType[BidictBase[VT, KT]]]', but really it should be declared as Optional[weakref.ReferenceType[Self]]`. If it were declared as such, then this problem wouldn't exist because the types on the LHS and RHS of the assignment would already match even without the use of bidirectional type inference.

The problem is that the Self type was added only recently (for Python 3.11). It is supported in typing_extensions and in pyright, but I don't think mypy has support for it yet.

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.

Copy link
Owner Author

Choose a reason for hiding this comment

The 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.
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand Down
8 changes: 3 additions & 5 deletions bidict/_bidict.py
Original file line number Diff line number Diff line change
Expand Up @@ -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] # https://github.com/python/mypy/issues/12390
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*
Expand All @@ -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] # https://github.com/jab/bidict/pull/242#discussion_r825464731
def update(self, __m: t.Mapping[KT, VT], **kw: VT) -> None: ...
@t.overload
def update(self, __i: IterItems[KT, VT], **kw: VT) -> None: ...
Expand Down
2 changes: 1 addition & 1 deletion bidict/_frozenbidict.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ class frozenbidict(BidictBase[KT, VT]):

_hash: int

# Work around lack of support for higher-kinded types in mypy.
# Work around lack of support for higher-kinded types in Python.
# Ref: https://github.com/python/typing/issues/548#issuecomment-621571821
if t.TYPE_CHECKING:
@property
Expand Down
2 changes: 1 addition & 1 deletion bidict/_frozenordered.py
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ class FrozenOrderedBidict(OrderedBidictBase[KT, VT]):
the ordering of the items can make the ordering dependence more explicit.
"""

__hash__: t.Callable[[t.Any], int] = frozenbidict.__hash__
__hash__: t.Callable[[t.Any], int] = frozenbidict.__hash__ # pyright: ignore

if t.TYPE_CHECKING:
@property
Expand Down
4 changes: 2 additions & 2 deletions bidict/_iter.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,15 +7,15 @@

"""Functions for iterating over items in a mapping."""

from collections.abc import Mapping
from operator import itemgetter
import typing as t

from ._typing import KT, VT, IterItems, MapOrIterItems


def iteritems_mapping_or_iterable(arg: MapOrIterItems[KT, VT]) -> IterItems[KT, VT]:
"""Yield the items in *arg* based on whether it's a mapping."""
yield from arg.items() if isinstance(arg, Mapping) else arg
yield from arg.items() if isinstance(arg, t.Mapping) else arg # pyright: ignore


def iteritems(__arg: MapOrIterItems[KT, VT], **kw: VT) -> IterItems[KT, VT]:
Expand Down
5 changes: 4 additions & 1 deletion bidict/_named.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@
from ._typing import KT, VT


# pyright: reportPrivateUsage=false, reportUnnecessaryIsInstance=false


class NamedBidictBase:
"""Base class that namedbidicts derive from."""

Expand Down Expand Up @@ -93,4 +96,4 @@ def _inv_cls_dict_diff(cls) -> t.Dict[str, t.Any]:
NamedInv.__doc__ = f'NamedBidictInv({basename}) {typename!r}: {valname} -> {keyname}'
caller_module = _getframe(1).f_globals.get('__name__', '__main__')
NamedBidict.__module__ = NamedInv.__module__ = caller_module
return NamedBidict
return NamedBidict # pyright: ignore [reportUnknownVariableType]
2 changes: 1 addition & 1 deletion bidict/_orderedbase.py
Original file line number Diff line number Diff line change
Expand Up @@ -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/3183
return new_last


Expand Down
5 changes: 4 additions & 1 deletion bidict/_orderedbidict.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@
from ._typing import KT, VT


# pyright: reportPrivateUsage=false


class OrderedBidict(OrderedBidictBase[KT, VT], MutableBidict[KT, VT]):
"""Mutable bidict type that maintains items in insertion order."""

Expand Down Expand Up @@ -57,7 +60,7 @@ def popitem(self, last: bool = True) -> t.Tuple[KT, VT]:
korv = self._node_by_korv.inverse[node]
if self._bykey:
return korv, self._pop(korv)
return self.inverse._pop(korv), korv
return self.inverse._pop(korv), korv # pyright: ignore [reportGeneralTypeIssues]

def move_to_end(self, key: KT, last: bool = True) -> None:
"""Move the item with the given key to the end if *last* is true, else to the beginning.
Expand Down
14 changes: 14 additions & 0 deletions pyrightconfig.json
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",
}
6 changes: 5 additions & 1 deletion requirements/dev.txt
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,9 @@ markupsafe==2.1.0
mccabe==0.6.1
# via pylint
nodeenv==1.6.0
# via pre-commit
# via
# pre-commit
# pyright
packaging==21.3
# via
# build
Expand Down Expand Up @@ -113,6 +115,8 @@ pylint==2.12.2
# via -r lint.in
pyparsing==3.0.7
# via packaging
pyright==1.1.231
# via -r lint.in
pytest==7.0.1
# via
# -r lint.in
Expand Down
3 changes: 3 additions & 0 deletions requirements/lint.in
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
pre-commit
# pre-commit would ideally be the only direct dependency needed for linting, but the
# following pre-commit hooks require "repo: local", so we add them to our dependencies:
pyright
pylint
# pylint runs against tests/*.py which import pytest and hypothesis
pytest
Expand Down
6 changes: 5 additions & 1 deletion requirements/lint.txt
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,9 @@ lazy-object-proxy==1.7.1
mccabe==0.6.1
# via pylint
nodeenv==1.6.0
# via pre-commit
# via
# pre-commit
# pyright
packaging==21.3
# via pytest
platformdirs==2.5.1
Expand All @@ -46,6 +48,8 @@ pylint==2.12.2
# via -r lint.in
pyparsing==3.0.7
# via packaging
pyright==1.1.231
# via -r lint.in
pytest==7.0.1
# via -r lint.in
pyyaml==6.0
Expand Down