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.

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

require decorator removes type annotations for mypy #93

Closed
jamescasbon opened this issue Jan 22, 2019 · 12 comments
Closed

require decorator removes type annotations for mypy #93

jamescasbon opened this issue Jan 22, 2019 · 12 comments

Comments

@jamescasbon
Copy link

jamescasbon commented Jan 22, 2019

$ cat ic.py  
from icontract import require

@require(lambda x: x > 0)
def f1(x: int): return x

def f2(x: int): return 0

f1("this is wrong")
f2("this is too")

$ mypy ic.py              
ic.py:9: error: Argument 1 to "f2" has incompatible type "str"; expected "int"

Both function calls are invalid types but mypy only sees the call as f2 as wrong. The decorator loses the type annotations?

Related python/mypy#3157

@mristin
Copy link
Collaborator

mristin commented Jan 22, 2019

Hi James,
Yes, that is unfortunate. I'm still hoping that the issue will be fixed in mypy. You can circumvent it by defining types in a .pyi file (unless I missed some new development?), but that is a very ugly solution.

Please let me know if you find a better workaround.

@mristin
Copy link
Collaborator

mristin commented Jan 22, 2019

(Let me do a couple of experiments. I think I misread python/mypy#3157)

@mristin
Copy link
Collaborator

mristin commented Jan 22, 2019

Hi @jamescasbon

My bad, I indeed misread python/mypy#3157. Mypy indeed supports decorators which do not change the signature of the function.

Would you mind reviewing the following snippet for me? I assume that I simply need to change the argument type and return type of icontract.require and icontract.ensure argument to fix the issue?

experiment.py:

from typing import TypeVar, Any, Callable

CallableT = TypeVar('CallableT', bound='Callable')

class some_decorator:
    def __init__(self, some_param: int)->None:
        return

    def __call__(self, func: CallableT) -> CallableT:
        def result(*args, **kwargs):
            print("hello")
            return func(*args, **kwargs)

        return result  # type: ignore

@some_decorator(some_param="oi")
def f(x: int, y: str)->str:
    return str(x + int(y))

f(x="oi", y=1)

I get the expected errors:

$ mypy experiment.py 
experiment.py:16: error: Argument "some_param" to "some_decorator" has incompatible type "str"; expected "int"
experiment.py:20: error: Argument "x" to "f" has incompatible type "str"; expected "int"
experiment.py:20: error: Argument "y" to "f" has incompatible type "int"; expected "str"

@mristin
Copy link
Collaborator

mristin commented Jan 22, 2019

@jamescasbon the PR is up: #94

If you have a spare minute, could you please check out the branch and double-check that it fixes the issue? (You might also want to review the relevant test in tests/test_mypy_decorators.py.)

@jamescasbon
Copy link
Author

Amazing work, yes the branch works with mypy now 👍

Thanks for the quick turnaround.

@jamescasbon
Copy link
Author

Ah, i redid the check with mypy --strict

The require decorator now gives this error:

ic.py:3: warning: Returning Any from function declared to return "bool"

I do think this PR is a lot better - but would be very nice if we could run with strict

@mristin
Copy link
Collaborator

mristin commented Jan 23, 2019 via email

@mristin
Copy link
Collaborator

mristin commented Jan 23, 2019

Hi @jamescasbon,
I think this is an issue with how mypy handles lambdas.

icontract.require's signature is:

class require:
    def __init__(self,
                 condition: Callable[..., bool],
                 description: Optional[str] = None,
                 a_repr: reprlib.Repr = icontract._globals.aRepr,
                 enabled: bool = __debug__,
                 error: Optional[Union[Callable[..., Exception], type]] = None) -> None:
                     ...

As you can see, it expects condition to be of type Callable[..., bool], whereas lambdas are inferred as Callable[..., Any] since the type of the input argument is not known (see https://mypy.readthedocs.io/en/latest/kinds_of_types.html#callable-types-and-lambdas).

If you replace it with def ..., everything works, see the commit:
928d749

Now, I'm not really sure what is the best approach. I expect that typing the condition as Callable[..., Any] would confuse the user, but on the other hand would make the mypy --strict pass. I'd prefer to stick with the clear type annotations rather than make mypy --strict pass. However, if mypy --strict is something many people are using then it is a barrier I would not like icontract to introduce.

Please double-check the latest commit and if everything is clear/working OK, let me bump the patch version.

@jamescasbon
Copy link
Author

Hmmmm. To be honest I don't know how many people are using mypy --strict. I am using DBC + mypy to get correctness so my aim would be strict. 3.6 can type lambdas but not anonymous ones :/

Seems to me if you expect condition to be a lambda you should match the type of a lambda but I appreciate it is not an easy choice.

@mristin
Copy link
Collaborator

mristin commented Jan 23, 2019

After some thinking I came to the conclusion that it makes sense to have condition return Any rather than bool. This is necessary to comply with pythonic condition checks:

x = []  # type: List[int]
if x:
    ...

Since this is so pervasive in Python, there is no reason why suddenly icontract would follow a "boolean-first" policy.

Please have a look at 3b35de1 and let me know if we are good to merge it into the master.

Thanks again for all the help!

@mristin
Copy link
Collaborator

mristin commented Jan 25, 2019

(@jamescasbon -- just a kind reminder; I'd like to wait for you to check that the latest commit fixes your problem before I bump the patch version.)

@jamescasbon
Copy link
Author

Thanks very much this looks great. I think you should ship it 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants