agronholm / typeguard

Run-time type checker for Python
Other
1.55k stars 114 forks source link

Incorrect checking of protocol methods with `**kwargs` but no `*args` #465

Closed comex closed 1 month ago

comex commented 6 months ago

Things to check first

Typeguard version

4.3.0

Python version

3.12

What happened?

The below example should pass checking, but instead gives:

Traceback (most recent call last):
  File "/Users/comex/src/typeguard/test.py", line 12, in <module>
    check_type(C(), P)
  File "/Users/comex/src/typeguard/src/typeguard/_functions.py", line 106, in check_type
    check_type_internal(value, expected_type, memo)
  File "/Users/comex/src/typeguard/src/typeguard/_checkers.py", line 861, in check_type_internal
    checker(value, origin_type, args, memo)
  File "/Users/comex/src/typeguard/src/typeguard/_checkers.py", line 729, in check_protocol
    raise TypeCheckError(
typeguard.TypeCheckError: __main__.C is not compatible with the P protocol because its 'foo' method has too few arguments in its declaration; expected 2 but 1 argument(s) declared

How can we reproduce the bug?

from typeguard import check_type
from typing import Protocol

class P(Protocol):
    def foo(self, **kwargs):
        ...

class C:
    def foo(self, **kwargs):
        pass

check_type(C(), P)
agronholm commented 6 months ago

I suspect that this isn't the only issue with the new protocol check. I'll expand the test suite for that a bit and fix any problems I find in the process.

jace commented 5 months ago

The new protocol check implementation also doesn't work with callback protocols because it checks against the type. This fails with typeguard.TypeCheckError: function is not compatible with the MyProtocol protocol because it has no attribute named '__weakref__'

agronholm commented 5 months ago

The new protocol check implementation also doesn't work with callback protocols because it checks against the type. This fails with typeguard.TypeCheckError: function is not compatible with the MyProtocol protocol because it has no attribute named '__weakref__'

Would you mind writing an example for me so I could make sure it works in the next release?

jkulhanek commented 5 months ago

Having the same issue.

agronholm commented 5 months ago

Having the same issue.

Then you can just subscribe to the issue instead of pinging everyone.

jkulhanek commented 5 months ago

Having the same issue.

Then you can just subscribe to the issue instead of pinging everyone.

So sorry, I will keep it in mind for next time.

bersbersbers commented 5 months ago

Would you mind writing an example for me so I could make sure it works in the next release?

I have one.

test_bug.py:

import typing

import pytest

class CallableFixture(typing.Protocol):
    def __call__(self) -> None: ...

@pytest.fixture(name="callable_fixture")
def make_callable_fixture() -> CallableFixture:
    def callable_fixture() -> None:
        print("Works!")

    return callable_fixture

def test_bug(callable_fixture: CallableFixture) -> None:
    callable_fixture()

Then run pytest -s to see that the code works and pytest --typeguard-packages=test_bug for the failure:

================================================================================= test session starts =================================================================================
platform win32 -- Python 3.12.3, pytest-8.2.1, pluggy-1.5.0
PySide6 6.7.1 -- Qt runtime 6.7.1 -- Qt compiled 6.7.1
rootdir: C:\Code\Bug
plugins: pylama-8.4.1, cov-5.0.0, order-1.2.1, qt-4.4.0, typeguard-4.3.0
collected 1 item

test_bug.py E                                                                                                                                                                    [100%]

======================================================================================= ERRORS ======================================================================================== 
_____________________________________________________________________________ ERROR at setup of test_bug ______________________________________________________________________________ 

    @pytest.fixture(name="callable_fixture")
    def make_callable_fixture() -> CallableFixture:
        def callable_fixture() -> None:
            print("Works!")

>       return callable_fixture

test_bug.py:13:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
C:\Code\.venv\Lib\site-packages\typeguard\_functions.py:165: in check_return_type
    check_type_internal(retval, annotation, memo)
C:\Code\.venv\Lib\site-packages\typeguard\_checkers.py:861: in check_type_internal
    checker(value, origin_type, args, memo)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

[...]

            # Check that all required non-callable members are present
            for attrname in expected_noncallable_members:
                # TODO: implement assignability checks for non-callable members
                if attrname not in subject_annotations and not hasattr(subject, attrname):
>                   raise TypeCheckError(
                        f"is not compatible with the {origin_type.__qualname__} protocol "
                        f"because it has no attribute named {attrname!r}"
                    )
E                   typeguard.TypeCheckError: the return value (function) is not compatible with the CallableFixture protocol because it has no attribute named '__weakref__'

C:\Code\.venv\Lib\site-packages\typeguard\_checkers.py:738: TypeCheckError
=============================================================================== short test summary info ===============================================================================     
ERROR test_bug.py::test_bug - typeguard.TypeCheckError: the return value (function) is not compatible with the CallableFixture protocol because it has no attribute named '__weakref__'     
================================================================================== 1 error in 0.35s ===================================================================================     

I guess this can be simplified to work without pytest, but this is what I hit in 4.3.0 and what makes me pin typeguard<4.3.0 for now (despite looking forward to the fix of https://github.com/agronholm/typeguard/issues/457).

agronholm commented 5 months ago

I'm already writing improved compatibility tests involving *args and **kwargs, and the fix will be in the next patch release.

bersbersbers commented 5 months ago

I'm already writing improved compatibility tests involving *args and **kwargs, and the fix will be in the next patch release.

Awesome, thanks. Just to avoid potential confusion: my example code in https://github.com/agronholm/typeguard/issues/465#issuecomment-2143857594 is for the sub-issue reported in https://github.com/agronholm/typeguard/issues/465#issuecomment-2141385044, not for the original one reported in https://github.com/agronholm/typeguard/issues/465#issue-2324380426.

jace commented 5 months ago

The new protocol check implementation also doesn't work with callback protocols because it checks against the type. This fails with typeguard.TypeCheckError: function is not compatible with the MyProtocol protocol because it has no attribute named '__weakref__'

I tried putting __weakref__ in the list of ignored attributes, but that is insufficient. Example:

class GetUserProtocol(Protocol):
    usermap: dict[str, str]
    def __call__(self, user: str) -> models.User: ...

@pytest.fixture
def getuser(request: pytest.FixtureRequest) -> GetUserProtocol:
    """Get a user fixture by their name (for descriptive names in behaviour tests)."""
    usermap = {"Twoflower": 'user_twoflower', "Rincewind": 'user_rincewind'}

    def func(user: str) -> models.User:
        if user not in usermap:
            pytest.fail(f"No user fixture named {user}")
        return request.getfixturevalue(usermap[user])

    func = cast(GetUserProtocol, func)
    func.usermap = usermap
    return func

This will fail because function's type has no attribute named usermap.

agronholm commented 5 months ago

I will take all this into account in the tests going forward.

bersbersbers commented 5 months ago

I tried putting __weakref__ in the list of ignored attributes

That helps my use cases, thank you!

bersbersbers commented 5 months ago

That helps my use cases, thank you!

Is there any way to apply this from the type-checked code, by monkey patching or similar? Since protocol checks cannot be skipped on 4.3.0, they fail on 4.3.0, and versions before 4.3.0 are not compatible with Python 3.12.4 and beyond, my CI is currently completely blocked on Python 3.12.

bersbersbers commented 5 months ago

Here's another related failure:

from typing import Protocol

from typeguard import typechecked, typeguard_ignore

@typeguard_ignore
class MyCallable(Protocol):
    def __call__(self) -> None: ...

def make_callable() -> MyCallable:
    def my_callable() -> None:
        print("Works!")

    return my_callable

@typechecked
def fun(my_callable: MyCallable) -> None:
    my_callable()

fun(make_callable())

Output:

typeguard.TypeCheckError: argument "my_callable" (function) is not compatible with the MyCallable protocol because it has no attribute named '__no_type_check__'
agronholm commented 5 months ago

That's not expected to work to begin with, as @typeguard_ignore is only supposed to be used on functions where you wish to skip type checking. Latching it to things you don't want to check against has never worked.

bersbersbers commented 5 months ago

Alright, thanks! This is my workaround for CI right now:

__init__.py

import typeguard._checkers as checkers  # noqa:PLC2701
from funcy import constantly

checkers.check_protocol = constantly(None)  # agronholm/typeguard#465
antonagestam commented 2 months ago

It looks like this also broke phantom-types. Minimal reproducible example, that works on 4.2.1 but breaks on 4.3.0.

import typeguard
from typing import runtime_checkable, Sized, Iterable, Protocol, TypeVar

T = TypeVar("T", bound=object, covariant=True)

@runtime_checkable
class SizedIterable(Sized, Iterable[T], Protocol[T]): ...

typeguard.check_type(
    value=(),
    expected_type=SizedIterable,
    typecheck_fail_callback=None,
    forward_ref_policy=typeguard.ForwardRefPolicy.ERROR,
    collection_check_strategy=typeguard.CollectionCheckStrategy.ALL_ITEMS,
)

Breaks with:

Traceback (most recent call last):
  File "/Users/annevoab/projects/phantom-types/tc-repro.py", line 17, in <module>
    typeguard.check_type(
  File "/Users/annevoab/.pyenv/versions/phantom-types/lib/python3.12/site-packages/typeguard/_functions.py", line 106, in check_type
    check_type_internal(value, expected_type, memo)
  File "/Users/annevoab/.pyenv/versions/phantom-types/lib/python3.12/site-packages/typeguard/_checkers.py", line 861, in check_type_internal
    checker(value, origin_type, args, memo)
  File "/Users/annevoab/.pyenv/versions/phantom-types/lib/python3.12/site-packages/typeguard/_checkers.py", line 738, in check_protocol
    raise TypeCheckError(
typeguard.TypeCheckError: tuple is not compatible with the SizedIterable protocol because it has no attribute named '__orig_bases__'

I think it was noteworthy that the "decomposed" protocols are passing:

typeguard.check_type(
    value=(),
    expected_type=Sized,
    typecheck_fail_callback=None,
    forward_ref_policy=typeguard.ForwardRefPolicy.ERROR,
    collection_check_strategy=typeguard.CollectionCheckStrategy.ALL_ITEMS,
)
typeguard.check_type(
    value=(),
    expected_type=Iterable[T],
    typecheck_fail_callback=None,
    forward_ref_policy=typeguard.ForwardRefPolicy.ERROR,
    collection_check_strategy=typeguard.CollectionCheckStrategy.ALL_ITEMS,
)

So the practical effect is I've lost the ability to check "intersection" protocols.

agronholm commented 1 month ago

I have a PR with some sweeping protocol check changes. Would you mind verifying it? I tested against the script in the OP and it passes.

antonagestam commented 1 month ago

@agronholm Thanks for the continued work on improving this library 💪

agronholm commented 1 month ago

Appreciated! My attention is so thinly spread between all my projects, the list of which is ever growing, that I barely have time to make bug fixes, let alone new features.