-
-
Notifications
You must be signed in to change notification settings - Fork 3k
--strict-equality for None
#19718
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
--strict-equality for None
#19718
Conversation
I'm curious to see what the Mypy primer thinks of it...
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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.
Thanks, this makes sense to me!
|
Would it be a good idea to add a warning/error if |
I thought about this too, but did not take the time to check how Mypy reacts in similar cases. So thanks for the soft push. First experiment: selecting only I will implement a similar check later today. |
Not a strong opinion, but IIUC these are per-module flags, so I guess some people may want to specify |
docs/source/error_code_list2.rst
Outdated
| return x == b'magic' # OK | ||
| :option:`--strict-equality <mypy --strict-equality>` does not include comparisons with | ||
| ``None`` for historical reasons (support for type comments): |
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 not the only reason, and probably not the main reason. IMO the main reason is that in large code-bases there is a chance that None can get through the cracks easily, so people often add some assert x is not None in tests. Flagging those test asserts would cause a lot of noise.
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.
Okay, then I will just remove the "for historical reasons (support for type comments)" part from this sentence.
By the way: I temporarily played with skipping None checks in assert statements, and I believe Pyright avoids all non-overlap checks in assertions. However, one might sometimes be happy if assertions are checked in this way, and adding (explained) type: ignores in such cases (or disabling --strict-quality-for-none specific modules) does not seem too much effort. That's why I dropped the idea again.
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 now removed it here and a similar clause in command_line.rst.
Oh yes, good point. Considering this, I would also tend to avoid throwing an error. At least, it is clearly documented behaviour. |
|
Diff from mypy_primer, showing the effect of this PR on open source code: aiohttp (https://github.com/aio-libs/aiohttp)
+ aiohttp/client_reqrep.py:147:48: error: Non-overlapping identity check (left operand type: "URL", right operand type: "type[_SENTINEL]") [comparison-overlap]
+ aiohttp/client_reqrep.py:147:48: note: See https://mypy.rtfd.io/en/stable/_refs.html#code-comparison-overlap for more info
pandas (https://github.com/pandas-dev/pandas)
+ pandas/core/arrays/datetimes.py:2958: error: Non-overlapping identity check (left operand type: "Timestamp", right operand type: "NaTType") [comparison-overlap]
+ pandas/core/arrays/datetimes.py:2966: error: Non-overlapping identity check (left operand type: "Timestamp", right operand type: "NaTType") [comparison-overlap]
+ pandas/core/arrays/arrow/array.py:703: error: Non-overlapping identity check (left operand type: "int | integer[Any] | slice[Any, Any, Any] | list[int]", right operand type: "EllipsisType") [comparison-overlap]
+ pandas/core/arrays/sparse/array.py:972: error: Non-overlapping identity check (left operand type: "int | integer[Any] | slice[Any, Any, Any] | list[int] | ndarray[tuple[Any, ...], dtype[Any]] | tuple[int | ellipsis, ...]", right operand type: "ellipsis") [comparison-overlap]
+ pandas/io/json/_json.py:1209: error: Non-overlapping identity check (left operand type: "ExtensionDtype | str | dtype[Any] | type[object] | Mapping[Hashable, ExtensionDtype | str | dtype[Any] | type[str] | type[complex] | type[bool] | type[object]]", right operand type: "Literal[True]") [comparison-overlap]
|
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.
OK, in general the bar for a new flag is quite high, but this is something that would look worse with a e.g. dedicated error code.
Fixes #18386 (at least partly)
(edited)
In a first test run, in which I included checks against
Nonein--strict-equality, the Mypy primer gave hundreds of newcomparison-overlapreports. Many of them seem really helpful (including those for the Mypy source code itself), because it is often hard to tell if non-overlappingNonechecks are just remnants of incomplete refactorings or can handle cases with corrupted data or similar issues. As it was only a little effort, I decided to add the option--strict-equality-for-noneto Mypy, which is disabled even in--strictmode. Other libraries could adjust to this new behaviour if and when they want. If many of them do so, we could eventually enable--strict-equality-for-nonein--strictmode or even merge it with--strict-equalitylater.The remaining new true positives revealed by the Mypy primer are the result of no longer excluding types with custom
__eq__methods for identity checks (which, in my opinion, makes sense even in case--strict-equality-for-nonewould be rejected).