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

type checking for attributes #20

Open
glyph opened this issue Jul 6, 2020 · 7 comments
Open

type checking for attributes #20

glyph opened this issue Jul 6, 2020 · 7 comments

Comments

@glyph
Copy link

glyph commented Jul 6, 2020

This example doesn't complain:

from zope.interface import Interface, Attribute, implementer

class IWhatever(Interface):
    stuff: str = Attribute("name")


@implementer(IWhatever)
class WhateverBroken(object):
    stuff: int

even though the type of stuff doesn't match.

This closed PR, from 2017, suggests that this syntax was intended to work one day:

zopefoundation/zope.interface#98

I'm guessing this is what the README is already talking about, when it says "Interface compatibility checker will not type-check non-method attributes"?

@kedder
Copy link
Member

kedder commented Jul 6, 2020

@glyph, yes, that README sentence refers to this issue. This feature was supported in the earlier version, but since interface checking was reimplemented, I had to drop it. The reason is documentde in _report_implementation_problems:

# We could check the field type for compatibility with the code
# below, however this yields too many false-positives in
# real-life projects. The problem is that we can only check
# types defined on a class, instead of instance types, so all
# "descriptor" properties will be falsly reported as type
# mismatches. Instead we opt-out from property type checking
# until we figure out the workaround.

@kedder
Copy link
Member

kedder commented Jul 6, 2020

If I remember correctly, the issue is boils down to the type difference between class and instance attributes, e.g.

class Descriptor:
    __get__(self, obj) -> int:
        return 0

class IWhatever(Interface):
    stuff: int = Attribute("name")

@implementer(IWhatever)
class Whatever:
    stuff = Descriptor()

The type of Whatever.stuff will be Descriptor, but type of Whatever().stuff will be int. IWhatever.stuff is int, but Whatever.stuff is Descriptor, which would be reported as mismatch.

@glyph
Copy link
Author

glyph commented Jul 6, 2020

@kedder Thanks for the explanation. I went looking for how this was handled by Protocol to see how it could work, but it looks like Protocols have this same issue! I added __set__ and __delete__ methods to make sure that it wasn't an issue with the property declared as mutable being incomplete, but this still results in an error:

from typing import Protocol

class Descriptor:
    def __get__(self, obj: object) -> int:
        return 0

    def __set__(self, obj: object, value: int) -> None:
        return None

    def __delete__(self, obj: object) -> None:
        return None

class Whatever:
    stuff = Descriptor()


class WhateverProto(Protocol):
    stuff: int


whatever: WhateverProto = Whatever()

@glyph
Copy link
Author

glyph commented Jul 6, 2020

Looks like it's a known issue: python/mypy#5481

@glyph
Copy link
Author

glyph commented Jul 6, 2020

Would it be possible to make this a configuration option? There are some hacks which would make this possible (like what attrs's stubs do with Factory)

@kedder
Copy link
Member

kedder commented Jul 6, 2020

We can definiely make it an option.

@glyph, can you try uncommenting these lines and check if that actually works for your project?

# api.check_subtype(
# impl_mtype,
# iface_mtype,
# context=ctx,
# subtype_label=f'"{impl_name}" has type',
# supertype_label=f'interface "{iface_name}" defines "{member}" as',
# )

@glyph
Copy link
Author

glyph commented Jul 6, 2020

Gotcha. There may be … significant latency on this experiment, since we're burning down LOTS of unrelated mypy errors on Twisted right now, so seeing how this works in practice might take a while. But @adiroiban @rodrigc might be interested to know that this is possible.

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