-
-
Notifications
You must be signed in to change notification settings - Fork 3k
Do not handle NotImplementedType as Any anymore.
#20114
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
base: master
Are you sure you want to change the base?
Do not handle NotImplementedType as Any anymore.
#20114
Conversation
This comment has been minimized.
This comment has been minimized.
…ny' into feature/notimplementedtype_not_any # Conflicts: # mypy/checker.py
This comment has been minimized.
This comment has been minimized.
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.
The change looks reasonable to me, but I'm a bit concerned about overriding typeshed definition in code, essentially pretending that there's no Any inheritance
| reveal_type(headers) # N: Revealed type is "Union[__main__.Headers, typing.Iterable[tuple[builtins.bytes, builtins.bytes]]]" | ||
| [builtins fixtures/isinstancelist.pyi] | ||
|
|
||
| [case testReturnNotImplementedInBinaryMagicMethods] |
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.
Why did you move this to check-overloading?
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.
These tests were originally in the "Returning Any" section of check-warnings.test, but that section no longer fits. Since returning NotImplemented is usually applied in the context of operator overloading, I moved the tests to check-overloading.test. Do you know a place that fits better?
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.
I'd probably put them in check-classes.test, but that one is already too big, so I'm fine with your decision - just a bit weird to have a set of tests without a single @overload in an overloads test file:)
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.
Yes, it is. But we may add many explicit overloads to them later...
mypy/mro.py
Outdated
| # The property of falling back to Any is inherited. | ||
| info.fallback_to_any = any(baseinfo.fallback_to_any for baseinfo in info.mro) | ||
| # The property of falling back to Any is (usually) inherited. | ||
| if info.fullname == "builtins._NotImplementedType": |
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.
This is a nice duct tape application, but maybe we should consider changing that in typeshed (or our own typeshed patch) directly?
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.
Yes, my favourite would be to update typeshed. However, this was closed in typeshed/13488 due to something with __eq__ and the required adjustments for type checkers. I will ask around.
I have not modified Mypy's own typeshed patch so far. I will try it...
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.
...it worked!
This comment has been minimized.
This comment has been minimized.
|
Oh, with the current state of this PR: class A:
def __add__(self, x: A)-> int | NotImplementedType: ...
reveal_type(A() + A()) # int | NotImplementedTypeHere, it is clear what to do (remove But what about the following (very special) case? class A:
def __add__(self, x: A) -> NotImplementedType: ...
reveal_type(A() + A()) # NotImplementedTypeIdeally, it would mean |
|
Replace |
…inary methods ("+", "and", etc.).
for more information, see https://pre-commit.ci
|
I added |
This comment has been minimized.
This comment has been minimized.
Wouldn't this cause more problems? class A:
def __add__(self, x: A) -> NotImplementedType: ...
reveal_type(A() + A()) # Never
a = A() + A() # error: Need type annotation for "a"(I hope this is only a temporary state and we soon have clear guidelines on how to use |
This comment has been minimized.
This comment has been minimized.
|
|
||
| @final | ||
| @type_check_only | ||
| class _NotImplementedType(Any): |
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.
...wait, but you still need a patch, right? (misc/typeshed_patches directory seems to be where they live) Our typeshed updates routine (misc/sync-typeshed.py) is "clone the HEAD of current typeshed, then apply patches in order", so your change will be lost during the next sync
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.
I had a short look at it, and you are likely right. I have to admit that I am absolutely unfamiliar with the workflow and not very interested in spending time learning it. Would you like to contribute this change here (or in a separate PR - I do not know...)? Otherwise, I would simply go back to the original solution.
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.
I'm not deeply familiar with that either, but... Just git format-patch -1 -o misc/typeshed_patches/ 9b584b mypy/typeshed/ should do the trick (rename and edit the Subject line of the new file if you wish, commit&push) - I can open a PR with that file in your fork, but might be easier to just generate it on your end? (I didn't run the command, but the patches all look like format-patch output, and I'm moderately certain that I remember its arguments correctly)
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.
Cool, thanks a lot for your help; it seems to have worked exactly as you suggested!
This comment has been minimized.
This comment has been minimized.
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.
LG!
mypy/checker.py
Outdated
| return typ | ||
|
|
||
| @staticmethod | ||
| def is_notimplemented(t: ProperType) -> bool: |
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.
Nit: are staticmethods as fast as bare functions with mypyc? (I really don't know the answer here, just wondering)
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.
I have no idea. But yes, maybe a relevant point, as it could be called quite often for many code bases.
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.
I had a look. Both @staticmethod and @classmethod are very rarely used in performance-critical code. (The only static method in semanal, get_deprecated, was introduced by me...) So performance might, in fact, be an issue. Or it's just a question of style. Whatever, I turned both is_notimplemented and erase_notimplemented into normal functions and moved them to typeops.
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.
Cool! I still don't know whether it makes any difference, but using free functions avoids this question altogether:)
This comment has been minimized.
This comment has been minimized.
|
Diff from mypy_primer, showing the effect of this PR on open source code: spack (https://github.com/spack/spack)
+ lib/spack/spack/llnl/util/lock.py:779: error: Incompatible return value type (got "_NotImplementedType", expected "bool") [return-value]
+ lib/spack/spack/llnl/util/lock.py:782: error: Incompatible return value type (got "_NotImplementedType", expected "bool") [return-value]
hydpy (https://github.com/hydpy-dev/hydpy)
+ hydpy/core/devicetools.py:1119: error: Incompatible return value type (got "_NotImplementedType", expected "bool") [return-value]
spark (https://github.com/apache/spark)
+ python/pyspark/pandas/numpy_compat.py:202: error: Incompatible return value type (got "_NotImplementedType", expected "IndexOpsMixin") [return-value]
+ python/pyspark/pandas/numpy_compat.py:229: error: Incompatible return value type (got "_NotImplementedType", expected "IndexOpsMixin") [return-value]
Tanjun (https://github.com/FasterSpeeding/Tanjun)
+ tanjun/annotations.py:2324: error: Dict entry 0 has incompatible type "type[Attachment]": "_NotImplementedType"; expected "Any": "tuple[Callable[..., Any], ...]" [dict-item]
+ tanjun/annotations.py:2326: error: Dict entry 2 has incompatible type "type[PartialChannel]": "_NotImplementedType"; expected "Any": "tuple[Callable[..., Any], ...]" [dict-item]
+ tanjun/annotations.py:2327: error: Dict entry 3 has incompatible type "type[InteractionChannel]": "_NotImplementedType"; expected "Any": "tuple[Callable[..., Any], ...]" [dict-item]
+ tanjun/annotations.py:2331: error: Dict entry 7 has incompatible type "type[InteractionMember]": "_NotImplementedType"; expected "Any": "tuple[Callable[..., Any], ...]" [dict-item]
+ tanjun/annotations.py:2451: error: Non-overlapping identity check (left operand type: "tuple[Callable[..., Any], ...]", right operand type: "_NotImplementedType") [comparison-overlap]
sympy (https://github.com/sympy/sympy)
+ sympy/core/relational.py:864: error: Incompatible return value type (got "_NotImplementedType", expected "Self | BooleanTrue | BooleanFalse") [return-value]
+ sympy/polys/domains/gaussiandomains.py:163: error: Incompatible return value type (got "_NotImplementedType", expected "tuple[Self, Self]") [return-value]
+ sympy/polys/rings.py:1113: error: Incompatible return value type (got "_NotImplementedType", expected "tuple[PolyElement[Er], PolyElement[Er]]") [return-value]
+ sympy/matrices/matrixbase.py:3118: error: Incompatible return value type (got "_NotImplementedType", expected "MatrixBase | Expr") [return-value]
+ sympy/matrices/matrixbase.py:3121: error: Incompatible return value type (got "_NotImplementedType", expected "MatrixBase | Expr") [return-value]
+ sympy/matrices/matrixbase.py:3301: error: Incompatible return value type (got "_NotImplementedType", expected "MatrixBase") [return-value]
+ sympy/matrices/matrixbase.py:3304: error: Incompatible return value type (got "_NotImplementedType", expected "MatrixBase") [return-value]
+ sympy/algebras/quaternion.py:972: error: Incompatible return value type (got "_NotImplementedType", expected "Quaternion") [return-value]
pandas (https://github.com/pandas-dev/pandas)
+ pandas/core/arrays/masked.py:855: error: Incompatible return value type (got "_NotImplementedType", expected "BooleanArray") [return-value]
discord.py (https://github.com/Rapptz/discord.py)
+ discord/ui/view.py:300: error: Incompatible return value type (got "_NotImplementedType", expected "list[dict[str, Any]]") [return-value]
ibis (https://github.com/ibis-project/ibis)
+ ibis/expr/types/core.py:979: error: Incompatible return value type (got "_NotImplementedType", expected "Value") [return-value]
+ ibis/expr/types/generic.py:1224: error: Incompatible return value type (got "_NotImplementedType", expected "BooleanValue") [return-value]
+ ibis/expr/types/numeric.py:1624: error: Incompatible return value type (got "_NotImplementedType", expected "IntegerValue") [return-value]
scipy (https://github.com/scipy/scipy)
+ scipy/optimize/_trustregion_constr/report.py:4: error: Incompatible types in assignment (expression has type "_NotImplementedType", variable has type "list[str]") [assignment]
+ scipy/optimize/_trustregion_constr/report.py:5: error: Incompatible types in assignment (expression has type "_NotImplementedType", variable has type "list[int]") [assignment]
+ scipy/optimize/_trustregion_constr/report.py:6: error: Incompatible types in assignment (expression has type "_NotImplementedType", variable has type "list[str]") [assignment]
+ scipy/integrate/_ivp/rk.py:76: error: Incompatible types in assignment (expression has type "_NotImplementedType", variable has type "ndarray[tuple[Any, ...], dtype[Any]]") [assignment]
+ scipy/integrate/_ivp/rk.py:77: error: Incompatible types in assignment (expression has type "_NotImplementedType", variable has type "ndarray[tuple[Any, ...], dtype[Any]]") [assignment]
+ scipy/integrate/_ivp/rk.py:78: error: Incompatible types in assignment (expression has type "_NotImplementedType", variable has type "ndarray[tuple[Any, ...], dtype[Any]]") [assignment]
+ scipy/integrate/_ivp/rk.py:79: error: Incompatible types in assignment (expression has type "_NotImplementedType", variable has type "ndarray[tuple[Any, ...], dtype[Any]]") [assignment]
+ scipy/integrate/_ivp/rk.py:80: error: Incompatible types in assignment (expression has type "_NotImplementedType", variable has type "ndarray[tuple[Any, ...], dtype[Any]]") [assignment]
+ scipy/integrate/_ivp/rk.py:81: error: Incompatible types in assignment (expression has type "_NotImplementedType", variable has type "int") [assignment]
+ scipy/integrate/_ivp/rk.py:82: error: Incompatible types in assignment (expression has type "_NotImplementedType", variable has type "int") [assignment]
+ scipy/integrate/_ivp/rk.py:83: error: Incompatible types in assignment (expression has type "_NotImplementedType", variable has type "int") [assignment]
yarl (https://github.com/aio-libs/yarl)
+ tests/test_url_cmp_and_hash.py:42:12: error: Non-overlapping identity check (left operand type: "bool", right operand type: "_NotImplementedType") [comparison-overlap]
+ tests/test_url_cmp_and_hash.py:42:12: note: See https://mypy.rtfd.io/en/stable/_refs.html#code-comparison-overlap for more info
+ tests/test_url_cmp_and_hash.py:55:12: error: Non-overlapping identity check (left operand type: "bool", right operand type: "_NotImplementedType") [comparison-overlap]
+ tests/test_url_cmp_and_hash.py:75:12: error: Non-overlapping identity check (left operand type: "bool", right operand type: "_NotImplementedType") [comparison-overlap]
+ tests/test_url_cmp_and_hash.py:88:12: error: Non-overlapping identity check (left operand type: "bool", right operand type: "_NotImplementedType") [comparison-overlap]
xarray (https://github.com/pydata/xarray)
+ xarray/core/dataset.py: note: In member "_binary_op" of class "Dataset":
+ xarray/core/dataset.py:7686: error: Incompatible return value type (got "_NotImplementedType", expected "Dataset") [return-value]
+ xarray/core/dataarray.py: note: In member "_binary_op" of class "DataArray":
+ xarray/core/dataarray.py:4890: error: Incompatible return value type (got "_NotImplementedType", expected "Self") [return-value]
+ xarray/core/datatree.py: note: In member "_binary_op" of class "DataTree":
+ xarray/core/datatree.py:1913: error: Incompatible return value type (got "_NotImplementedType", expected "DataTree") [return-value]
|
Fixes #18914
Completes #363
Handling
NotImplementedTypeasAnydecreases type safety and interferes with using it in type annotations (as discussed in python/typing#1548 and elsewhere). There was already an attempt to change this in typeshed (python/typeshed#13488).This PR modifies Mypy's internal representation of
NotImplementedTypeas if typeshed would not imply it inherits fromAny. Additionally, it stops treatingNotImplementedTypeasAnyitself during return type analysis.Most of the Mypy primer results suggest this change is directly helpful. However, some type annotations would need to be extended with
NotImplementedType, which is only available since Python 3.10, and Mypy currently still supports Python 3.9.A (hopefully complete enough) summary of the Mypy primer results:
Usages of
NotImplementedto indicate that a method must be overridden (instead of relying onabc):Incompletely annotated helper functions:
Usages of
NotImplementedas a placeholder for features not available so far:Usage of
NotImplementedto mark class attributes that must be overridden:Accessing the (unmodified) results of the corresponding dunder methods:
In my opinion, the last case is the only problematic one if the relevant dunder method's return type is defined in another library. I hope such cases are relatively rare, but maybe typeshed should be updated in sync??? (And then, if necessary, other type checkers??????) cc @davidhalter