Skip to content
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.

Sign up for GitHub

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

724 fix incorrect redundant-expr error for isinstance on constra… #754

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

724 fix incorrect redundant-expr error for isinstance on constra… #754

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
37f5021
WIP: 724 fix incorrect redundant-expr error for isinstance on constra…
Loch64 Sep 7, 2024
408465a
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Sep 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
127 changes: 87 additions & 40 deletions mypy/checker.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,7 @@
TypeOfAny,
TypeTranslator,
TypeType,
TypeVarConstraintWrapper,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needs a changelog entry

TypeVarId,
TypeVarLikeType,
TypeVarTupleType,
Expand Down Expand Up @@ -2149,7 +2150,9 @@ def expand_typevars(
tvars += defn.info.defn.type_vars or []
for tvar in tvars:
if isinstance(tvar, TypeVarType) and tvar.values:
subst.append([(tvar.id, value) for value in tvar.values])
subst.append(
[(tvar.id, TypeVarConstraintWrapper(value, None)) for value in tvar.values]
)
# Make a copy of the function to check for each combination of
# value restricted type variables. (Except when running mypyc,
# where we need one canonical version of the function.)
Expand Down Expand Up @@ -5032,11 +5035,17 @@ def visit_if_stmt(self, s: IfStmt) -> None:
if isinstance(t, DeletedType):
self.msg.deleted_as_rvalue(t, s)

if_map, else_map = self.find_isinstance_check(e)
if_map, else_map, is_constrained = self.find_isinstance_check(
e, returned_type_constrained_param=True
)

# If if_map is None then we know mypy considers the expression
# to be redundant.
if codes.REDUNDANT_EXPR in self.options.enabled_error_codes and if_map is None:
if (
codes.REDUNDANT_EXPR in self.options.enabled_error_codes
and if_map is None
and not is_constrained
):
self.msg.fail("Condition is always false", e, code=codes.REDUNDANT_EXPR)

with self.binder.frame_context(can_skip=True, fall_through=2):
Expand All @@ -5047,6 +5056,7 @@ def visit_if_stmt(self, s: IfStmt) -> None:
codes.REDUNDANT_EXPR in self.options.enabled_error_codes
and else_map is None
and not allow_redundant_expr
and not is_constrained
):
self.msg.fail("Condition is always true", e, code=codes.REDUNDANT_EXPR)
self.push_type_map(else_map)
Expand Down Expand Up @@ -6220,9 +6230,26 @@ def combine_maps(list_maps: list[TypeMap]) -> TypeMap:
else_map = {}
return if_map, else_map

@overload
def find_isinstance_check(
self,
node: Expression,
*,
in_boolean_context: bool = True,
returned_type_constrained_param: True,
) -> tuple[TypeMap, TypeMap, bool]: ...
@overload
def find_isinstance_check(
self, node: Expression, *, in_boolean_context: bool = True
) -> tuple[TypeMap, TypeMap]:
) -> tuple[TypeMap, TypeMap]: ...

def find_isinstance_check(
self,
node: Expression,
*,
in_boolean_context: bool = True,
returned_type_constrained_param: bool = False,
) -> tuple[TypeMap, TypeMap] | tuple[TypeMap, TypeMap, bool]:
"""Find any isinstance checks (within a chain of ands). Includes
implicit and explicit checks for None and calls to callable.
Also includes TypeGuard and TypeIs functions.
Expand All @@ -6241,11 +6268,13 @@ def find_isinstance_check(
May return {}, {}.
Can return None, None in situations involving NoReturn.
"""
if_map, else_map = self.find_isinstance_check_helper(
if_map, else_map, is_constrained = self.find_isinstance_check_helper(
node, in_boolean_context=in_boolean_context
)
new_if_map = self.propagate_up_typemap_info(if_map)
new_else_map = self.propagate_up_typemap_info(else_map)
if returned_type_constrained_param:
return new_if_map, new_else_map, is_constrained
return new_if_map, new_else_map

def upstream_typeis(self, node: CallExpr, expr: Expression) -> tuple[TypeMap, TypeMap]:
Expand Down Expand Up @@ -6298,46 +6327,54 @@ def upstream_typeis(self, node: CallExpr, expr: Expression) -> tuple[TypeMap, Ty

def find_isinstance_check_helper(
self, node: Expression, *, in_boolean_context: bool = True
) -> tuple[TypeMap, TypeMap]:
) -> tuple[TypeMap, TypeMap, bool]:
default_return = {}, {}, False
if is_true_literal(node):
return {}, None
return {}, None, False
if is_false_literal(node):
return None, {}
return None, {}, False

if isinstance(node, CallExpr):
if len(node.args) != 0:
expr = collapse_walrus(node.args[0])
if refers_to_fullname(node.callee, "builtins.isinstance"):
if len(node.args) != 2: # the error will be reported elsewhere
return {}, {}
return {}, {}, False
if literal(expr) == LITERAL_TYPE:
return conditional_types_to_typemaps(
expr,
*self.conditional_types_with_intersection(
self.lookup_type(expr),
self.get_isinstance_type(node.args[1]),
typ = self.lookup_type(expr)
return (
*conditional_types_to_typemaps(
expr,
*self.conditional_types_with_intersection(
self.lookup_type(expr),
self.get_isinstance_type(node.args[1]),
expr,
),
),
isinstance(typ, TypeVarConstraintWrapper),
)
elif refers_to_fullname(node.callee, "builtins.issubclass"):
if len(node.args) != 2: # the error will be reported elsewhere
return {}, {}
return {}, {}, False
if literal(expr) == LITERAL_TYPE:
return self.infer_issubclass_maps(node, expr)
return (*self.infer_issubclass_maps(node, expr), False)
elif refers_to_fullname(node.callee, "builtins.callable"):
if len(node.args) != 1: # the error will be reported elsewhere
return {}, {}
return default_return
if literal(expr) == LITERAL_TYPE:
vartype = self.lookup_type(expr)
return self.conditional_callable_type_map(expr, vartype)
return (*self.conditional_callable_type_map(expr, vartype), False)
elif refers_to_fullname(node.callee, "builtins.hasattr"):
if len(node.args) != 2: # the error will be reported elsewhere
return {}, {}
return default_return
attr = try_getting_str_literals(node.args[1], self.lookup_type(node.args[1]))
if literal(expr) == LITERAL_TYPE and attr and len(attr) == 1:
return self.hasattr_type_maps(expr, self.lookup_type(expr), attr[0])
return (
*self.hasattr_type_maps(expr, self.lookup_type(expr), attr[0]),
False,
)
if isinstance(node.callee, RefExpr) and node.callee.type_is:
return self.upstream_typeis(node, expr)
return (*self.upstream_typeis(node, expr), False)
if isinstance(node.callee, (RefExpr, CallExpr, LambdaExpr)):
# TODO: AssignmentExpr `(a := A())()`
if node.callee.type_guard is not None:
Expand Down Expand Up @@ -6374,10 +6411,10 @@ def find_isinstance_check_helper(
get_proper_type(node.callee.type_guard.type_guard)
)
if not guarded:
return {}, {}
return default_return
else:
guarded = node.callee.type_guard.type_guard
return {collapse_walrus(expr): guarded}, {}
return {collapse_walrus(expr): guarded}, {}, False
if isinstance(target, int):
idx = target
if called_type.items[0].def_extras.get("first_arg"):
Expand All @@ -6386,24 +6423,24 @@ def find_isinstance_check_helper(
node,
code=codes.TYPEGUARD_LIMITATION,
)
return {}, {}
return default_return
elif target in called_type.items[0].arg_names:
idx = called_type.items[0].arg_names.index(target)
elif target == "first argument":
idx = 0
if not node.args:
return {}, {}
return default_return
else:
definition = called_type.items[0].definition
assert isinstance(definition, FuncDef)
idx = [arg.variable.name for arg in definition.arguments].index(target)
if target in node.arg_names:
idx = node.arg_names.index(target) # type: ignore[arg-type]
if len(node.arg_kinds) <= idx:
return {}, {}
return default_return
if node.arg_kinds[idx].is_star():
self.fail(message_registry.TYPE_GUARD_POS_LIMITATION, node)
return {}, {}
return default_return
# we want the idx-th variable to be narrowed
expr = collapse_walrus(node.args[idx])
if literal(expr) == LITERAL_TYPE:
Expand All @@ -6415,15 +6452,19 @@ def find_isinstance_check_helper(
#
# Based: We don't do any of that.
if guard.only_true:
return {expr: guard.type_guard}, {}
return conditional_types_to_typemaps(
expr,
*self.conditional_types_with_intersection(
self.lookup_type(expr),
[TypeRange(guard.type_guard, is_upper_bound=False)],
return {expr: guard.type_guard}, {}, False
typ = self.lookup_type(expr)
return (
*conditional_types_to_typemaps(
expr,
erase=False,
*self.conditional_types_with_intersection(
typ,
[TypeRange(guard.type_guard, is_upper_bound=False)],
expr,
erase=False,
),
),
isinstance(typ, TypeVarConstraintWrapper),
)
if isinstance(node, CallExpr) and isinstance(node.callee, LambdaExpr):
return self.find_isinstance_check_helper(
Expand All @@ -6439,7 +6480,7 @@ def find_isinstance_check_helper(
narrowable_operand_index_to_hash = {}
for i, expr in enumerate(operands):
if not self.has_type(expr):
return {}, {}
return default_return
expr_type = self.lookup_type(expr)
operand_types.append(expr_type)

Expand Down Expand Up @@ -6595,11 +6636,14 @@ def has_no_custom_eq_checks(t: Type) -> bool:
# then return soon. Otherwise try to infer restrictions involving `len(x)`.
# TODO: support regular and len() narrowing in the same chain.
if any(m != ({}, {}) for m in partial_type_maps):
return reduce_conditional_maps(partial_type_maps)
return (*reduce_conditional_maps(partial_type_maps), False)
else:
# Use meet for `and` maps to get correct results for chained checks
# like `if 1 < len(x) < 4: ...`
return reduce_conditional_maps(self.find_tuple_len_narrowing(node), use_meet=True)
return (
*reduce_conditional_maps(self.find_tuple_len_narrowing(node), use_meet=True),
False,
)
elif isinstance(node, AssignmentExpr):
if_map = {}
else_map = {}
Expand All @@ -6623,6 +6667,7 @@ def has_no_custom_eq_checks(t: Type) -> bool:
return (
(None if if_assignment_map is None or if_condition_map is None else if_map),
(None if else_assignment_map is None or else_condition_map is None else else_map),
False,
)
elif isinstance(node, OpExpr) and node.op == "and":
left_if_vars, left_else_vars = self.find_isinstance_check(node.left)
Expand All @@ -6636,6 +6681,7 @@ def has_no_custom_eq_checks(t: Type) -> bool:
# types to it, since the right maps were computed assuming
# the left is True, which may be not the case in the else branch.
or_conditional_maps(left_else_vars, right_else_vars, coalesce_any=True),
False,
)
elif isinstance(node, OpExpr) and node.op == "or":
left_if_vars, left_else_vars = self.find_isinstance_check(node.left)
Expand All @@ -6646,10 +6692,11 @@ def has_no_custom_eq_checks(t: Type) -> bool:
return (
or_conditional_maps(left_if_vars, right_if_vars),
and_conditional_maps(left_else_vars, right_else_vars),
False,
)
elif isinstance(node, UnaryExpr) and node.op == "not":
left, right = self.find_isinstance_check(node.expr)
return right, left
return right, left, False
elif (
literal(node) == LITERAL_TYPE
and self.has_type(node)
Expand All @@ -6670,7 +6717,7 @@ def has_no_custom_eq_checks(t: Type) -> bool:
no_type = UninhabitedType()
if_map = {node: yes_type} if not isinstance(yes_type, UninhabitedType) else None
else_map = {node: no_type} if not isinstance(no_type, UninhabitedType) else None
return if_map, else_map
return if_map, else_map, False

# Restrict the type of the variable to True-ish/False-ish in the if and else branches
# respectively
Expand All @@ -6685,7 +6732,7 @@ def has_no_custom_eq_checks(t: Type) -> bool:
else_type = false_only(vartype)
if_map = {node: if_type} if not isinstance(if_type, UninhabitedType) else None
else_map = {node: else_type} if not isinstance(else_type, UninhabitedType) else None
return if_map, else_map
return if_map, else_map, False

def propagate_up_typemap_info(self, new_types: TypeMap) -> TypeMap:
"""Attempts refining parent expressions of any MemberExpr or IndexExprs in new_types.
Expand Down
16 changes: 16 additions & 0 deletions mypy/types.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -555,6 +555,22 @@ def is_self(self) -> bool:
return self.raw_id == 0


#
class TypeVarConstraintWrapper(Type): # TODO: add support with get_proper_type
__slots__ = ("type", "typevar")

type: Type
typevar: TypeVarType

def __init__(self, typ: Type, typevar: TypeVar):
super().__init__(typ.line, typ.column)
self.type = typ
self.typevar = typevar

def accept(self, visitor: TypeVisitor[T]) -> T:
return self.type.accept(visitor)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to make sure cache works with this



class TypeVarLikeType(ProperType):
__slots__ = ("name", "fullname", "id", "upper_bound", "default")

Expand Down
22 changes: 22 additions & 0 deletions test-data/unit/check-based-constraints.test
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
[case testSimpleConstraint]
# flags: --allow-untyped-calls
from typing import TypeVar

T = TypeVar("T", int, str)

def f(t: T):
if isinstance(t, int):
reveal_type(t) # N: Revealed type is "int"
if isinstance(t, int): # E: Condition is always true [redundant-expr]
...
else:
reveal_type(t) # N: Revealed type is "str"
if isinstance(t, int): # E: Condition is always false [redundant-expr]
...
if isinstance(t, object): # E: Condition is always true [redundant-expr]
...
if isinstance(t, list): # E: Condition is always false [redundant-expr]
...

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what happens when:

    if isinstance(t, int) and isinstance(1, int):


[builtins fixtures/isinstancelist.pyi]
Loading