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

[TrueBrain]

Continuation of #2380, with fixing issues indicated in #2503.

[gvanrossum]

Hm... There are still a few issues around assignment to next and __iter__. I will try to look into those tomorrow.

[gvanrossum]

[UPDATE: converted the example to Python 3.]
So I have this example that gives an error (reduced from some real code):

from typing import Iterator

class E: pass

class A(Iterator[E]):
    def n(self) -> E: pass
    __next__ = n

class C(A):
    def n(self) -> E: pass
    __next__ = n  # <-- error

which gives this error on the very last line:

__tmp__.py: note: In class "C":
__tmp__.py:11: error: Incompatible types in assignment (expression has type Callable[[C], E], variable has type Callable[[A], E])

But I need to think more about whether that's a bug in your PR or not. The problem seems to be that the variable (being a callable) wants the arguments to be contravariant, but the expression needs it to be covariant.

[gvanrossum]

So here's a more systematic example exploring this:

class I:
    def foo(self) -> None: pass

class A(I):
    def foo(self) -> None: pass

class B(A):
    def bar(self) -> None: pass
    foo = bar

class C(B):
    def bar(self) -> None: pass
    foo = bar

We get an error on foo = bar in C, but not on the same line in B.

__tmp__.py: note: In class "C":
__tmp__.py:13: error: Incompatible types in assignment (expression has type Callable[[C], None], variable has type Callable[[B], None])

The reason appears to be that once foo is used as a variable its type is fixed -- while a method that's overridden is still allowed to be covariant in self. Perhaps the solution is not to see foo as a variable definition in B or C but as assignment to a "variable" whose (rather special) type is inherited from the base class?

[gvanrossum]

Marking this as requiring changes, see main comment stream.

[gvanrossum]

I ran this over our internal codebases and it found a bunch of missing/incorrect type annotations and a few bits of questionable code. I will try to review one more time and then hopefully merge. Thanks a bundle for this significant piece of work!

[gvanrossum]

Gotta run but here's one review item.

[gvanrossum]

I think there's something fishy here. I have this test code:

class A:
  x = 0
class B(A):
  x = ''  # type: ignore
  x = 0  # <-- line 5

This gives TWO errors, both on the last line:

__tmp__.py:5: error: Incompatible types in assignment (expression has type "str", variable has type "int")
__tmp__.py:5: error: Incompatible types in assignment (expression has type "int", variable has type "str")

The first of these is repeated for line 4 if I remove the # type: ignore.

I'm not sure what's going on but I suspect that its somehow taking the inferred type of x from line 4 (i.e. str) and checking it against the base class (i.e. int), resulting in the first error above; then it is taking the type of the expression on line 5 (i.e. int) and checking it against the type of x from the previous line, resulting in the second error.

[gvanrossum]

FWIW it's perhaps easier to understand if you use a different type on the last line. E.g. if you use 0.0 there the errors changes to

__tmp__.py:5: error: Incompatible types in assignment (expression has type "str", variable has type "int")
__tmp__.py:5: error: Incompatible types in assignment (expression has type "float", variable has type "str")

This also reminds me of a general issue I have with this PR: it would be nice to show the file+line of the conflicting variable definition in the base class. (I vaguely recall asking for that before?)

And that then reminds me of another thing: since your loop over the MRO breaks at the first conflict, I think it would be better to go in forward direction (i.e. drop the reversed() call), so that the nearest conflict is reported. That seems important in a case like this:

class A:
    x = 0
class B(A):
    x = ''  # type: ignore
class C(B):
    x = 0.0

Your code currently reports a conflict between float and int for class C, while (given the # type: ignore in class B) the more relevant conflict is between float and str. In fact, I believe this code should not have an error at all:

class A:
    x = 0
class B(A):
    x = ''  # type: ignore
class C(B):
    x = ''  # should be okay

[TrueBrain]

Tnx for the review (and sorry for the slow reply).

Fixed the issue of double error reporting; it checked line 5 against the base class (reporting the first error), and after that line 5 against line 4 (reporting the second error). It now no longer reports the second error if there was any issue with the base class validation.

For some reason, if you set the type to ignore, the type of the variable is still set to the type of the rvalue. Take for example this snippet:

class A():
    a = 0
    a = ""  # type: ignore

    def b(self) -> None:
        self.a = "a"

This is currently (without my patch) also reporting an error. Do you agree that both cases are the same? As solving them both should be relative easy, but possibly something for another patch?

I agree that changing the order of checks is more intuitive.

You indeed asked before to show where the type is original declared. I have looked into it a few times, but I keep running into issues. I will continue to look at that, but I would prefer that in another PR, as it seems to require more than a few lines of code.

[gvanrossum]

[snippet] This is currently (without my patch) also reporting an error. Do you agree that both cases are the same? As solving them both should be relative easy, but possibly something for another patch?

No, I think the cases are subtly different. Within the same scope, we have the general rule that the first type (whether inferred or declared) wins, even if there's a # type: ignore on the second. (See also discussion at #2589 and PR #2591 -- even though that's a slightly different case since it is about conflicting explicit declarations.)

But when we're overriding a variable whose initial type comes from a base class I think the new type should win (if it comes with a # type: ignore at least), because there are some legitimate cases for this. It's similar with the check that a method override matches the base class method it is overriding -- if you have a mis-matching override, you can put a # type: ignore on it and the subclass will use the type it has declared.

So:

def f() -> None:
    a = 0
    a = ''  # type: ignore
    reveal_type(a)  # it's still int

but

class B:
    a = 0
class C(B):
    a = ""  # type: ignore
    reveal_type(a)  # it's a str

You indeed asked before to show where the type is original declared. I have looked into it a few times, but I keep running into issues. I will continue to look at that, but I would prefer that in another PR, as it seems to require more than a few lines of code.

If you really can't show the previous line, can you at least make the error message different, e.g. mention that the type doesn't match the type from a base class? Otherwise I worry that we'll just have a lot of confused users.

[gvanrossum]

Also, can you please add tests for the last two changes you made? For each change there should be at least one test that fails without it.

[TrueBrain]

Thank you for the detailed explanation! So far the only way I found to reliable implement this, is to also allow the following:

class A:
    a = None  # type: int
class B(A):
    a = None  # type: str  # will show an error
class C(B):
    a = "a"  # will not show an error

Initially this would throw two errors (on line 4 and 6); with the latest version only one (on line 4). This is mainly because type: ignore just adds the line to an ignore list, but doesn't change how MyPy does the validation from there on. Now the above example is already in the test-suite, and it was said it should throw two errors. But with the request for type: ignore to be allowed, do you agree that the above snippet should only show a single error, and that a from there on is considered str?

I also changed the error message to include the base class. That hopefully indeed avoids confusion where the error comes from.

Thank you for all the comments and remarks; much appreciated!

[JukkaL]

It seems to me that only showing a single error for the example is okay and perhaps sometimes even desirable.

[gvanrossum]

Indeed, I want it to only show the error in B and not in C. These kinds of things are subtle (and not spelled out in detail by PEP 484) but having a subclass override the type of some attribute is occasionally a feature, even though it's not type-safe, and that's a decent use case for # type: ignore (the other use case is to work around outright mypy bugs -- but this is not a bug). So what you coded up here should be correct (I'll review it ASAP).

[gvanrossum]

Thanks! This LGTM. I am going to wait merging until we've sorted out some other issues though. Should take at most a few days. This has been a tremendous piece of work and it's much appreciated how you've stuck to the project.

[gvanrossum]

Whee! This now passes with our internal codebases.

[JukkaL]

Yay!

[TrueBrain]

Whoho! 50 commits, and it was worth the effort :D Thank you both!