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

Apply generic class fix also to non-callable types #8030

Merged
merged 4 commits into from Nov 29, 2019

Conversation

@ilevkivskyi
Copy link
Collaborator

ilevkivskyi commented Nov 28, 2019

This is a follow up for #8021, that applies the fix also to non-callable types. Currently non-callable types are still wrong:

R = TypeVar('R')
T = TypeVar('T')
# Can be any decorator that makes type non-callable.
def classproperty(f: Callable[..., R]) -> R: ...

class C(Generic[T]):
    @classproperty
    def test(self) -> T: ...

x: C[int]
y: Type[C[int]]
reveal_type(x.test)  # Revealed type is 'int', OK
reveal_type(y.test)  # Revealed type is 'T' ???

So, #7724 strikes again. It turns out there is not only duplicated logic for attribute kinds (decorators vs normal methods), but also for callable vs non-callable types. In latter case we still need to expand the type (like in other places, e.g., analyze_var).

@ilevkivskyi ilevkivskyi requested a review from JukkaL Nov 28, 2019
Copy link
Collaborator

JukkaL left a comment

Thanks for fixing this special case! Some additional comments would be helpful here, since it wasn't obvious to me how the fix works exactly.

Maybe also update the commit message with a code example that this fixes, since it's not entirely obvious from the explanation (or create an issue and refer to it in the commit message).

def test(self) -> T: ...

class D(C[str]): ...
class G(C[List[T]]): ...

This comment has been minimized.

Copy link
@JukkaL

JukkaL Nov 28, 2019

Collaborator

What about testing if there are two levels of generic subclasses, such as class F(G[Tuple[int]]): ...?

What if the derived class adds a type variable? There may be two interesting cases -- 1) derived class adds a type variable that becomes the first type variable 2) derived class adds a type variable the becomes the second type variable.

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Nov 28, 2019

Author Collaborator

I can add few more tests, but essentially this will be testing map_instance_to_supertype() which is already quite well tested.

This comment has been minimized.

Copy link
@JukkaL

JukkaL Nov 29, 2019

Collaborator

Yes, the extra tests don't seem useful.

@@ -855,6 +855,8 @@ class B(A[str]): pass
builtin_type, original_type,
original_vars=original_vars))
for item in t.items()])
if isuper is not None:
t = cast(ProperType, expand_type_by_instance(t, isuper))

This comment has been minimized.

Copy link
@JukkaL

JukkaL Nov 28, 2019

Collaborator

It would be nice to have a comment here, or maybe add a case to the above comment where the type is not callable.

Also, I have trouble convincing myself how we can be sure that the type arguments from isuper can always be applied to t. Maybe explain this is briefly (in the above comment perhaps, since this doesn't seem specific to this case).

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Nov 28, 2019

Author Collaborator

Also, I have trouble convincing myself how we can be sure that the type arguments from isuper can always be applied to t.

I actually have trouble understanding how this can not work :-) My guess is confusion comes from missing docstring entry for isuper, which is current instance mapped to the method definition superclass. I was also thinking about an assert, but this would be probably too much.

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Nov 28, 2019

Author Collaborator

Btw, while adding docstring items I noticed there are two unused arguments in this function.

This comment has been minimized.

Copy link
@JukkaL

JukkaL Nov 29, 2019

Collaborator

Yeah, I was confused about isuper. Now it all makes sense.

@JukkaL
JukkaL approved these changes Nov 29, 2019
Copy link
Collaborator

JukkaL left a comment

Looks good now after the docstring update!

component in case if a union (this is used to bind the self-types); original_vars are type
variables of the class callable on which the method was accessed.
Args:
t: Declared type of the method (or property);

This comment has been minimized.

Copy link
@JukkaL

JukkaL Nov 29, 2019

Collaborator

Style nit: We don't use ; after each description of an argument in docstrings.

@@ -855,6 +855,8 @@ class B(A[str]): pass
builtin_type, original_type,
original_vars=original_vars))
for item in t.items()])
if isuper is not None:
t = cast(ProperType, expand_type_by_instance(t, isuper))

This comment has been minimized.

Copy link
@JukkaL

JukkaL Nov 29, 2019

Collaborator

Yeah, I was confused about isuper. Now it all makes sense.

def test(self) -> T: ...

class D(C[str]): ...
class G(C[List[T]]): ...

This comment has been minimized.

Copy link
@JukkaL

JukkaL Nov 29, 2019

Collaborator

Yes, the extra tests don't seem useful.

@ilevkivskyi ilevkivskyi merged commit b5f4df9 into python:master Nov 29, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ilevkivskyi ilevkivskyi deleted the ilevkivskyi:fix-class-property-too branch Nov 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.