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

Class is not compatible with protocol because it has no attribute named ... #499

Open
2 tasks done
bersbersbers opened this issue Nov 3, 2024 · 11 comments
Open
2 tasks done
Labels

Comments

@bersbersbers
Copy link

Things to check first

  • I have searched the existing issues and didn't find my bug already reported there

  • I have checked that my bug is still present in the latest release

Typeguard version

4.4.1

Python version

3.13.0

What happened?

I think #498 is not fully fixed yet.

How can we reproduce the bug?

bug.py

from typeguard import install_import_hook

install_import_hook()

from code import Class, function_of_instance, function_of_type

# This works:
function_of_instance(Class())

# This does not:
function_of_type(Class)

code.py

from typing import Protocol

class Proto(Protocol):
    foo: str

class Class(Proto):
    def __init__(self):
        self.foo = "bar"

def function_of_instance(instance: Proto): ...

def function_of_type(type_: type[Proto]): ...
@bersbersbers
Copy link
Author

I think that check may be hard to due at runtime because the __init__ function is never actually called. So if there isn't another way of declaring it (is there?), __init__ would have to be parsed, or maybe the attribute checks may need to be disabled for this kind of check.

@agronholm
Copy link
Owner

But is it a bug? The class does not have a value for foo.

@agronholm
Copy link
Owner

The way it works now is according to my interpretation of how protocols should work. I could of course be mistaken, but I'd like to see someone point that out in the spec.

@bersbersbers
Copy link
Author

But is it a bug? The class does not have a value for foo.

That's right. But how else would I describe an instance attribute via a Procotol?

I'd like to see someone point that out in the spec.

Does https://peps.python.org/pep-0544/#protocol-members help? This is pretty similar to my code:

class Template(Protocol):
    name: str        # This is a protocol member
    value: int = 0   # This one too (with default)

class Concrete:
    def __init__(self, name: str, value: int) -> None:
        self.name = name
        self.value = value

var: Template = Concrete('value', 42)  # OK

@bersbersbers
Copy link
Author

PEP 544 also says:

To distinguish between protocol class variables and protocol instance variables, the special ClassVar annotation should be used as specified by PEP 526.

So I'd say anything that does not have ClassVar is considered an instance variable by default.

@agronholm
Copy link
Owner

But is it a bug? The class does not have a value for foo.

That's right. But how else would I describe an instance attribute via a Procotol?

I'd like to see someone point that out in the spec.

Does https://peps.python.org/pep-0544/#protocol-members help? This is pretty similar to my code:

class Template(Protocol):
    name: str        # This is a protocol member
    value: int = 0   # This one too (with default)

class Concrete:
    def __init__(self, name: str, value: int) -> None:
        self.name = name
        self.value = value

var: Template = Concrete('value', 42)  # OK

This compares an instance against the protocol, which was not the failing case.

@bersbersbers
Copy link
Author

bersbersbers commented Nov 3, 2024

This compares an instance against the protocol, which was not the failing case.

Yes, I agree. But theoretically, if the instance type-checks against the protocol, is there any reason the instance's type should not also type-check against the protocol's type? IMHO, the latter is what static type checkers check, and mypy/pyright are okay with this code.

I am not saying this check is easy to do at runtime, compared to the instance checks. But I would consider the current behavior a false-positive.

@agronholm
Copy link
Owner

Ok, so how about this? When checking a class against a protocol class:

  • Ignore attribute annotations other than ClassVar[...] in the protocol
  • Check that values exist for all ClassVar annotations in the protocol

@bersbersbers
Copy link
Author

bersbersbers commented Nov 3, 2024

Ok, so how about this? When checking a class against a protocol class:

  • Ignore attribute annotations other than ClassVar[...] in the protocol
  • Check that values exist for all ClassVar annotations in the protocol

I don't know too much about the inner workings of typeguard, but I would be okay with that, taking into account that parsing __init__ sounds like a lot of work and knowing your time on this project is limited.

The proposed approach would go from having potential false positives (where self.foo = "bar" exists but is not detected) to having potential false negatives (where foo: str is defined but self.foo = "bar" is missing), but I would actually prefer that (not least because mypy and pyright both feature the same false negative).

@bersbersbers
Copy link
Author

I should correct my last statement. Both mypy and pyright detect, statically, that Proto.foo is not implemented here; both pass if self.foo = "bar" is commented in:

from typing import Protocol

class Proto(Protocol):
    foo: str

class Class(Proto):
    def __init__(self) -> None:
        # self.foo = "bar"
        pass

def function_of_instance(instance: Proto) -> None: ...

def function_of_type(type_: type[Proto]) -> None: ...

function_of_instance(Class())

function_of_type(Class)

@agronholm
Copy link
Owner

False negatives are way better than false positives. The run-time type checks aren't perfect, and should not be assumed to be.

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

No branches or pull requests

2 participants