-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Generalize reachability checks to support enums #7000
Changes from 2 commits
df76db6
eaa0872
4cb5572
0c084dc
5ec6868
1d95ec0
d1d99a7
5b70ff5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,7 @@ | |
|
||
import itertools | ||
import fnmatch | ||
import sys | ||
from contextlib import contextmanager | ||
|
||
from typing import ( | ||
|
@@ -3487,21 +3488,34 @@ def find_isinstance_check(self, node: Expression | |
vartype = type_map[expr] | ||
return self.conditional_callable_type_map(expr, vartype) | ||
elif isinstance(node, ComparisonExpr): | ||
# Check for `x is None` and `x is not None`. | ||
operand_types = [coerce_to_literal(type_map[expr]) | ||
for expr in node.operands if expr in type_map] | ||
|
||
is_not = node.operators == ['is not'] | ||
if any(is_literal_none(n) for n in node.operands) and ( | ||
is_not or node.operators == ['is']): | ||
if (is_not or node.operators == ['is']) and len(operand_types) == len(node.operands): | ||
if_vars = {} # type: TypeMap | ||
else_vars = {} # type: TypeMap | ||
for expr in node.operands: | ||
if (literal(expr) == LITERAL_TYPE and not is_literal_none(expr) | ||
and expr in type_map): | ||
|
||
for i, expr in enumerate(node.operands): | ||
var_type = operand_types[i] | ||
other_type = operand_types[1 - i] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This looks like a mystery to me. What if one has There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We've actually never properly handled this case, I think. The old if-check on line 3490-ish lets this code run only if there's exactly only a single operator; the new if-check I'm replacing that with continues to do the same thing. So, as a consequence, we can safely assume there'll be exactly two operands at this point. I have a fix for this, but I decided it might be better to submit it as a separate PR. Once I combined this with the equality changes mentioned above, the changes ended up being much more intrusive. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Maybe I am missing something, but the code there looks like it is about completely different case, it is about (Also it is in a different There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, maybe the line numbers shifted after I merged. It's now line 3541-ish. The old check used to do this:
And the new checks do this:
We also make the same assumption when handling the
and:
|
||
|
||
if literal(expr) == LITERAL_TYPE and is_singleton_type(other_type): | ||
# This should only be true at most once: there should be | ||
# two elements in node.operands, and at least one of them | ||
# should represent a None. | ||
vartype = type_map[expr] | ||
none_typ = [TypeRange(NoneType(), is_upper_bound=False)] | ||
if_vars, else_vars = conditional_type_map(expr, vartype, none_typ) | ||
# exactly two elements in node.operands and if the 'other type' is | ||
# a singleton type, it by definition does not need to be narrowed: | ||
# it already has the most precise type possible so does not need to | ||
# be narrowed/included in the output map. | ||
# | ||
# TODO: Generalize this to handle the case where 'other_type' is | ||
# a union of singleton types. | ||
|
||
if isinstance(other_type, LiteralType) and other_type.is_enum_literal(): | ||
fallback_name = other_type.fallback.type.fullname() | ||
var_type = try_expanding_enum_to_union(var_type, fallback_name) | ||
|
||
target_type = [TypeRange(other_type, is_upper_bound=False)] | ||
if_vars, else_vars = conditional_type_map(expr, var_type, target_type) | ||
break | ||
|
||
if is_not: | ||
|
@@ -4438,3 +4452,73 @@ def is_overlapping_types_no_promote(left: Type, right: Type) -> bool: | |
def is_private(node_name: str) -> bool: | ||
"""Check if node is private to class definition.""" | ||
return node_name.startswith('__') and not node_name.endswith('__') | ||
|
||
|
||
def is_singleton_type(typ: Type) -> bool: | ||
"""Returns 'true' if this type is a "singleton type" -- if there exists | ||
exactly only one runtime value associated with this type. | ||
|
||
That is, given two values 'a' and 'b' that have the same type 't', | ||
'is_singleton_type(t)' returns True if and only if the expression 'a is b' is | ||
always true. | ||
|
||
Currently, this returns True when given NoneTypes and enum LiteralTypes. | ||
|
||
Note that other kinds of LiteralTypes cannot count as singleton types. For | ||
example, suppose we do 'a = 100000 + 1' and 'b = 100001'. It is not guaranteed | ||
that 'a is b' will always be true -- some implementations of Python will end up | ||
constructing two distinct instances of 100001. | ||
""" | ||
# TODO: Also make this return True if the type is a bool LiteralType. | ||
# Also make this return True if the type corresponds to ... (ellipsis) or NotImplemented? | ||
return isinstance(typ, NoneType) or (isinstance(typ, LiteralType) and typ.is_enum_literal()) | ||
|
||
|
||
def try_expanding_enum_to_union(typ: Type, target_fullname: str) -> Type: | ||
"""Attempts to recursively any enum Instances with the given target_fullname | ||
Michael0x2a marked this conversation as resolved.
Show resolved
Hide resolved
|
||
into a Union of all of its component LiteralTypes. | ||
|
||
For example, if we have: | ||
|
||
class Color(Enum): | ||
RED = 1 | ||
BLUE = 2 | ||
YELLOW = 3 | ||
|
||
...and if we call `try_expanding_enum_to_union(color_instance, 'module.Color')`, | ||
this function will return Literal[Color.RED, Color.BLUE, Color.YELLOW]. | ||
Michael0x2a marked this conversation as resolved.
Show resolved
Hide resolved
|
||
""" | ||
if isinstance(typ, UnionType): | ||
new_items = [try_expanding_enum_to_union(item, target_fullname) | ||
for item in typ.items] | ||
return UnionType.make_simplified_union(new_items) | ||
elif isinstance(typ, Instance) and typ.type.is_enum and typ.type.fullname() == target_fullname: | ||
new_items = [] | ||
for name, symbol in typ.type.names.items(): | ||
if not isinstance(symbol.node, Var): | ||
continue | ||
new_items.append(LiteralType(name, typ)) | ||
# SymbolTables are really just dicts, and dicts are guaranteed to preserve | ||
# insertion order only starting with Python 3.7. So, we sort these for older | ||
# versions of Python to help make tests deterministic. | ||
# | ||
# We could probably skip the sort for Python 3.6 since people probably run mypy | ||
# only using CPython, but we might as well for the sake of full correctness. | ||
if sys.version_info < (3, 7): | ||
new_items.sort(key=lambda lit: lit.value) | ||
return UnionType.make_simplified_union(new_items) | ||
else: | ||
return typ | ||
|
||
|
||
def coerce_to_literal(typ: Type) -> Type: | ||
"""Recursively converts any Instances that have a last_known_value into the | ||
corresponding LiteralType. | ||
""" | ||
if isinstance(typ, UnionType): | ||
new_items = [coerce_to_literal(item) for item in typ.items] | ||
return UnionType.make_simplified_union(new_items) | ||
elif isinstance(typ, Instance) and typ.last_known_value: | ||
return typ.last_known_value | ||
else: | ||
return typ |
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.
How hard would be it be to do exactly the same for
==
? (Mostly so that example in #4223 will not give the false positive.)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's slightly trickier -- the semantics of
a is SomeEnum.Foo
anda == SomeEnum.Foo
are different, unfortunately.If
a
is something like an int or some other unrelated type, we know the first expression will always be False. But for the second, we have no idea sincea
could have defined a custom__eq__
function. SomeEnum itself could also have defined/inherited a custom__eq__
method, which would further complicate things.I'll submit a separate PR for this: it ended up being easier to make this change if I also added support for chained operator comparisons at the same time (see below).