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

Unsoundness from unbound type variables in attributes #6520

Open
msullivan opened this issue Mar 6, 2019 · 5 comments
Open

Unsoundness from unbound type variables in attributes #6520

msullivan opened this issue Mar 6, 2019 · 5 comments

Comments

@msullivan
Copy link
Collaborator

msullivan commented Mar 6, 2019

Minimized from some real code at dropbox exhibiting this failure in a nasty way.

The following code typechecks:

from typing import TypeVar, Generic

T = TypeVar('T')
class A:
    def __init__(self, x: T) -> None:
        self.x = x
    def get(self) -> T:
        return self.x

x: int = A('lol').get()

Here, T is not bound in the class but ends up in the attribute, where it is then matched with the unbound return type variable in get.
(Interestingly, it also works if get returns a different unbound type variable)

A reveal type reports: Revealed type is 'T`-1'

@ilevkivskyi
Copy link
Member

This is a consequence of the fact that we allow methods to be generic "on their own", i.e. this is perfectly valid:

T = TypeVar('T')
class C:
    def meth(self, x: T) -> T: ...

But then there is #2885.

Note there is similar (but not directly related) #3172 that proposes to reject use of class type variables in method bodies of static methods. In general, we might want to perform some overhaul of type variable scopes and clearly document the rules. Currently it often works how it works.

@msullivan
Copy link
Collaborator Author

It seems more that the problem is the treatment of the type variable's interaction with the attribute?

@ilevkivskyi
Copy link
Member

Yes, probably the simplest solution would be to prohibit storing such type on self attribute, but it may be not as easy as it looks, because type variable scopes are not accessible it type checker, while the assignment is analyzed. There is a hacky way of just rejecting all type variables with negative ids (all variables bound by functions have negative ids, while those bound by class have positive ids). This however looks fragile.

In general, as I already said we might want to re-think how type variable binding works in general (maybe at least store a pointer to the binder for every TypeVarType, currently we just copy id and other things from the binder in TypeVarType constructor).

@ilevkivskyi
Copy link
Member

It turns out this is a duplicate of an older issue #1903, but I am going to close the other one, since it has less discussion.

@pirate
Copy link

pirate commented Nov 21, 2019

Apologies if this is off-topic, but is there any way to use TypeVars as class attributes that can be overwritten by subclasses, but have them resolve within the context of the subclass when checking?

How would one go about doing something like this?

TypeForAPI = TypeVar('TypeForAPI')
TypeForDB = TypeVar('TypeForDB')

class Parser:
    type_for_api: TypeForAPI = str
    type_for_db: TypeForDB = str

    @classmethod
    def dump_to_db(cls, value: TypeForAPI) -> TypeForDB:
        return cls.type_for_db(value)

    @classmethod
    def dump_to_api(cls, value: TypeForDB) -> TypeForAPI:
        return cls.type_for_api(value)


class NumberParser(Parser):
    type_for_db: TypeForDB = float
    type_for_api: TypeForAPI = Decimal

class DateParser(Parser):
    type_for_db: TypeForDB = datetime.date
    type_for_api: TypeForAPI = str

    @classmethod
    def dump_to_db(cls, value: TypeForAPI) -> TypeForDB:
        return dateutil.parse(value).date()

    @classmethod
    def dump_to_api(cls, value: TypeForDB) -> TypeForAPI:
        return value.isoformat()

Appears to be superficially related to #3625 (I'm having the same issue with typevars on metaclasses elsewhere in the same file that example was taken from ^, but it could be a red herring).

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

No branches or pull requests

3 participants