-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Tweaks to --strict-equality based on user feedback #6674
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
Conversation
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 should make --strict-equality
generate many fewer false positives. Left some comments about minor things. You can merge this once you've addressed the feedback.
from typing import Union | ||
x: Union[bytes, str] | ||
|
||
b'abc' in x |
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.
b'a' in 'b'
fails at runtime. Should this generate an error?
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, but this is independent of this flag. The error message says "Non-overlapping container check ..." while in this example the check may return True. I think this can be tightened in typeshed, we can just define str.__contains__
as accepting str
, because 42 in 'b'
etc. all fail as well at runtime.
mypy/checkexpr.py
Outdated
return False | ||
|
||
def dangerous_comparison(self, left: Type, right: Type, | ||
original_cont_type: Optional[Type] = None) -> 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: original_cont_type
was not immediately clear. Maybe rename to original_container
or container_type
?
mypy/checkexpr.py
Outdated
# TODO: support other types (see has_member())? | ||
return False | ||
|
||
def has_bytes_component(self, typ: Type) -> 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.
Style nit: This should be a module-level function.
mypy/checkexpr.py
Outdated
@@ -1974,9 +1980,33 @@ def visit_comparison_expr(self, e: ComparisonExpr) -> Type: | |||
assert result is not None | |||
return result | |||
|
|||
def dangerous_comparison(self, left: Type, right: Type) -> bool: | |||
def custom_equality_method(self, typ: Type) -> 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.
Style nit: This should be a module-level function, since it doesn't depend on self
.
mypy/checkexpr.py
Outdated
if method and method.info: | ||
return not method.info.fullname().startswith('builtins.') | ||
return False | ||
# TODO: support other types (see has_member())? |
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 think that it would be good to support some additional types -- at least TupleType
(use fallback). Maybe Any
should return true as well?
mypy/checkexpr.py
Outdated
"""Does this type have a custom __eq__() method?""" | ||
if isinstance(typ, UnionType): | ||
return any(self.custom_equality_method(t) for t in typ.items) | ||
if isinstance(typ, Instance): |
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.
A really minor nit: maybe handle Instance
first, since it's probably the most common case.
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.
Looks good now. Thanks for the updates!
Opened python/typeshed#2937 for the |
Fixes #6607
Fixes #6608