Specify type when type variables are remained in inherited classmethod.

[kitsuyui]

Fix: #3645

- Fix: 1337, 3645

[ilevkivskyi]

Thanks for the PR!

This may be not the best first issue to fix, but if you are ambitious, then here are few more comments about what to do:

1. Add tests for all patterns mentioned in the issues you want to close
2. Add more sophisticated tests that test your solution in edge cases (e.g. multiple inheritance bases, multiple inheritance levels, overriding, binding one type variable to another, etc.)
3. In @classmethod of Generic class #1337 there is also a question what to do with class attributes (in addition to class methods), you should address this. Namely,

class C(Generic[T]):
    x: T
class D(C[int]): ...
D.x  # should not error and result in 'builtins.int'

4. I think a "proper" fix for these issues should also add some more changes. For example, this currently correctly generates an error about ambiguity:

class C(Generic[T]):
    x: T
reveal_type(C.x)

but the revealed type should be Any, type variables should not "leak out" even in case of errors to avoid more spurious errors.

[kitsuyui]

@ilevkivskyi
Thank you for reviewing my codes.
I will try that you pointed!

[kitsuyui]

@ilevkivskyi

In #1337 there is also a question what to do with class attributes (in addition to class methods), you should address this. Namely,

class C(Generic[T]):
    x: T
class D(C[int]): ...
D.x  # should not error and result in 'builtins.int'

I think this sample should be like this

class C(Generic[T]):
    x: List[T] = []
class D(C[int]): pass
D.x # should not error and result in 'builtins.int' 

Because x: T become type hint for instance attribute.
So I will test x: List[T] = [] as class attribute.

[kitsuyui]

Sorry, I've tried and I understood it will not be solved for class attributes.

Class attributes are shared with super-class. So subclasses cannot be restricted to narrow type.

from typing import Generic, TypeVar, List

T = TypeVar('T')
class A(Generic[T]):
    x: List[T] = []
class B(A[int]): pass
class C(A[str]): pass
B.x.append(1)
C.x.append('1')
print(B.x)  # => [1, '1']
print(C.x)  # => [1, '1']

[ilevkivskyi]

Sorry for a delay. I will try to review this PR next week. In the meantime could you please fix the merge conflict?

[kitsuyui]

@ilevkivskyi I've updated!

[ilevkivskyi]

Hi @kitsuyui! Sorry for a delay with review, are you still interested in working on this?

[kitsuyui]

@ilevkivskyi
Yeah, I'd love to.
I will challenge updating that it works even in case of #5664 if needed.

[ilevkivskyi]

OK, let us give this one more chance. The general idea is good, but the implementation requires polishing. I left some comments below.

[ilevkivskyi]

This algorithm currently looks wrong, and this is why #5664 and similar more complex type variable bindings don't work. You need to map type variables correctly, look for example in maptype.py

[ilevkivskyi]

The algorithm is still wrong. For example it will not work for something like this:

class B(Generic[T]):
    @classmethod
    def f(cls) -> T: ...
class C(B[Tuple[T, T]]):
    ...
class D(C[str]):
    ...
reveal_type(D.f())  # Revealed type is 'str', but should be 'Tuple[str, str]'

This is not just a small bug there, it is "conceptually" wrong. As I already said twice, you should use something similar to map_instance_to_supertype from maptype.py. If this is too complex, then I can overtake this PR, it already takes too long.

[oraluben]

I've tried the latest code and found something that may be added to the test cases, here's the code.

from typing import Generic, Callable, TypeVar, Iterator, Type

T = TypeVar('T')
BoxT = TypeVar('BoxT', bound='Box', covariant=True)


class Box(Generic[T]):
    @classmethod
    def wrap(cls, generator: Callable[[], T]) -> 'Box[T]':
        pass


class IteratorBox(Box[Iterator[T]],
                  Generic[T]):
    @classmethod
    def wrap_iter(cls: Type[BoxT],
                  typ: Type[Box[T]]) -> Callable[[Callable[[], Iterator[T]]], BoxT]:
        def inner(generator: Callable[[], Iterator[T]]) -> BoxT:
            pass

        return inner


class Str(Box[str]):
    pass


class StrI(IteratorBox[str]):
    pass


@StrI.wrap_iter(Str)
def g() -> Iterator[str]:
    pass

I believe there's no type error in the signature, but the output was

test.py:32: error: Argument 1 has incompatible type "Callable[[], Iterator[str]]"; expected "Callable[[], Iterator[Iterator[str]]]"

test.py:32: error: Argument 1 to "wrap_iter" of "IteratorBox" has incompatible type "Type[Str]"; expected "Type[Box[Iterator[str]]]

Hope this will help!

[kitsuyui]

@oraluben

Your code works on my local mypy! But I still have not pushed commits yet.
The biggest uncompleting task is about handling error and replacing unbound variable to Any with consistency.

[kitsuyui]

I have pushed commits that rebased from latest master.

But this is still insufficient yet.

I have been still in the way of this part: #5420 (comment)
So #5664 is not solved too.

It might be little more time to solve.

[ilevkivskyi]

@kitsuyui what is exactly the problem you have? I might be able to give some hints.

[kitsuyui]

@ilevkivskyi

I don't know what is the true behavior on following case;

from typing import Generic, TypeVar
T = TypeVar('T')
class A(Generic[T]):
    @classmethod
    def f(cls, value: T) -> T: pass
reveal_type(A.f(1))  # : Revealed type is 'builtins.int*'

T is solved by arguments. But T is not clear on class.
And #5664 seems to happen because of this behavior.

function and type unbounded classmethod's should be checked by different algorithm.
I think it is needed to be fix checkexpr.py.

But this fix may be too large. So I'm wondering "Does it exist Plan B?"

[kitsuyui]

I've finally found the answer myself.
I added error message of when accessing classmethod of generic class contains type variables.

from typing import Generic, TypeVar
T = TypeVar('T')
S = TypeVar('S')
class A(Generic[T]):
    @classmethod
    def f(cls) -> T: pass
class B(A[S]): pass
A.f  # E: Access to classmethod of generic type remains type variables T is ambiguous.
B.f  # E: Access to classmethod of generic type remains type variables S is ambiguous.

@ilevkivskyi

Would you please review this once more?

[ilevkivskyi]

Sorry for another delay. I will try to review this in the next few days.

[kitsuyui]

@ilevkivskyi

Thank you.
And I've just found myself that my branch has false negative like following case;

from typing import Generic, TypeVar


T = TypeVar('T')


class X(Generic[T]):
    @classmethod
    def a(cls) -> T:
        ...

    @classmethod
    def b(cls) -> T:
        return cls.a()

This should be treated as valid but error: Access to classmethod of generic type remains type variables T is ambiguous. will be happen.

So I'm planning to add more refactoring and more test cases. Please review after this.

[kitsuyui]

@ilevkivskyi
I did it. Please review this.

[ilevkivskyi]

Here is another pass of review. Unfortunately, this is still not ready.

[ilevkivskyi]

The algorithm is still wrong. For example it will not work for something like this:

class B(Generic[T]):
    @classmethod
    def f(cls) -> T: ...
class C(B[Tuple[T, T]]):
    ...
class D(C[str]):
    ...
reveal_type(D.f())  # Revealed type is 'str', but should be 'Tuple[str, str]'

This is not just a small bug there, it is "conceptually" wrong. As I already said twice, you should use something similar to map_instance_to_supertype from maptype.py. If this is too complex, then I can overtake this PR, it already takes too long.

[kitsuyui]

I'm going to fix them in a few days.

[kitsuyui]

@ilevkivskyi

I did it. Please review this again?

• Added test-case that class method return type depends on type variables from two different bases.
• Fixed test-cases and error messages calling directly generic class method such like A[str].f().
• Added newlines between logical blocks for readability.

[ilevkivskyi]

I did it. Please review this again?

You didn't fix the main problem (although you marked it as resolved, why?), as I mentioned in two previous reviews, you algorithm is "conceptually" wrong.

It looks like it doesn't make sense to push this forward any more.

[kitsuyui]

@ilevkivskyi
I had closed it by misunderstanding twice.
It is better that you solve this than I do. Please over take this if you can.
I'm so sorry.

[kitsuyui]

@ilevkivskyi Sorry for taking your time again.

I rewrote the code to use map_instance_to_supertype.
I read the code that using map_instance_to_supertype for ordinary method.

The code for analyze ordinary method uses TypeInfo.get_method(name). This method doesn't suit for classmethod.
So I add get_classmethod to TypeInfo.

And I added a test for #5420 (comment)

How does this?

[ilevkivskyi]

@kitsuyui Thank you for your work! It is a useful starting point for me to try making a better fix for this. Don't worry that this PR didn't make it. You just took a bit too hard issue as a first time contribution.

[kitsuyui]

Thank you @ilevkivskyi !
I'm very thankful for mypy. I will take more easier issue next.