For class variables, lookup type in base classes (#1338, #2022, #2211)

[TrueBrain]

With this patch, class variables are validated for their type if only the base class defines the type.

Typeshed shows 1 issue in the pyi files. doc is set by MyPy to be 'str'. In stdlib/2/types.pyi, in FunctionType, it is set to func_doc, which is 'Optional[str]'. Because with this patch assignments are checked against their definition, assigning 'Optional[str]' to 'str' is incompatible.

The solution appears simple, by doing types.pyi similar as stdlib/3: not using a level of indirection when assigning these types.

So instead of:

func_doc = ...  # type: Optional[str]
 __doc__ = func_doc

Doing:

__doc__ = ... # type: Optional[str]

If you like, I can make a PR for that change too; but I was first more curious if this PR is correct.

[TrueBrain]

I am having a hard time figuring out how TypeVars could be resolved. Any pointers (given the rest of the patch is a good idea) would be very welcome. For now I simply disabled inheriting TypeVars from the base class, and possibly it is even something for another PR.

[gvanrossum]

I'm excited that you're working on this! I'm not sure I can answer your questions (maybe @JukkaL can help). Personally I'm not sure whether type variables declared in a class should ever be overridden. However I don't see any tests for generic classes -- surely there are all sorts of interesting cases when the base class is generic over T and the class variable's type involves T, and then the subclass either substitutes a specific type for T, or a different type variable, etc...

regarding __doc__, I think that should just always be considered Optional[str].

[TrueBrain]

Found a way to lookup TypeVars. Added some test cases for it, let me know if you can think of more juicy ones.

Regarding __doc__, it is hard coded in the MyPy codebase as being '__builtins__.str' (mypy/nodes.py). I cannot figure out how to change that to Optional. Hopefully someone can point me in the right direction to fix that issue. Nevermind; turns out __builtin__.pyi also set __doc__ to str again. Will prepare a PR for Typeshed to change all __doc__ to Optional[str]. There are quiet a few str atm.

[JukkaL]

You can probably use map_type_from_supertype to translate generic types from a base class to a derived class.

Example where this would be useful:

T = TypeVar('T')

class A(Generic[T]):
    x = None  # type: List[T]
    y = None  # type: List[T]

class B(A[int])
    x = [1]   # ok
    y = ['']  # report error

If that doesn't work for some reason, it's also okay to reject assignments to class variables with type variables in them and file an issue about supporting them.

[gvanrossum]

I'm not sure it's a good idea to make those errors. I expect those to be common, as stand-in for instance variables. (Also encouraged by PEP 526.)

…

--Guido (mobile)

[TrueBrain]

I indeed found out that map_type_from_supertype does the trick. While adding your example to the test-suite, I noticed I overlooked something (type T worked, but List[T] didn't). Also fixed now with latest commit.

This should implement all cases I could find (including TypeVars). Let me know if you can think up any more test cases to include!

[gvanrossum]

This is a pretty neat small change! I have a few questions.

[gvanrossum]

Hm, this would become invalid with strict none checking. Maybe just use another int value?

[gvanrossum]

In a large class hierarchy this error could be pretty mysterious. Can you figure out a way to at least link to the superclass where the variable is declared first? (Even better the exact line, but that may be tricky.)

[TrueBrain]

I wonder if this is an issue for only class variables; in general it would be nice if we can tell where the original declaration came from. As the same issue is already true for when using "self.a = 1" for example. I will fiddle with this a bit, see what I can come up with.

[gvanrossum]

Hm, are you sure that this (without caring about C even) should be allowed? I think it's just as much an error as when no type is given. (However, the declared type here should be allowed to be a subclass of the type declared earlier.)

[JukkaL]

Class variables are invariant, so the type should be equivalent (is_equivalent) to the base class type. Example:

class A:
    a = None  # type: object
    def f(self) -> None:
        self.a = 'x'

class B(A):
    a = None  # type: int
    def g(self) -> None:
        print(self.a + 1)  # ouch, could be str

[gvanrossum]

Another idea for more tests: the superclass could have a method; a subclass should not be able to override this with a variable. And vice versa.

[TrueBrain]

I always love how adding a test case can show you other issues. Fixed all issues I am aware off. I also added that if a superclass defines a @property it can be redefined as variable in subclasses. This mostly because io.pyi uses this. Still investigating if we can show where a type was defined.

[TrueBrain]

Splitted off the "find where a type was first declared" into a bug ticket and a PR in my fork. Hope you don't mind, but I would like to resolve it a bit more generic :)

[TrueBrain]

Found one additional issue.

https://github.com/python/typeshed/blob/5a2a46d3bd43f5fffb9a6216fe6dedf59ce2f7a1/stdlib/2/ConfigParser.pyi

Line 35 defines args as Tuple[str,str,str]. Line 40 as Tuple[str,str,str,str]. This is of course incompatible, and is now an error. This also happens on line 51 and 58.

I have no idea how to resolve this cleanly, as I cannot figure out what 'args' refers to. Mainly because the Python source does not reference it. Any suggestions would be appreciated.

[gvanrossum]

Ah, I see what this is. args is a predefined attribute on BaseException. Its value is always a tuple and its initial value is always the tuple of positional arguments passed to the BaseException constructor. (It can be assigned to as well, but it must always be an iterable, and it is always converted to a tuple.)

I don't know how this ended up in the stubs but the solution is to just remove these from the ConfigParser stub. (For bonus points, change it to Tuple[Any, ...] in BaseException for Python 2 and 3.)

[gvanrossum]

Um, you synced typeshed one commit before the fix you applied. You should probably do something like this:

cd typeshed
git co master
git pull
cd ..
git add typeshed
git ci -m 'Sync typeshed'
git push

[TrueBrain]

Oops, indeed, I forgot to rebase my master branch against upstream. Silly mistake :D Tnx!

[JukkaL]

@TrueBrain This looks good to me. If you fix the merge conflict (should be easy) this can be merged. Optionally, implement/test redefining an attribute with an Any type:

class A:
    a = 1
class B(A):
    a = 'x'  # type: Any  # Should probably be okay

[JukkaL]

Added #2500 as a potential follow-up task based on above discussion about showing the class that originally defined the attribute.