Skip to content

Commit

Permalink
Reject unsafe operator method overrides
Browse files Browse the repository at this point in the history
  • Loading branch information
@JukkaL
JukkaL committed Sep 3, 2013
1 parent aeb8426 commit 9dd1086
Show file tree
Hide file tree
Showing 2 changed files with 52 additions and 0 deletions.
10 changes: 10 additions & 0 deletions mypy/checker.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -378,7 +378,17 @@ def check_override(self, override: FunctionLike, original: FunctionLike,
len(cast(Callable, original).arg_types) or
cast(Callable, override).min_args !=
cast(Callable, original).min_args):
# Use boolean variable to clarify code.
fail = False
if not is_subtype(override, original):
fail = True
elif (not isinstance(original, Overloaded) and
isinstance(override, Overloaded) and
name in nodes.reverse_op_methods.keys()):
# Operator method overrides cannot introduce overloading, as
# this could be unsafe with reverse operator methods.
fail = True
if fail:
self.msg.signature_incompatible_with_supertype(
name, supertype, node)
return
Expand Down
42 changes: 42 additions & 0 deletions mypy/test/data/check-classes.test
View file
Original file line number Diff line number Diff line change
Expand Up @@ -828,6 +828,48 @@ main: In class "D":
main, line 6: disjointclass constraint of class B disallows A as a base class


-- Operator methods
-- ----------------


[case testOperatorMethodOverrideIntroducingOverloading]
from typing import overload
class A:
def __add__(self, x: int) -> int: pass
class B(A):
@overload # E: Signature of "__add__" incompatible with supertype "A"
def __add__(self, x: int) -> int: pass
@overload
def __add__(self, x: str) -> str: pass

This comment has been minimized.

Sign in to view

Copy link
@elazarg

elazarg Sep 19, 2016

Contributor

In what sense is this code unsafe? Is there an example of otherwise-unrejected code that leads to runtime error using this operation?

This comment has been minimized.

Sign in to view

Copy link
@elazarg

elazarg Sep 19, 2016 •
edited
Loading

Contributor 
a = A()   # type: A
_ = a + 1  # type: int
_ = a + "a"  # type: Any. rejected

b = B()  # type: B
_ = b + 1  # type: int
_ = b + "a"  # type: str

ab = B()  # type: A
_ = ab + 1  # type: int, which is what happens at runtime
_ = ab + "a"  # type: int. false at runtime, but it is rejected anyway.

Are there any more possibilities?

This comment has been minimized.

Sign in to view

Copy link
@JukkaL

JukkaL Sep 19, 2016

Author Collaborator

Some of these may be false positives. I think that the case which is unsafe is something like this:

class A:
    def __add__(self, x: int) -> int: pass

class B(A):
    @overload  # E: Signature of "__add__" incompatible with supertype "A"
    def __add__(self, x: int) -> int: pass
    @overload
    def __add__(self, x: 'C') -> str: pass

class C:
    def __radd__(self, x: A) -> float: ...

def f(a: A) -> float:
    return a + C()  # okay?

f(B())  # ouch, returns str, not float

(Didn't verify if that actually misbehaves, but IIRC the unsoudness is something like that.)

This comment has been minimized.

Sign in to view

Copy link
@elazarg

elazarg Sep 19, 2016

Contributor

This code should not typecheck, regardless of B. The runtime dispatch will go to __add__, not __radd__, and it does not handle C. Perhaps you meant a reversed version?

This comment has been minimized.

Sign in to view

Copy link
@elazarg

elazarg Sep 19, 2016

Contributor

Here's a problematic version:

class A:
    def __add__(self, x: 'B') -> int: ...

class B: pass

class B1(B): pass

def f(a: A, b: B1) -> int:
    return a + b  # good?


class A1(A):
    @overload  # E: Signature of "__add__" incompatible with supertype "A"
    def __add__(self, x: B) -> int: pass
    @overload
    def __add__(self, x: B1) -> str: pass

f(A1(), B1())  # ouch, returns str, not int

This looks like a similar problem to covariance in method parameters.

I think we should test for the erroneous case, not mirror the checks in the code.

This comment has been minimized.

Sign in to view

Copy link
@elazarg

elazarg Sep 19, 2016 •
edited
Loading

Contributor

Additionally, this behavior has nothing to do with __add__ specifically, and if it's not __add__, mypy accepts the unsafe code.

This comment has been minimized.

Sign in to view

Copy link
@elazarg

elazarg Sep 19, 2016

Contributor

Yet another problematic version:

class A: pass

class B:
    def __radd__(self, a: A) -> int: ...

def f(a: A) -> int:
    return a + B()  # good?

class A1(A):
    def __add__(self, b: object) -> str: ...  # If we knew it's B we could have rejected

f(A1())  # ouch, returns str, not int

This code is unsafe, and accepted by mypy. But it has nothing to do with explicit overloading, and there's very little to do about it (except disallowing __radd__ on types that do not prohibit subtypes from declaring __add__); so mypy isn't sound.

This comment has been minimized.

Sign in to view

Copy link
@JukkaL

JukkaL Sep 20, 2016

Author Collaborator

It shouldn't be specific to __add__ -- the same issue also affects other operator methods that have r variants, such as __mul__. Regular methods should be fine. This is caused by how NotImplemented works.

And yes, mypy accepts some unsafe code, since IIRC there are some stdlib modules that motivated us to relax the checks that used to be stricter (can't remember which ones). My intention is to remove the remaining checks, as currently mypy is inconsistent (it complains about some things but not other things are pretty similar) and users will invariably be confused when mypy complains about these things, as the issue is very subtle.

So here's my thinking about this:

  • We'll remove most or all of the operator method specific checks at some point, and we'll live with the unsafety for the time being. I haven't seen any concrete examples where the unsafety has actually caused real-world trouble, but I've seen users getting confused by the safety checks multiple times.
  • In the future, consider having more specific checks that perhaps compare operator method signatures across all classes defined in the program instead of just within a single MRO list at a time. This would be a non-modular check, and thus it wouldn't be safe if there is dynamic loading of code etc., but I don't really care. So basically the idea is to only complain about this if we can actually highlight that some __[r]foo__ methods in classes X, Y and Z are not compatible, not about things that could potentially be unsafe. It would also be important to generate an error message that explains what is going on in more detail. I haven't worked out the details of this yet, though.

This comment has been minimized.

Sign in to view

Copy link
@elazarg

elazarg Sep 20, 2016

Contributor

Sounds good to me. Except for one thing - I see a problem with overloading+inheritance, and I see a (seemingly unfixable) problem with reversed operators. I don't understand where is the problem with the combination of overloading and reverse operators.
[out]
main: In class "B":

[case testOperatorMethodOverrideWideningArgumentType]
from typing import overload
class A:
def __add__(self, x: int) -> int: pass
class B(A):
def __add__(self, x: object) -> int: pass # E: Argument 1 of "__add__" incompatible with supertype "A"
[out]
main: In class "B":

[case testOperatorMethodOverrideNarrowingReturnType]
from typing import overload
class A:
def __add__(self, x: int) -> 'A': pass
class B(A):
def __add__(self, x: int) -> 'B': pass
[out]

[case testOperatorMethodOverrideWithDynamicallyTyped]
from typing import overload
class A:
def __add__(self, x: int) -> 'A': pass
class B(A):
def __add__(self, x): pass
[out]


-- Special cases
-- -------------

Expand Down

0 comments on commit 9dd1086

Please sign in to comment.