bloomberg / attrs-strict

Provides runtime validation of attributes specified in Python 'attr'-based data classes.
Apache License 2.0
52 stars 19 forks source link

Fix some `Callable` false positives #67

Closed RuRo closed 3 years ago

RuRo commented 3 years ago

Describe your changes

This PR fixes some false positive errors with Callable types.

1) According to PEP 484 "When used in a type hint, the expression None is considered equivalent to type(None)." Currently, this is not the case for return values of Callable type hints.

This leads to "funny" error messages like this:
```python
attrs_strict._error.CallableError: Error with: call_me . Expected Callable signature: typing.Callable[[int, int], NoneType] got: 
(a:int, b:int) -> None. None should be <class 'NoneType'>
```
from
```python
expected = Callable[[int, int], None]
def call_me(a:int, b:int) -> None:
    pass
```

2) Functions with default values for arguments weren't handled properly.
With this PR, the following function

    def foo(a: int, b: int = 123) -> str: ...
will also satisfy `Callable[[int], str]` and not just `Callable[[int, int], str]`.

3) Functions with keyword-only arguments weren't handled properly.
With this PR,

    def foo1(a: int, *, b: int = 123) -> str: ...
    def foo2(a: int, *, b: int) -> str: ...
 `foo1` will satisfy `Callable[[int], str]` but not `Callable[[int, int], str]`.
 `foo2` won't satisfy any of the above signatures (since `Callable` is positional-only).

Testing performed

I added test cases for each item in "Describe your changes" (both false positives and false negatives).

RuRo commented 3 years ago

I am assuming, that you aren't pinning the exact dependency versions for CI? Sphinx version 4 came out fairly recently and the docs are now apparently failing. I didn't touch anything documentation-related, so I doubt that this is my fault.

Edit: the problem seems to be that you are setting the language to python in docs/conf.py which is pretty weird considering this confval is supposed to be used for natural languages and translations (English/Japanese/etc). It's kind of surprising that it worked in the first place.

Edit Edit: You can either just pin the sphinx version to be <4 in setup.cfg like this:

[options.extras_require]
docs =
    Sphinx>=3,<4

or remove the

language = "python"

line from doc/conf.py. The default value for language doesn't perform any translation, which is probably what was happening with language = "python" anyway.

RuRo commented 3 years ago

@erikseulean wdyt?

RuRo commented 3 years ago

I've also fixed a few more Callable bugs, PTAL.

erikseulean commented 3 years ago

Sorry, for some reasons I missed this completely. Apologise. Will review today.

erikseulean commented 3 years ago

@RuRo could you please squash your commits and rebase ?

RuRo commented 3 years ago

@RuRo could you please squash your commits and rebase ?

I think the current changes are already rebased on top of the current master. If you can't squash the commits automatically when merging the PR for some reason, I can squash them after the current changes are approved.

RuRo commented 3 years ago

Sure. Just to make sure, by "squash the test and implementation commits of each kind" do you mean

1) squash all the tests together and all the implementation commits together 2) squash each implementation commit with its respective test commit 3) squash all the commits

?

RuRo commented 3 years ago

I've decided that you probably meant (2) and did that.

RuRo commented 3 years ago

It's that time of the week, where I go around all the PRs I am participating in and ping everyone!

@erikseulean @saytosid can I do anything to get this PR accepted?

RuRo commented 3 years ago

Oof. I haven't noticed, that you still support python 2.7. I'll fix the syntax errors.

As for the documentation failing, see my earlier comment. That problem wasn't caused by me, but by your unpinned sphinx version getting updated. You'll need to either pin the version or change the language setting.

(I can fix it for you in this PR if you want, but you need to choose, which option do you prefer)

gaborbernat commented 3 years ago

Change the language settings, feel free to add it here or even better a parallel pr 🙏