From b46f970c0dea54b2ed568ecb1a2ec359ad3e479e Mon Sep 17 00:00:00 2001 From: Michael Lee Date: Tue, 5 Jul 2016 16:07:51 -0700 Subject: [PATCH 1/6] Make type comparisons work This commit introduces a workaround to fix the bug discussed in https://github.com/python/mypy/pull/1787 Previously, code where you compared two types (eg `int == int`) would cause mypy to incorrectly report a "too few arguments" error. --- mypy/checkmember.py | 16 +++++++++----- test-data/unit/check-classes.test | 36 +++++++++++++++++++++++++++++++ test-data/unit/fixtures/args.py | 3 +++ 3 files changed, 50 insertions(+), 5 deletions(-) diff --git a/mypy/checkmember.py b/mypy/checkmember.py index cc0be2058fbf..1b3352f47ec2 100644 --- a/mypy/checkmember.py +++ b/mypy/checkmember.py @@ -91,10 +91,15 @@ def analyze_member_access(name: str, typ: Type, node: Context, is_lvalue: bool, if isinstance(ret_type, TupleType): ret_type = ret_type.fallback if isinstance(ret_type, Instance): - result = analyze_class_attribute_access(ret_type, name, node, is_lvalue, - builtin_type, not_ready_callback, msg) - if result: - return result + if name not in {'__eq__', '__ne__'}: + # We skip here so that when mypy sees `type(foo) == type(bar)`, it doesn't try + # and typecheck against `foo.__eq__`. This workaround makes sure that mypy falls + # through and uses `foo.__class__.__eq__`.instead. See the bug discussed in + # https://github.com/python/mypy/pull/1787 for more info. + result = analyze_class_attribute_access(ret_type, name, node, is_lvalue, + builtin_type, not_ready_callback, msg) + if result: + return result # Look up from the 'type' type. return analyze_member_access(name, typ.fallback, node, is_lvalue, is_super, builtin_type, not_ready_callback, msg, @@ -121,7 +126,8 @@ def analyze_member_access(name: str, typ: Type, node: Context, is_lvalue: bool, elif isinstance(typ.item, TypeVarType): if isinstance(typ.item.upper_bound, Instance): item = typ.item.upper_bound - if item: + if item and name not in {'__eq__', '__ne__'}: + # See comment above for why __eq__ and __ne__ are skipped result = analyze_class_attribute_access(item, name, node, is_lvalue, builtin_type, not_ready_callback, msg) if result: diff --git a/test-data/unit/check-classes.test b/test-data/unit/check-classes.test index 513478259d54..7e5396e8e1db 100644 --- a/test-data/unit/check-classes.test +++ b/test-data/unit/check-classes.test @@ -2006,3 +2006,39 @@ reveal_type(User) # E: Revealed type is 'builtins.type' [builtins fixtures/args.py] [out] +[case testTypeTypeComparisonWorks] +class User: pass + +User == User +User == type(User()) +type(User()) == User +type(User()) == type(User()) + +User != User +User != type(User()) +type(User()) != User +type(User()) != type(User()) + +int == int +int == type(3) +type(3) == int +type(3) == type(3) + +int != int +int != type(3) +type(3) != int +type(3) != type(3) + +User is User +User is type(User) +type(User) is User +type(User) is type(User) + +int is int +int is type(3) +type(3) is int +type(3) is type(3) +[builtins fixtures/args.py] +[out] + + diff --git a/test-data/unit/fixtures/args.py b/test-data/unit/fixtures/args.py index dd7d9ea61c6f..5f628bb2d00b 100644 --- a/test-data/unit/fixtures/args.py +++ b/test-data/unit/fixtures/args.py @@ -8,12 +8,15 @@ class object: def __init__(self) -> None: pass + def __eq__(self, o: object) -> bool: pass + def __ne__(self, o: object) -> bool: pass class type: @overload def __init__(self, o: object) -> None: pass @overload def __init__(self, name: str, bases: Tuple[type, ...], dict: Dict[str, Any]) -> None: pass + def __call__(self, *args: Any, **kwargs: Any) -> Any: pass class tuple(Iterable[Tco], Generic[Tco]): pass class dict(Generic[T, S]): pass From e39d0b43be7b2867ccfa278ea40c237b3d623fbd Mon Sep 17 00:00:00 2001 From: Michael Lee Date: Tue, 5 Jul 2016 16:07:51 -0700 Subject: [PATCH 2/6] Make type comparisons work This commit introduces a workaround to fix the bug discussed in https://github.com/python/mypy/pull/1787 Previously, code where you compared two types (eg `int == int`) would cause mypy to incorrectly report a "too few arguments" error. --- mypy/checkmember.py | 16 +++++++++----- test-data/unit/check-classes.test | 36 +++++++++++++++++++++++++++++++ test-data/unit/fixtures/args.py | 3 +++ 3 files changed, 50 insertions(+), 5 deletions(-) diff --git a/mypy/checkmember.py b/mypy/checkmember.py index cc0be2058fbf..1b3352f47ec2 100644 --- a/mypy/checkmember.py +++ b/mypy/checkmember.py @@ -91,10 +91,15 @@ def analyze_member_access(name: str, typ: Type, node: Context, is_lvalue: bool, if isinstance(ret_type, TupleType): ret_type = ret_type.fallback if isinstance(ret_type, Instance): - result = analyze_class_attribute_access(ret_type, name, node, is_lvalue, - builtin_type, not_ready_callback, msg) - if result: - return result + if name not in {'__eq__', '__ne__'}: + # We skip here so that when mypy sees `type(foo) == type(bar)`, it doesn't try + # and typecheck against `foo.__eq__`. This workaround makes sure that mypy falls + # through and uses `foo.__class__.__eq__`.instead. See the bug discussed in + # https://github.com/python/mypy/pull/1787 for more info. + result = analyze_class_attribute_access(ret_type, name, node, is_lvalue, + builtin_type, not_ready_callback, msg) + if result: + return result # Look up from the 'type' type. return analyze_member_access(name, typ.fallback, node, is_lvalue, is_super, builtin_type, not_ready_callback, msg, @@ -121,7 +126,8 @@ def analyze_member_access(name: str, typ: Type, node: Context, is_lvalue: bool, elif isinstance(typ.item, TypeVarType): if isinstance(typ.item.upper_bound, Instance): item = typ.item.upper_bound - if item: + if item and name not in {'__eq__', '__ne__'}: + # See comment above for why __eq__ and __ne__ are skipped result = analyze_class_attribute_access(item, name, node, is_lvalue, builtin_type, not_ready_callback, msg) if result: diff --git a/test-data/unit/check-classes.test b/test-data/unit/check-classes.test index 513478259d54..7e5396e8e1db 100644 --- a/test-data/unit/check-classes.test +++ b/test-data/unit/check-classes.test @@ -2006,3 +2006,39 @@ reveal_type(User) # E: Revealed type is 'builtins.type' [builtins fixtures/args.py] [out] +[case testTypeTypeComparisonWorks] +class User: pass + +User == User +User == type(User()) +type(User()) == User +type(User()) == type(User()) + +User != User +User != type(User()) +type(User()) != User +type(User()) != type(User()) + +int == int +int == type(3) +type(3) == int +type(3) == type(3) + +int != int +int != type(3) +type(3) != int +type(3) != type(3) + +User is User +User is type(User) +type(User) is User +type(User) is type(User) + +int is int +int is type(3) +type(3) is int +type(3) is type(3) +[builtins fixtures/args.py] +[out] + + diff --git a/test-data/unit/fixtures/args.py b/test-data/unit/fixtures/args.py index dd7d9ea61c6f..5f628bb2d00b 100644 --- a/test-data/unit/fixtures/args.py +++ b/test-data/unit/fixtures/args.py @@ -8,12 +8,15 @@ class object: def __init__(self) -> None: pass + def __eq__(self, o: object) -> bool: pass + def __ne__(self, o: object) -> bool: pass class type: @overload def __init__(self, o: object) -> None: pass @overload def __init__(self, name: str, bases: Tuple[type, ...], dict: Dict[str, Any]) -> None: pass + def __call__(self, *args: Any, **kwargs: Any) -> Any: pass class tuple(Iterable[Tco], Generic[Tco]): pass class dict(Generic[T, S]): pass From c9c70f93cb6f3d0b0fc83ac5f576c74d8cac8720 Mon Sep 17 00:00:00 2001 From: Michael Lee Date: Thu, 7 Jul 2016 11:49:24 -0700 Subject: [PATCH 3/6] Make first attempt at making checks general purpose --- mypy/checkmember.py | 17 +++++++++-------- mypy/nodes.py | 3 +++ 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/mypy/checkmember.py b/mypy/checkmember.py index 1b3352f47ec2..9fa574490569 100644 --- a/mypy/checkmember.py +++ b/mypy/checkmember.py @@ -8,7 +8,8 @@ DeletedType, NoneTyp, TypeType ) from mypy.nodes import TypeInfo, FuncBase, Var, FuncDef, SymbolNode, Context -from mypy.nodes import ARG_POS, ARG_STAR, ARG_STAR2, function_type, Decorator, OverloadedFuncDef +from mypy.nodes import ARG_POS, ARG_STAR, ARG_STAR2, all_operator_methods +from mypy.nodes import function_type, Decorator, OverloadedFuncDef from mypy.messages import MessageBuilder from mypy.maptype import map_instance_to_supertype from mypy.expandtype import expand_type_by_instance @@ -91,11 +92,11 @@ def analyze_member_access(name: str, typ: Type, node: Context, is_lvalue: bool, if isinstance(ret_type, TupleType): ret_type = ret_type.fallback if isinstance(ret_type, Instance): - if name not in {'__eq__', '__ne__'}: - # We skip here so that when mypy sees `type(foo) == type(bar)`, it doesn't try - # and typecheck against `foo.__eq__`. This workaround makes sure that mypy falls - # through and uses `foo.__class__.__eq__`.instead. See the bug discussed in - # https://github.com/python/mypy/pull/1787 for more info. + if name not in all_operator_methods: + # We skip here so that when mypy sees comparisons like `type(foo) == type(bar)`, + # it doesn't try and typecheck against `foo.__eq__`. This workaround makes sure + # that mypy falls through and uses `foo.__class__.__eq__`.instead. See the bug + # discussed in https://github.com/python/mypy/pull/1787 for more info. result = analyze_class_attribute_access(ret_type, name, node, is_lvalue, builtin_type, not_ready_callback, msg) if result: @@ -126,8 +127,8 @@ def analyze_member_access(name: str, typ: Type, node: Context, is_lvalue: bool, elif isinstance(typ.item, TypeVarType): if isinstance(typ.item.upper_bound, Instance): item = typ.item.upper_bound - if item and name not in {'__eq__', '__ne__'}: - # See comment above for why __eq__ and __ne__ are skipped + if item and name not in all_operator_methods: + # See comment above for why operator methods are skipped result = analyze_class_attribute_access(item, name, node, is_lvalue, builtin_type, not_ready_callback, msg) if result: diff --git a/mypy/nodes.py b/mypy/nodes.py index b068715d3816..c3bcb3d232a7 100644 --- a/mypy/nodes.py +++ b/mypy/nodes.py @@ -1306,6 +1306,9 @@ def accept(self, visitor: NodeVisitor[T]) -> T: normal_from_reverse_op = dict((m, n) for n, m in reverse_op_methods.items()) reverse_op_method_set = set(reverse_op_methods.values()) +all_operator_methods = (inplace_operator_methods + | set(op_methods.values()) + | set(reverse_op_methods.values())) class OpExpr(Expression): From 53e001ee5871c0c195a08038ba0be29aab0723b9 Mon Sep 17 00:00:00 2001 From: Michael Lee Date: Thu, 7 Jul 2016 17:33:28 -0700 Subject: [PATCH 4/6] Replace hack with a more principled check After talking with Guido, I realized Python special-cases literally the operators (==, +, etc), NOT the magic methods themselves. So, that means we need to special-case "a == b", but not "a.__eq__(b)". As a result, checking to see if the method name was a magic method or not was the wrong thing to do -- if the name of the method is "__eq__", there's no way to know if the code was originally "foo == bar" or "foo.__eq__(bar)". So, rather then trying to do something with the operator name, I can instead check and see if the node is an OpExpr or ComparisonExpr node vs a CallExpr node. --- mypy/checkmember.py | 25 +++++++++++++++++-------- mypy/nodes.py | 3 --- test-data/unit/check-classes.test | 5 +++++ test-data/unit/fixtures/args.py | 3 ++- 4 files changed, 24 insertions(+), 12 deletions(-) diff --git a/mypy/checkmember.py b/mypy/checkmember.py index 9fa574490569..ccb00f52fab6 100644 --- a/mypy/checkmember.py +++ b/mypy/checkmember.py @@ -8,7 +8,7 @@ DeletedType, NoneTyp, TypeType ) from mypy.nodes import TypeInfo, FuncBase, Var, FuncDef, SymbolNode, Context -from mypy.nodes import ARG_POS, ARG_STAR, ARG_STAR2, all_operator_methods +from mypy.nodes import ARG_POS, ARG_STAR, ARG_STAR2, OpExpr, ComparisonExpr from mypy.nodes import function_type, Decorator, OverloadedFuncDef from mypy.messages import MessageBuilder from mypy.maptype import map_instance_to_supertype @@ -92,11 +92,20 @@ def analyze_member_access(name: str, typ: Type, node: Context, is_lvalue: bool, if isinstance(ret_type, TupleType): ret_type = ret_type.fallback if isinstance(ret_type, Instance): - if name not in all_operator_methods: - # We skip here so that when mypy sees comparisons like `type(foo) == type(bar)`, - # it doesn't try and typecheck against `foo.__eq__`. This workaround makes sure - # that mypy falls through and uses `foo.__class__.__eq__`.instead. See the bug - # discussed in https://github.com/python/mypy/pull/1787 for more info. + if not isinstance(node, (OpExpr, ComparisonExpr)): + # When Python sees an operator (eg `3 == 4`), it automatically translates that + # into something like `int.__eq__(3, 4)` instead of `(3).__eq__(4)` as an + # optimation. + # + # While it normally it doesn't matter which of the two versions are used, it + # does cause inconsistencies when working with classes. For example, translating + # `int == int` to `int.__eq__(int)` would not work since `int.__eq__` is meant to + # compare two int _instances_. What we really want is `type(int).__eq__`, which + # is meant to compare two types or classes. + # + # This check makes sure that when we encounter an operator, we skip looking up + # the corresponding method in the current instance to avoid this edge case. + # See https://github.com/python/mypy/pull/1787 for more info. result = analyze_class_attribute_access(ret_type, name, node, is_lvalue, builtin_type, not_ready_callback, msg) if result: @@ -127,8 +136,8 @@ def analyze_member_access(name: str, typ: Type, node: Context, is_lvalue: bool, elif isinstance(typ.item, TypeVarType): if isinstance(typ.item.upper_bound, Instance): item = typ.item.upper_bound - if item and name not in all_operator_methods: - # See comment above for why operator methods are skipped + if item and not isinstance(node, (OpExpr, ComparisonExpr)): + # See comment above for why operators are skipped result = analyze_class_attribute_access(item, name, node, is_lvalue, builtin_type, not_ready_callback, msg) if result: diff --git a/mypy/nodes.py b/mypy/nodes.py index c3bcb3d232a7..b068715d3816 100644 --- a/mypy/nodes.py +++ b/mypy/nodes.py @@ -1306,9 +1306,6 @@ def accept(self, visitor: NodeVisitor[T]) -> T: normal_from_reverse_op = dict((m, n) for n, m in reverse_op_methods.items()) reverse_op_method_set = set(reverse_op_methods.values()) -all_operator_methods = (inplace_operator_methods - | set(op_methods.values()) - | set(reverse_op_methods.values())) class OpExpr(Expression): diff --git a/test-data/unit/check-classes.test b/test-data/unit/check-classes.test index 7e5396e8e1db..5670f9cd346a 100644 --- a/test-data/unit/check-classes.test +++ b/test-data/unit/check-classes.test @@ -2038,7 +2038,12 @@ int is int int is type(3) type(3) is int type(3) is type(3) + +int.__eq__(int) +int.__eq__(3, 4) [builtins fixtures/args.py] [out] +main:33: error: Too few arguments for "__eq__" of "int" +main:33: error: Unsupported operand types for == ("int" and "int") diff --git a/test-data/unit/fixtures/args.py b/test-data/unit/fixtures/args.py index 5f628bb2d00b..b084fc6c68e5 100644 --- a/test-data/unit/fixtures/args.py +++ b/test-data/unit/fixtures/args.py @@ -21,7 +21,8 @@ def __call__(self, *args: Any, **kwargs: Any) -> Any: pass class tuple(Iterable[Tco], Generic[Tco]): pass class dict(Generic[T, S]): pass -class int: pass +class int: + def __eq__(self, o: object) -> bool: pass class str: pass class bool: pass class function: pass From 4ca2038d45650939b64cabeee4f54d3725176029 Mon Sep 17 00:00:00 2001 From: Michael Lee Date: Fri, 8 Jul 2016 15:45:54 -0700 Subject: [PATCH 5/6] Respond to pull request feedback --- mypy/checkexpr.py | 12 ++++++------ mypy/checkmember.py | 22 +++++++++++----------- 2 files changed, 17 insertions(+), 17 deletions(-) diff --git a/mypy/checkexpr.py b/mypy/checkexpr.py index a9aeac69c3e7..17467825f3a3 100644 --- a/mypy/checkexpr.py +++ b/mypy/checkexpr.py @@ -286,8 +286,8 @@ def check_call(self, callee: Type, args: List[Node], callee) elif isinstance(callee, Instance): call_function = analyze_member_access('__call__', callee, context, - False, False, self.named_type, self.not_ready_callback, - self.msg) + False, False, False, self.named_type, + self.not_ready_callback, self.msg) return self.check_call(call_function, args, arg_kinds, context, arg_names, callable_node, arg_messages) elif isinstance(callee, TypeVarType): @@ -861,7 +861,7 @@ def analyze_ordinary_member_access(self, e: MemberExpr, else: # This is a reference to a non-module attribute. return analyze_member_access(e.name, self.accept(e.expr), e, - is_lvalue, False, + is_lvalue, False, False, self.named_type, self.not_ready_callback, self.msg) def analyze_external_member_access(self, member: str, base_type: Type, @@ -870,7 +870,7 @@ def analyze_external_member_access(self, member: str, base_type: Type, refer to private definitions. Return the result type. """ # TODO remove; no private definitions in mypy - return analyze_member_access(member, base_type, context, False, False, + return analyze_member_access(member, base_type, context, False, False, False, self.named_type, self.not_ready_callback, self.msg) def visit_int_expr(self, e: IntExpr) -> Type: @@ -1008,7 +1008,7 @@ def check_op_local(self, method: str, base_type: Type, arg: Node, Return tuple (result type, inferred operator method type). """ - method_type = analyze_member_access(method, base_type, context, False, False, + method_type = analyze_member_access(method, base_type, context, False, False, True, self.named_type, self.not_ready_callback, local_errors) return self.check_call(method_type, [arg], [nodes.ARG_POS], context, arg_messages=local_errors) @@ -1434,7 +1434,7 @@ def analyze_super(self, e: SuperExpr, is_lvalue: bool) -> Type: if not self.chk.typing_mode_full(): return AnyType() return analyze_member_access(e.name, self_type(e.info), e, - is_lvalue, True, + is_lvalue, True, False, self.named_type, self.not_ready_callback, self.msg, base) else: diff --git a/mypy/checkmember.py b/mypy/checkmember.py index ccb00f52fab6..e336d223d5a2 100644 --- a/mypy/checkmember.py +++ b/mypy/checkmember.py @@ -20,7 +20,7 @@ def analyze_member_access(name: str, typ: Type, node: Context, is_lvalue: bool, - is_super: bool, + is_super: bool, is_operator: bool, builtin_type: Callable[[str], Instance], not_ready_callback: Callable[[str, Context], None], msg: MessageBuilder, override_info: TypeInfo = None, @@ -76,15 +76,15 @@ def analyze_member_access(name: str, typ: Type, node: Context, is_lvalue: bool, elif isinstance(typ, UnionType): # The base object has dynamic type. msg.disable_type_names += 1 - results = [analyze_member_access(name, subtype, node, is_lvalue, - is_super, builtin_type, not_ready_callback, msg) + results = [analyze_member_access(name, subtype, node, is_lvalue, is_super, + is_operator, builtin_type, not_ready_callback, msg) for subtype in typ.items] msg.disable_type_names -= 1 return UnionType.make_simplified_union(results) elif isinstance(typ, TupleType): # Actually look up from the fallback instance type. - return analyze_member_access(name, typ.fallback, node, is_lvalue, - is_super, builtin_type, not_ready_callback, msg) + return analyze_member_access(name, typ.fallback, node, is_lvalue, is_super, + is_operator, builtin_type, not_ready_callback, msg) elif isinstance(typ, FunctionLike) and typ.is_type_obj(): # Class attribute. # TODO super? @@ -92,7 +92,7 @@ def analyze_member_access(name: str, typ: Type, node: Context, is_lvalue: bool, if isinstance(ret_type, TupleType): ret_type = ret_type.fallback if isinstance(ret_type, Instance): - if not isinstance(node, (OpExpr, ComparisonExpr)): + if not is_operator: # When Python sees an operator (eg `3 == 4`), it automatically translates that # into something like `int.__eq__(3, 4)` instead of `(3).__eq__(4)` as an # optimation. @@ -112,18 +112,18 @@ def analyze_member_access(name: str, typ: Type, node: Context, is_lvalue: bool, return result # Look up from the 'type' type. return analyze_member_access(name, typ.fallback, node, is_lvalue, is_super, - builtin_type, not_ready_callback, msg, + is_operator, builtin_type, not_ready_callback, msg, report_type=report_type) else: assert False, 'Unexpected type {}'.format(repr(ret_type)) elif isinstance(typ, FunctionLike): # Look up from the 'function' type. return analyze_member_access(name, typ.fallback, node, is_lvalue, is_super, - builtin_type, not_ready_callback, msg, + is_operator, builtin_type, not_ready_callback, msg, report_type=report_type) elif isinstance(typ, TypeVarType): return analyze_member_access(name, typ.upper_bound, node, is_lvalue, is_super, - builtin_type, not_ready_callback, msg, + is_operator, builtin_type, not_ready_callback, msg, report_type=report_type) elif isinstance(typ, DeletedType): msg.deleted_as_rvalue(typ, node) @@ -136,7 +136,7 @@ def analyze_member_access(name: str, typ: Type, node: Context, is_lvalue: bool, elif isinstance(typ.item, TypeVarType): if isinstance(typ.item.upper_bound, Instance): item = typ.item.upper_bound - if item and not isinstance(node, (OpExpr, ComparisonExpr)): + if item and not is_operator: # See comment above for why operators are skipped result = analyze_class_attribute_access(item, name, node, is_lvalue, builtin_type, not_ready_callback, msg) @@ -144,7 +144,7 @@ def analyze_member_access(name: str, typ: Type, node: Context, is_lvalue: bool, return result fallback = builtin_type('builtins.type') return analyze_member_access(name, fallback, node, is_lvalue, is_super, - builtin_type, not_ready_callback, msg, + is_operator, builtin_type, not_ready_callback, msg, report_type=report_type) return msg.has_no_attr(report_type, name, node) From bf351a4bcdc4e81c7c961280d80585f30fcb94b8 Mon Sep 17 00:00:00 2001 From: Michael Lee Date: Fri, 8 Jul 2016 16:23:33 -0700 Subject: [PATCH 6/6] Fix ci regression --- mypy/checkmember.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mypy/checkmember.py b/mypy/checkmember.py index 991fa8cab62b..36768e0830ea 100644 --- a/mypy/checkmember.py +++ b/mypy/checkmember.py @@ -81,7 +81,7 @@ def analyze_member_access(name: str, elif isinstance(typ, NoneTyp): # The only attribute NoneType has are those it inherits from object return analyze_member_access(name, builtin_type('builtins.object'), node, is_lvalue, - is_super, builtin_type, not_ready_callback, msg, + is_super, is_operator, builtin_type, not_ready_callback, msg, report_type=report_type) elif isinstance(typ, UnionType): # The base object has dynamic type.