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 properties that are other classes are not treated properly #435

Closed
Dr-Irv opened this issue Sep 30, 2020 · 12 comments
Closed

class properties that are other classes are not treated properly #435

Dr-Irv opened this issue Sep 30, 2020 · 12 comments
Assignees
Labels
enhancement New feature or request needs decision Do we want this enhancement? type checking

Comments

@Dr-Irv
Copy link

Dr-Irv commented Sep 30, 2020

Environment data

  • Language Server version: 2020.9.6
  • OS and version: Windows 10
  • Python version (& distribution if applicable, e.g. Anaconda): Anaconda Python 3.7.5

Expected behaviour

No error

Actual behaviour

from abc import ABC, abstractmethod
from typing import Type


class SolClass(ABC):
    def __init__(self, x: int):
        self._x = x

    @abstractmethod
    def getx(self) -> int:
        pass


class Container(ABC):
    @property
    @classmethod
    @abstractmethod
    def SolutionClass(cls) -> Type[SolClass]:
        pass

    def __init__(self, x: int):
        self.solution = self.SolutionClass(x)

    def getSolution(self) -> SolClass:
        return self.solution


class MySolClass(SolClass):
    def getx(self) -> int:
        return 2 * self._x


class UseContainer(Container):
    SolutionClass = MySolClass

    def __init__(self, x: int):
        super().__init__(x)


uc = UseContainer(120)
print(uc.getSolution().getx())

The above is valid python, and mypy reports no issues. But pylance reports

"self.SolutionClass" has type "property" and is not callable
  Type "property" is not callable

which corresponds to the line:

        self.solution = self.SolutionClass(x)

The property in this case is callable, as it is a class.

@erictraut
Copy link
Contributor

Pylance is doing the right thing here. It is not legal to define a property with a classmethod.

To demonstrate this, try this simple program:

class MyClass:
    @property
    @classmethod
    def my_property(cls) -> int:
        return 3


a = MyClass()
print(a.my_property)

This will generate an exception TypeError: 'classmethod' object is not callable.

If you really want to create a property that works with a class (as opposed to a class instance), you need to define it on the metaclass, but I don't think that's a solution to the problem you're trying to solve here.

You are also doing something that's not really correct from a type standpoint. You are declaring a property in an abstract base class and then overriding it with a variable in a subclass. A property and a variable have different semantics (e.g. a property is not writable or deletable), so pylance considers it an error if you try to override one with the other.

If you replace the property declaration in your code with the following, it will work as you intend:

    SolutionClass: Type[SolClass]

The technique you're using here is an anti-pattern because you're expecting subclasses to override a class variable. That's probably why you're finding it difficult to properly annotate the types. A more typical (and preferable) solution would look like this:

from abc import ABC, abstractmethod
from typing import Type

class SolClass(ABC):
    def __init__(self, x: int):
        self._x = x

    @abstractmethod
    def getx(self) -> int:
        pass

class Container(ABC):
    def __init__(self, sol_class: Type[SolClass], x: int):
        self.solution = sol_class(x)

    def getSolution(self) -> SolClass:
        return self.solution

class MySolClass(SolClass):
    def getx(self) -> int:
        return 2 * self._x

class UseContainer(Container):
    def __init__(self, x: int):
        super().__init__(MySolClass, x)

@jakebailey jakebailey added the waiting for user response Requires more information from user label Sep 30, 2020
@github-actions github-actions bot removed the triage label Sep 30, 2020
@Dr-Irv
Copy link
Author

Dr-Irv commented Sep 30, 2020

@erictraut Thanks for your answer. The construct above was something I copied from StackOverflow here:
https://stackoverflow.com/a/53417582/1970354 . The nice thing about that construct is that mypy and pylance both report an issue if the line SolutionClass = MySolClass is missing in the definition of UseClass

Your solution of just declaring the variable SolutionClass at the meta class level will work, but if you forget to define that variable in the subclass, it will only get caught at runtime.

I'm also not sure I agree with your statement " It is not legal to define a property with a classmethod." Do you have a reference for that?

@erictraut
Copy link
Contributor

Try running the code in my post above where I combine property with classmethod. It generates a runtime exception. This makes sense if you think about how a property object is implemented.

@Dr-Irv
Copy link
Author

Dr-Irv commented Sep 30, 2020

Yes, I get that, but in my case, I made it an abstract class property. And that code runs fine.

@erictraut
Copy link
Contributor

That's because you have dynamically overridden the illegal code with something else. That doesn't make it legal. It means that you've managed to mask the problem at runtime.

Is there any reason you can't switch to the pattern I suggested? I think that's a much cleaner solution all around.

@savannahostrowski
Copy link
Contributor

This issue has been waiting for information for 30 days. Because we haven't heard back, we'll be closing this ticket. Please reopen if this issue persists!

@Dr-Irv
Copy link
Author

Dr-Irv commented Oct 30, 2020

@savannahostrowski How can I reopen?

@Dr-Irv
Copy link
Author

Dr-Irv commented Nov 2, 2020

That's because you have dynamically overridden the illegal code with something else. That doesn't make it legal. It means that you've managed to mask the problem at runtime.

Is there any reason you can't switch to the pattern I suggested? I think that's a much cleaner solution all around.

@erictraut

The reason is because we have multiple subclasses of type Container that we want to call dynamically. So let's say we had UseContainer, UseContainer2 and UseContainer3 all as subclasses of Container, and we map the values 1,2,3 to those 3 classes respectively:

So we have code like this (in addition to what I initially provided):

class MySolClass2(SolClass):
    def getx(self) -> int:
        return 3 * self._x


class MySolClass3(SolClass):
    def getx(self) -> int:
        return 4 * self._x


class UseContainer2(Container):
    SolutionClass = MySolClass2
    def __init__(self, x: int):
        super().__init__(x)


class UseContainer3(Container):
    SolutionClass = MySolClass3
    def __init__(self, x: int):
        super().__init__(x)


d: Dict[int, Type[Container]] = {1: UseContainer, 2: UseContainer2, 3: UseContainer3}
for i in [1, 2, 3]:
    uc: Container = d[i](120)
    print(uc.getSolution().getx())

Your solution doesn't allow the above to work, because the line that defines uc in the loop has to know which SolutionClass was used in order to call the constructor.

@erictraut
Copy link
Contributor

I see, thanks for the additional context.

My solution, with a minor tweak, would still work.

from abc import ABC, abstractmethod
from typing import Dict, Type

class SolClass(ABC):
    def __init__(self, x: int):
        self._x = x

    @abstractmethod
    def getx(self) -> int:
        pass

class Container(ABC):
    def __init__(self, x: int, sol_class: Type[SolClass] = SolClass):
        self.solution = sol_class(x)

    def getSolution(self) -> SolClass:
        return self.solution

class MySolClass(SolClass):
    def getx(self) -> int:
        return 2 * self._x

class UseContainer(Container):
    def __init__(self, x: int):
        super().__init__(x, MySolClass)

class MySolClass2(SolClass):
    def getx(self) -> int:
        return 3 * self._x

class MySolClass3(SolClass):
    def getx(self) -> int:
        return 4 * self._x

class UseContainer2(Container):
    def __init__(self, x: int):
        super().__init__(x, MySolClass2)

class UseContainer3(Container):
    def __init__(self, x: int):
        super().__init__(x, MySolClass3)

d: Dict[int, Type[Container]] = {1: UseContainer, 2: UseContainer2, 3: UseContainer3}
for i in [1, 2, 3]:
    uc = d[i](120)
    print(uc.getSolution().getx())

@Dr-Irv
Copy link
Author

Dr-Irv commented Nov 2, 2020

Still have an issue, because I would like the codes 1,2,3 to be class properties. So here is the code that passes mypy but not pylance

from abc import ABC, abstractmethod
from typing import Type, Dict, List, cast


class SolClass(ABC):
    def __init__(self, x: int):
        self._x = x

    @abstractmethod
    def getx(self) -> int:
        pass


class Container(ABC):
    @property
    @classmethod
    @abstractmethod
    def SolutionClass(cls) -> Type[SolClass]:
        pass

    @property
    @classmethod
    @abstractmethod
    def container_code(cls) -> int:
        pass

    def __init__(self, x: int):
        self.solution = self.SolutionClass(x)

    def getSolution(self) -> SolClass:
        return self.solution


class MySolClass(SolClass):
    def getx(self) -> int:
        return 2 * self._x


class UseContainer(Container):
    SolutionClass = MySolClass
    container_code = 1


    def __init__(self, x: int):
        super().__init__(x)


class MySolClass2(SolClass):
    def getx(self) -> int:
        return 3 * self._x


class MySolClass3(SolClass):
    def getx(self) -> int:
        return 4 * self._x


class UseContainer2(Container):
    SolutionClass = MySolClass2
    container_code = 2


    def __init__(self, x: int):
        super().__init__(x)


class UseContainer3(Container):
    SolutionClass = MySolClass3
    container_code = 3

    def __init__(self, x: int):
        super().__init__(x)


container_list: List[Type[Container]] = [UseContainer, UseContainer2, UseContainer3]

d: Dict[int, Type[Container]] = {cast(int, c.container_code): c for c in container_list}
for i in d.keys():
    uc: Container = d[i](120)
    print(uc.getSolution().getx())

@jakebailey jakebailey added needs investigation Could be an issue - needs investigation and removed waiting for user response Requires more information from user labels Dec 15, 2020
@heejaechang heejaechang self-assigned this Apr 12, 2022
@heejaechang heejaechang added needs decision Do we want this enhancement? enhancement New feature or request and removed needs investigation Could be an issue - needs investigation labels Apr 13, 2022
@heejaechang
Copy link
Contributor

we are not going to support this at this time.

@OGoodness
Copy link

+1 Would like this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request needs decision Do we want this enhancement? type checking
Projects
None yet
Development

No branches or pull requests

6 participants