Use Pyright in CI #242
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 = None | ||
| inv._invweak = weakref.ref(self) | ||
|
There was a problem hiding this comment. 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.
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.
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.
(Sorry for going off on this tangent, but now I'm interested :) ) I tried code like There was a problem hiding this comment. 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. 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. | ||
|
|
@@ -486,7 +487,7 @@ def _init_from(self, other: MapOrIterItems[KT, VT]) -> None: | |
| self._fwdm.update(other) | ||
| # 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: MapOrIterItems[t.Any, t.Any] = other.inverse if isinstance(other, BidictBase) else inverted(self._fwdm) | ||
jab marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| self._invm.update(inv) | ||
|
|
||
| #: Used for the copy protocol. | ||
|
|
@@ -496,15 +497,15 @@ def _init_from(self, other: MapOrIterItems[KT, VT]) -> None: | |
| def __or__(self: BT, other: t.Mapping[KT, VT]) -> BT: | ||
| """Return self|other.""" | ||
| if not isinstance(other, t.Mapping): | ||
| return NotImplemented # flagged by pyright | ||
| 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 # flagged by pyright | ||
|
There was a problem hiding this comment. 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
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. There was a problem hiding this comment. 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 There was a problem hiding this comment. 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
There was a problem hiding this comment.
@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: _NotImplementedTypeAs Jelle conjectured, the reason you're seeing different behavior between I've filed the following pyright bug to track this: microsoft/pyright#3171 There was a problem hiding this comment. 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 There was a problem hiding this comment. It looks like I can add 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 fileI don't see any way to run mypy in strict mode and have it warn-but-not-error on unused As https://mypy.readthedocs.io/en/stable/command_line.html#cmdoption-mypy-warn-unused-ignores says:
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:
There was a problem hiding this comment. 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):
There was a problem hiding this comment. 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. There was a problem hiding this comment. Fair enough! That was the first bullet. What about the second (supporting “mypy: ignore”)? There was a problem hiding this comment. I opened python/mypy#12359 and python/mypy#12358 to track these proposals properly so they're not buried here. |
||
| new = self.__class__(other) | ||
| new._update(self, rbof=False) | ||
| return new | ||
|
|
@@ -528,7 +529,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] | ||
|
|
||
|
|
||
| # See BidictBase._set_reversed() above. | ||
|
|
||
| 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 # pyright: ignore # https://github.com/microsoft/pyright/issues/3172 | ||
|
There was a problem hiding this comment. 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. (hit "enter" too soon) There was a problem hiding this comment. (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,42 @@ | ||
| { | ||
| "exclude": [ | ||
| "docs", | ||
| "tests", | ||
| ], | ||
| "reportConstantRedefinition": "error", | ||
| "reportDuplicateImport": "error", | ||
| "reportFunctionMemberAccess": "error", | ||
| "reportIncompatibleMethodOverride": "error", | ||
| "reportIncompatibleVariableOverride": "error", | ||
| "reportInvalidStringEscapeSequence": "error", | ||
| "reportInvalidStubStatement": "error", | ||
| "reportInvalidTypeVarUse": "error", | ||
| "reportMatchNotExhaustive": "error", | ||
| "reportMissingTypeArgument": "error", | ||
| "reportMissingTypeStubs": "error", | ||
| "reportOverlappingOverload": "error", | ||
| "reportPropertyTypeMismatch": "error", | ||
| "reportSelfClsParameterName": "error", | ||
| "reportUnknownLambdaType": "error", | ||
| "reportUnknownParameterType": "error", | ||
| "reportUnnecessaryTypeIgnoreComment": "warning", | ||
| "reportUnsupportedDunderAll": "error", | ||
| "reportUntypedBaseClass": "error", | ||
| "reportUntypedClassDecorator": "error", | ||
| "reportUntypedFunctionDecorator": "error", | ||
| "reportUntypedNamedTuple": "error", | ||
| "reportUnusedClass": "error", | ||
| "reportUnusedFunction": "error", | ||
| "reportUnusedImport": "error", | ||
| "reportUnusedVariable": "error", | ||
| "strictDictionaryInference": true, | ||
| "strictListInference": true, | ||
| "strictParameterNoneValue": true, | ||
|
|
||
| // Leave these off for now: | ||
| "reportPrivateUsage": "none", | ||
| "reportUnknownArgumentType": "none", | ||
| "reportUnknownMemberType": "none", | ||
| "reportUnknownVariableType": "none", | ||
| "reportUnusedCallResult": "none", | ||
|
There was a problem hiding this comment. (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? There was a problem hiding this comment. 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. There was a problem hiding this comment. 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! There was a problem hiding this comment. I opened microsoft/pyright#3198 so this suggestion can be tracked properly. |
||
| } | ||
| 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 thelocalenvironment gets set up with therequirements/lint.txtdependencies already