Use Pyright in CI #242

jab · 2022-03-10T15:18:13Z

...in addition to mypy. Double the type checkers, double the fun?

gitpod-io · 2022-03-10T15:18:19Z

codecov · 2022-03-10T15:21:55Z

Codecov Report

Merging #242 (fb9d971) into main (62bc061) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main      #242   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           14        14           
  Lines          623       624    +1     
  Branches        99        99           
=========================================
+ Hits           623       624    +1

Impacted Files	Coverage Δ
bidict/_bidict.py	`100.00% <ø> (ø)`
bidict/_frozenbidict.py	`100.00% <ø> (ø)`
bidict/_base.py	`100.00% <100.00%> (ø)`
bidict/_frozenordered.py	`100.00% <100.00%> (ø)`
bidict/_iter.py	`100.00% <100.00%> (ø)`
bidict/_named.py	`100.00% <100.00%> (ø)`
bidict/_orderedbase.py	`100.00% <100.00%> (ø)`
bidict/_orderedbidict.py	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 62bc061...fb9d971. Read the comment docs.

jab

Please see inline for questions and comments arising from trying pyright with bidict.

I've indicated several places where code changes were necessary to appease pyright, but where (the latest stable release of) mypy in strict mode was not complaining.

I've also indicated several places where I could not thus far come up with a reasonable way to appease pyright without also making mypy fail.

Looking forward to incorporating pyright into bidict's CI once there's an acceptable way to get past these issues and have pyright work in harmony with mypy.

jab · 2022-03-10T15:29:15Z

.pre-commit-config.yaml

+        entry: pyright
+        language: node
+        types: [python]
+        exclude: setup.py|run_tests.py|docs/conf.py


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

jab · 2022-03-10T15:41:24Z

bidict/_base.py

@@ -184,7 +184,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)


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:

Is this a bug in pyright? (This is with the latest version, 1.1.228.)

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).

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.

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.:)

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.

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.

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+.

bidict/_base.py

jab · 2022-03-10T16:34:01Z

bidict/_base.py

        new = self.copy()
        new._update(other, rbof=False)
        return new

    def __ror__(self: BT, other: t.Mapping[KT, VT]) -> BT:
        """Return other|self."""
        if not isinstance(other, t.Mapping):
-            return NotImplemented
+            return NotImplemented  # flagged by pyright


pyright (but not mypy) is flagging this line and the similar line above it as follows:

/Users/jab/src/bidict/bidict/_base.py:500:20 - error: Expression of type "_NotImplementedType" cannot be assigned to return type "BT@__or__" Type "_NotImplementedType" cannot be assigned to type "BT@__or__" (reportGeneralTypeIssues) /Users/jab/src/bidict/bidict/_base.py:508:20 - error: Expression of type "_NotImplementedType" cannot be assigned to return type "BT@__ror__" Type "_NotImplementedType" cannot be assigned to type "BT@__ror__" (reportGeneralTypeIssues)

But earlier in this file, we have:

def __eq__(self, other: object) -> bool: ... return NotImplemented

...and pyright has no problem with the return NotImplemented there, even though the return type of the method is declared as bool.

Is this a bug in pyright?

What is the best way to appease pyright?

Changing the return type of the method from BT to Union[BT, NotImplementedType] seems infeasible:

Importing types.NotImplementedType requires Python 3.10+, and bidict must support 3.7+.

Defining my own NotImplementedType = type(NotImplemented) and using that in a type hint causes mypy (but not pyright) to fail with Variable "...NotImplementedType" is not valid as a type.

I can't use typing.TypeAlias because that also requires Python 3.10+.

Introducing a dependency on typing_extensions just for this would be overkill.

Adding a # type: ignore comment to these lines gets pyright to back off, but causes mypy to fail with error: Unused "type: ignore" comment.

It's valuable to have mypy complain about unused "type: ignore" comments. That way, once fixes for extant bugs like Add missing type hint for AbstractSet._hash() python/typeshed#7153 make it into a release I can use, I can count on mypy to remind me to remove my associated type: ignore comments, as in

bidict/bidict/_frozenbidict.py

Line 38 in 62bc061

self._hash = t.ItemsView(self)._hash() # type: ignore [attr-defined] # https://github.com/python/typeshed/pull/7153

.

In light of the above, it seems like the fairest "fix" would be allowing me to do something like:

return NotImplemented # pyright: ignore # [maybe link to associated bug]

But this was rejected in microsoft/pyright#3143.

This seems like a pyright bug to me. I suspect there's special handling for NotImplemented but it doesn't handle the interaction with TypeVars.

Another workaround is cast(Any, NotImplemented).

Indeed, thanks, @JelleZijlstra! (Much appreciate your help with this PR, didn't even realize you'd notice it!)

FWIW, I think that using something like # pyright: ignore [link to bug] is preferable to cast(Any, NotImplemented) here, because:

It makes it clear that this is only necessary to work around a bug in pyright

It clearly does not affect how mypy is interpreting this code

NotImplemented is defined in a strange way in typeshed. The definition (which I've pasted below) alludes to the fact that "it makes the error message better". I presume this is referring to the error message emitted by mypy. It's probably not a good idea to work around mypy-specific error handling by adding hacks like this in typeshed. But I digress.

@final class _NotImplementedType(Any): # type: ignore[misc] # A little weird, but typing the __call__ as NotImplemented makes the error message # for NotImplemented() much better __call__: NotImplemented # type: ignore[valid-type] NotImplemented: _NotImplementedType

As Jelle conjectured, the reason you're seeing different behavior between __eq__ and __ror__ is because the former returns bool and the latter returns a TypeVar. Pyright is treating those two inconsistently. _NotImplementedType inherits from Any, so it should be allowed in the TypeVar case.

I've filed the following pyright bug to track this: microsoft/pyright#3171

Your take on typeshed here makes sense to me. I hope typeshed can be improved in this regard.

Thanks for following up on this with microsoft/pyright#3171! I'll keep on eye on that issue. In the meantime, does this help make the case for supporting something like

return NotImplemented # pyright: ignore # see microsoft/pyright#3171

?

It seems like this would provide users hitting this or any future bugs with a more forgiving and maintainable escape hatch. (Especially if, once the bug were fixed, the next version of pyright could emit some kind of unused "pyright: ignore" error à la mypy.)

It looks like I can add warn_unused_ignores = False (which gets implicitly set to True by the strict = True at the beginning), to the end of my mypy.ini, to silence all mypy output about any unused type: ignore comments, without having to give up any of the other desired checks that strict = True enables.

Minimized example:

❯ cat test.py x = 1 + 1 # type: ignore ❯ mypy test.py Success: no issues found in 1 source file ❯ mypy --strict test.py # exits nonzero test.py:1: error: Unused "type: ignore" comment Found 1 error in 1 file (checked 1 source file) ❯ mypy --strict --no-warn-unused-ignores test.py # exits zero Success: no issues found in 1 source file

I don't see any way to run mypy in strict mode and have it warn-but-not-error on unused type: ignores (in other words, still exit zero, but also still output a warning). @JelleZijlstra, am I missing something? If not, shouldn't that be supported?

As https://mypy.readthedocs.io/en/stable/command_line.html#cmdoption-mypy-warn-unused-ignores says:

This flag, along with the --warn-redundant-casts flag, are both particularly useful when you are upgrading mypy. Previously, you may have needed to add casts or # type: ignore annotations to work around bugs in mypy or missing stubs for 3rd party libraries.

These two flags let you discover cases where either workarounds are no longer necessary.

Quite validating to see this in the docs (hadn't noticed it before), as it matches my exact experience.

So to summarize all my current suggestions:

Both mypy and pyright should support a mode where they complain in their output about any inline comments intended to suppress an error that are not actually suppressing an error.

It should be possible to configure whether this results in an error (nonzero exit status), a warning (exit zero, but still complain in the output), or total silence (exit zero, no mention in the output).

Both mypy and pyright should support targeted inline comments that allow users to unambiguously suppress an error that only one of them is emitting (possibly erroneously due to a bug), without affecting the behavior of the other one.

After upgrading to pyright 1.1.229, pyright is no longer giving false positives here. Nice.

I'll leave this thread unresolved in case @JelleZijlstra can give thoughts on adding the following to mypy (and link any newly-created issues if interested):

support for warning-but-not-erroring on unused suppression comments

support for "mypy: ignore" (and not just "type: ignore") suppression comments

If this were available, I could replace all my "type: ignore" comments with "mypy: ignore" comments in the case that only mypy and not pyright is flagging the associated lines (which I can easily see thanks to pyright's reportUnnecessaryTypeIgnoreComment=warning).

Then for each "mypy: ignore" suppression, decide whether it's a pyright false negative or a mypy false positive, and ensure there is a corresponding open bug I can link to.

Finally, that will leave all remaining "type: ignore" comments indicating lines that pyright, mypy, and I all agree should be type errors, but where turning off the suppression would require typing features I can't use yet, or would make the code less maintainable, or where I just need to move on for now and come back to the particular line when I have time.

I see the use case, but I think mypy doesn't currently have a "warning" output level, and mypy already has a fiendishly complicated set of configuration options.

Fair enough!

That was the first bullet. What about the second (supporting “mypy: ignore”)?

I opened python/mypy#12359 and python/mypy#12358 to track these proposals properly so they're not buried here.

jab · 2022-03-10T16:35:31Z

bidict/_bidict.py

@@ -110,7 +110,7 @@ def pop(self, __key: KT, __default: DT) -> t.Union[VT, DT]: ...
    @t.overload
    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: t.Any = ...) -> t.Any: ...


Without this change, pyright (but not mypy) flags this code with:

/Users/jab/src/bidict/bidict/_bidict.py:109:9 - error: Overload 1 for "pop" overlaps overload 3 and returns an incompatible type (reportOverlappingOverload) /Users/jab/src/bidict/bidict/_bidict.py:115:9 - error: Overloaded function implementation is not consistent with signature of overload 3 Function return type "VT@MutableBidict | DT@pop" is incompatible with type "VT@MutableBidict" Type "VT@MutableBidict | DT@pop" cannot be assigned to type "VT@MutableBidict" (reportGeneralTypeIssues)

Is this a bug in pyright and/or mypy?

Is there a better way to appease pyright here?

Your overload is a bit squirrelly here. I think pyright is correct in pointing out the incompatible overlap. I'm not sure why mypy doesn't flag it in this case.

If I correctly understand the intent of this overload, I think this would be a better solution:

@t.overload def pop(self, __key: KT, __default: t.Literal[MISSING]) -> VT: ... @t.overload def pop(self, __key: KT, __default: DT) -> t.Union[VT, DT]: ... @t.overload def pop(self, __key: KT) -> VT: ...

Thanks, @erictraut, your proposed type hints indeed appease pyright, but now mypy is rejecting them:

bidict/_bidict.py: note: In member "pop" of class "MutableBidict": bidict/_bidict.py:108:6: error: Signature of "pop" incompatible with supertype "MutableMapping" [override] @t.overload ^ bidict/_bidict.py:108:6: note: Superclass: bidict/_bidict.py:108:6: note: @overload bidict/_bidict.py:108:6: note: def pop(self, KT) -> VT bidict/_bidict.py:108:6: note: @overload bidict/_bidict.py:108:6: note: def [_T] pop(self, KT, Union[VT, _T] = ...) -> Union[VT, _T] bidict/_bidict.py:108:6: note: Subclass: bidict/_bidict.py:108:6: note: @overload bidict/_bidict.py:108:6: note: def pop(self, KT, Any) -> VT bidict/_bidict.py:108:6: note: @overload bidict/_bidict.py:108:6: note: def [DT] pop(self, KT, DT) -> Union[VT, DT] bidict/_bidict.py:108:6: note: @overload bidict/_bidict.py:108:6: note: def pop(self, KT) -> VT bidict/_bidict.py: note: In function "pop": bidict/_bidict.py:109: error: Parameter 1 of Literal[...] is invalid [misc] def pop(self, __key: KT, __default: 't.Literal[MISSING]') -> VT: ... ^ bidict/_bidict.py:109:41: error: Variable "bidict._typing.MISSING" is not valid as a type [valid-type] def pop(self, __key: KT, __default: 't.Literal[MISSING]') -> VT: ... ^ bidict/_bidict.py:109:41: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#variables-vs-type-aliases

Also, Literal requires Python 3.8+, and TypeAlias requires 3.10+. Bidict currently supports 3.7+, and it'd be nice to avoid taking a dependency on typing_extensions.

Any other suggestions?

You can change the first overload to accept __default: MissingT.

I meant to say, I tried that too, and again, it made pyright happy but made mypy start failing, as follows:

@t.overload def pop(self, __key: KT) -> VT: ... @t.overload def pop(self, __key: KT, __default: MissingT) -> VT: ... @t.overload def pop(self, __key: KT, __default: DT) -> t.Union[VT, DT]: ... def pop(self, key: KT, default: ODT[DT] = MISSING) -> t.Union[VT, DT]: ...

bidict/_bidict.py: note: In member "pop" of class "MutableBidict": bidict/_bidict.py:108:6: error: Signature of "pop" incompatible with supertype "MutableMapping" [override] @t.overload ^ bidict/_bidict.py:108:6: note: Superclass: bidict/_bidict.py:108:6: note: @overload bidict/_bidict.py:108:6: note: def pop(self, KT) -> VT bidict/_bidict.py:108:6: note: @overload bidict/_bidict.py:108:6: note: def [_T] pop(self, KT, Union[VT, _T] = ...) -> Union[VT, _T] bidict/_bidict.py:108:6: note: Subclass: bidict/_bidict.py:108:6: note: @overload bidict/_bidict.py:108:6: note: def pop(self, KT) -> VT bidict/_bidict.py:108:6: note: @overload bidict/_bidict.py:108:6: note: def pop(self, KT, MissingT) -> VT bidict/_bidict.py:108:6: note: @overload bidict/_bidict.py:108:6: note: def [DT] pop(self, KT, DT) -> Union[VT, DT]

Does this indicate a bug somewhere between typeshed and/or pyright and/or mypy?

I think mypy is right there. The superclass allows you to pass an unrelated type as the second argument, but the subclass doesn't.

Pyright doesn't seem to handle overrides involving overloads correctly. I'll report a bug.

On line 111 of the latest revision, the subclass’s overload does allow you to pass an unrelated type:

def pop(self, __key: KT, __default: DT) -> t.Union[VT, DT]: ...

No?

Do you still get the same mypy error from that? I was going off the error in your previous message.

Yes I do. Sorry that wasn’t clear. (You can see there is still a “type: ignore [override]” comment, which would cause mypy to fail if it were unused, but the mypy check in CI is passing for the latest revision.)

The latest type hints:

@t.overload # temporarily remove 'type: ignore' comment from this line def pop(self, __key: KT) -> VT: ... @t.overload def pop(self, __key: KT, __default: DT) -> t.Union[VT, DT]: ... def pop(self, key: KT, default: ODT[DT] = MISSING) -> t.Union[VT, DT]: ...

The mypy output (with the 'type: ignore' removed as per above):

❯ mypy bidict/_bidict.py bidict/_bidict.py: note: In member "pop" of class "MutableBidict": bidict/_bidict.py:108:6: error: Signature of "pop" incompatible with supertype "MutableMapping" [override] @t.overload ^ bidict/_bidict.py:108:6: note: Superclass: bidict/_bidict.py:108:6: note: @overload bidict/_bidict.py:108:6: note: def pop(self, KT) -> VT bidict/_bidict.py:108:6: note: @overload bidict/_bidict.py:108:6: note: def [_T] pop(self, KT, Union[VT, _T] = ...) -> Union[VT, _T] bidict/_bidict.py:108:6: note: Subclass: bidict/_bidict.py:108:6: note: @overload bidict/_bidict.py:108:6: note: def pop(self, KT) -> VT bidict/_bidict.py:108:6: note: @overload bidict/_bidict.py:108:6: note: def [DT] pop(self, KT, DT) -> Union[VT, DT] Found 1 error in 1 file (checked 1 source file)

From mypy's output, the only difference between the superclass and subclass is that the superclass uses the typing module's private _T typevar, and my subclass is using the bidict._typing module's DT typevar. But that shouldn't change the meaning of these types, should it?

Filed python/mypy#12390 so this bug can be tracked properly.

bidict/_iter.py

jab · 2022-03-10T16:48:03Z

bidict/_orderedbidict.py

-            return korv, self._pop(korv)
-        return self.inverse._pop(korv), korv
+        kv = (korv, self._pop(korv)) if self._bykey else (self.inverse._pop(korv), korv)
+        return t.cast(t.Tuple[KT, VT], kv)


Without this change, pyright (but not mypy) flags this code, as follows:

/Users/jab/src/bidict/bidict/_orderedbidict.py:60:16 - error: Expression of type "tuple[VT@OrderedBidict, Any]" cannot be assigned to return type "Tuple[KT@OrderedBidict, VT@OrderedBidict]" Tuple entry 1 is incorrect type Type "VT@OrderedBidict" cannot be assigned to type "KT@OrderedBidict" (reportGeneralTypeIssues)

Is this a bug in pyright and/or mypy?

Is there a better way to appease pyright here? The "fix" I came up with here makes the code noisier and no safer; I think the code was more readable before.

Again, I can't add a # type: ignore here without causing mypy to fail with error: Unused "type: ignore" comment, and wish I at least had the option of doing # pyright: ignore on a per-line basis instead.

I think the "reportGeneralTypeIssues" pyright check that is flagging this code is so broad, and can catch too many actual issues, to disable it globally.

This is an intentional difference in the way mypy and pyright handle overloads when some of the arguments are type Any. Mypy assumes that the return type of the overloaded call is Any in this case. Pyright uses the first overload that matches. Mypy's behavior is somewhat justifiable given that it is focused entirely on type checking. Pyright is used as both a type checker and a language server, and users rely on types for completion suggestions. When the return type is Any, no completion suggestions are possible, and users get very angry. So we've opted to not special-case Any arguments for overloads. The specific resolution rules for overloads are not specified in PEP 484, so it's reasonable to expect that there might be small variations between type checkers here.

The root issue here is that the variable korv has type Any. Is there a way you could provide a more accurate type in this case?

Interesting. Makes sense, thanks for explaining.

The root issue here is that the variable korv has type Any. Is there a way you could provide a more accurate type in this case?

I tried adding a cast to provide a more precise type:

korv = self._node_by_korv.inverse[node] if self._bykey: return korv, self._pop(korv) korv = t.cast(VT, korv) return self.inverse._pop(korv), korv

But now pyright errors with:

/Users/jab/src/bidict/bidict/_orderedbidict.py:61:34 - error: Argument of type "VT@OrderedBidict" cannot be assigned to parameter "key" of type "KT@OrderedBidict" in function "_pop" Type "VT@OrderedBidict" cannot be assigned to type "KT@OrderedBidict" (reportGeneralTypeIssues) /Users/jab/src/bidict/bidict/_orderedbidict.py:61:16 - error: Expression of type "tuple[VT@OrderedBidict, VT@OrderedBidict]" cannot be assigned to return type "Tuple[KT@OrderedBidict, VT@OrderedBidict]" Tuple entry 1 is incorrect type Type "VT@OrderedBidict" cannot be assigned to type "KT@OrderedBidict" (reportGeneralTypeIssues)

Here pyright seems confused. If I add

reveal_type(self.inverse) reveal_type(self.inverse._pop)

right before the return self.inverse._pop(korv), korv, I get:

/Users/jab/src/bidict/bidict/_orderedbidict.py:61:21 - information: Type of "self.inverse" is "OrderedBidict[VT@OrderedBidict, KT@OrderedBidict]" /Users/jab/src/bidict/bidict/_orderedbidict.py:62:21 - information: Type of "self.inverse._pop" is "(key: KT@OrderedBidict) -> VT@OrderedBidict"

Could this be a bug in pyright?

Using a pyright: ignore here for now. Will come back to this if it is not in fact a bug in pyright.

jab · 2022-03-10T16:53:52Z

pyrightconfig.json

+  "reportUnknownArgumentType": "none",
+  "reportUnknownMemberType": "none",
+  "reportUnknownVariableType": "none",
+  "reportUnusedCallResult": "none",


(In this PR, I'm much less concerned about the following, so if this stands to take attention away from any of the other, more important threads here, let's discuss this further after the other threads are resolved. With that said...)

I based this configuration on @erictraut's suggestion in microsoft/pyright#3143 (comment). I would have kept this in TOML, which I generally prefer over JSON (especially strict JSON, where e.g. comments and trailing commas are unsupported), but it looks like pyright supports JSON5 in pyrightconfig.json. But if you want a standalone config file, it looks like pyright currently only supports pyrightconfig.json, not e.g. pyrightconfig.toml. (When configuration is meant for a single tool, and not meant to be shared by multiple tools, I prefer it to go in a dedicated config file for that tool. Putting some tools' config in pyproject.toml, while having to leave other tools' configs in their own dedicated config files, since they don't support pyproject.toml (and in some cases never will), makes it harder for me to remember where to find the config for a given tool - "is this one of the tools whose config is in pyproject.toml, or in its own config file?" - or whether the tool is non-obviously being influenced by any of the other config sections in pyproject.toml.)

Also, I was hoping pyright offered a way to express this configuration more like:

{ // Enable all strict checks (current, as well as any added in future) // without duplicating/exploding them here: "strict": true, // Selectively override the following checks which were enabled via strict=true, // since these checks are known to be unhelpful in this codebase: "reportFoo": "none", }

But couldn't get that to work. Is this not currently possible?

No, that's not currently possible. You can specify which diagnostic rules you want to enable by default for all files, and then you can specify "strict" on a per-file or per-directory basis. The "strict" overrides the less-than-strict settings, not the other way around. This is designed primarily for incremental typing where a type base starts with just a few rules enabled for the bulk of the code base but new files and modules are strictly typed.

One big benefit of json over toml is that json files can be described using a schema. The Pyright LSP ships with a schema to its own config file, and VS Code (or other smart editors) can then use that schema to validate the config file and offer edit-time completion suggestions and documentation. None of that is currently possible with ".toml" files.

Thanks for explaining. I do think it’d still be useful to be able to go in the other direction too: start with a global strict=true, and then globally turn off a select few checks as needed.

Re toml, good point. Now I’ll have to search for tomlschema to see if anyone’s working on that yet!

I opened microsoft/pyright#3198 so this suggestion can be tracked properly.

bidict/_orderedbase.py

jab · 2022-03-12T15:27:27Z

bidict/_orderedbase.py

@@ -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


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 pyright: ignore here. Really feels like the best UX for users in this situation.

(hit "enter" too soon)
Big thanks again to @erictraut for staying open to that suggestion. I hope it turns out to stand the test of time as more users adopt pyright.

(For completeness, the correct link to the issue tracking this bug is microsoft/pyright#3183)

jab · 2022-03-13T15:27:36Z

bidict/_bidict.py

@@ -135,7 +135,7 @@ def popitem(self) -> t.Tuple[KT, VT]:
        del self._invm[val]
        return key, val

-    @t.overload
+    @t.overload  # type: ignore [override]


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: MutableMapping.update() does work with any SupportsKeysAndGetItem type, whereas my subclass does require the narrower type Mapping (since it calls items() on it), violating the Liskov substitution principle.)

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 SupportsKeysAndGetItem at this time. (While this may technically be the minimum interface that my update() method should accept, in all likelihood, not once in the history of this code has anyone ever passed in a SupportsKeysAndGetItems that wasn't also a Mapping.) So I'm going with a type: ignore here for now.

For completeness, here's a sketch of the kind of changes that would be required to support non-Mapping SupportsKeysAndGetItems objects, before I throw this away:

show/hide diff

diff --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)

jab · 2022-03-20T14:54:13Z

As of the latest revision of this PR, both the Pyright and mypy checks are passing, thanks in no small part to being able to use ignore comments for the cases where they disagree (whether due to bugs, design differences, or whatever else).

Unsurprisingly, the immediate benefit to the quality of the bidict codebase gained by adding a second type checker to CI is marginal. But being able to see where the two type checkers agree vs. disagree that there's a problem provides a valuable input into decisions about how much to change the implicated code. This benefit will continue as new-and-improved versions of these type checkers are released and integrated. This has also proven a powerful way to discover previously-unreported bugs in both of these type checkers.

On that note, I believe I've created issues for at least the most obvious outstanding mypy and Pyright issues uncovered by this PR, and linked to them from here. Hope these findings end up being helpful for many more projects and users than bidict touches directly.

For now, I'll (squash and) merge this, with big thanks again to @erictraut and @JelleZijlstra for collaborating on this. It's been a pleasure, and I look forward to more collaboration in the future.

Trial pyright [wip]

5d76ab7

add 'flagged by pyright' comments for code review

7129938

jab force-pushed the pyright branch from bd07620 to 7129938 Compare March 10, 2022 16:55

jab commented Mar 10, 2022

View reviewed changes

This was referenced Mar 10, 2022

Allow line-level type controls microsoft/pyright#3143

Closed

Add devcontainer setup #237

Merged

Take advantage of improvements in latest pyright

fde6470

jab changed the title ~~Trial pyright [wip]~~ Use Pyright in CI Mar 12, 2022

jab marked this pull request as ready for review March 12, 2022 15:03

jab commented Mar 12, 2022

View reviewed changes

Enable Pyright strict mode globally.

2105419

jab force-pushed the pyright branch from f30b6ad to 2105419 Compare March 13, 2022 01:14

JelleZijlstra mentioned this pull request Mar 13, 2022

Incompatible override of overloaded function not caught microsoft/pyright#3189

Closed

Upgrade pre-commit hooks, particularly mypy

649a4c2

JelleZijlstra mentioned this pull request Mar 13, 2022

dict.pop: Remove default for second argument python/typeshed#7481

Merged

jab commented Mar 13, 2022

View reviewed changes

Upgrade to latest mypy in pre-commit hook

452b027

jab force-pushed the pyright branch from 8bb8d96 to ab3ae43 Compare March 20, 2022 13:54

jab added 2 commits March 20, 2022 10:03

Improve some comments

d9f9da1

Upgrade to latest Pyright

5956a19

jab force-pushed the pyright branch from ab3ae43 to 5956a19 Compare March 20, 2022 14:06

Improve some comments

fb9d971

jab merged commit bdddb58 into main Mar 20, 2022

jab deleted the pyright branch March 20, 2022 14:54

jab mentioned this pull request Mar 21, 2022

Support mypy: ignore inline comments to suppress mypy errors python/mypy#12358

Open

Use Pyright in CI #242

Use Pyright in CI #242

Conversation

jab commented Mar 10, 2022 • edited Loading

gitpod-io bot commented Mar 10, 2022

codecov bot commented Mar 10, 2022 • edited Loading

Codecov Report

jab left a comment • edited Loading

Choose a reason for hiding this comment

jab Mar 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erictraut Mar 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jab Mar 11, 2022 • edited Loading

Choose a reason for hiding this comment

jab Mar 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jab Mar 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jab Mar 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jab Mar 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jab Mar 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jab Mar 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jab commented Mar 20, 2022 • edited Loading

jab commented Mar 10, 2022 •

edited

Loading

codecov bot commented Mar 10, 2022 •

edited

Loading

jab left a comment •

edited

Loading

jab Mar 10, 2022 •

edited

Loading

erictraut Mar 10, 2022 •

edited

Loading

jab Mar 11, 2022 •

edited

Loading

jab Mar 11, 2022 •

edited

Loading

jab Mar 15, 2022 •

edited

Loading

jab Mar 11, 2022 •

edited

Loading

jab Mar 13, 2022 •

edited

Loading

jab Mar 10, 2022 •

edited

Loading

jab Mar 11, 2022 •

edited

Loading

jab commented Mar 20, 2022 •

edited

Loading