Skip to content

Treat type equivalent to Type[Any] #2825

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

Merged
merged 8 commits into from
Mar 14, 2017
Merged

Conversation

ilevkivskyi
Copy link
Member

Fixes #2655

As discussed in typing docs, Type[Any] is equivalent to plain type. is_subtype was checking this only one side, here I add the opposite side.

@ddfisher
Copy link
Collaborator

ddfisher commented Feb 8, 2017

Instead of fixing this up in one particular place, I think it'd be better to actually translate all builtins.type types to Type[Any].

@elazarg
Copy link
Contributor

elazarg commented Feb 8, 2017

I would expect type to be compatible with Type[T] for any T. Is this the case in this PR?

@ilevkivskyi
Copy link
Member Author

@ddfisher I don't think it is a good idea to translate type everywhere. For example, Type is not valid as a base class (for a good reason, at runtime it is just a normal class, not a metaclass, and inheriting from Type leads to unexpected behaviour), it is not valid in runtime context, etc. In addition this could complicate #999 and #2428.

However, I agree that it makes sense to translate type everywhere in subtypes. I fixed this in a new commit and this also allowed to remove few other special cases. (Note that this means that type will be subtype of all Type[t], since Type is considered covariant and Any is subtype of everything)

@elazarg Currently all Type[t] already considered subtypes of type, I only fix here the opposite direction.

@@ -168,7 +168,7 @@ def test_var_arg_callable_subtyping_9(self) -> None:
self.fx.callable_var_arg(0, self.fx.b, self.fx.d))

def test_type_callable_subtyping(self) -> None:
self.assert_strict_subtype(
self.assert_subtype(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this change? It doesn't seem right to me.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, nevermind -- I see the preexisting comment about it in visit_type_type.

@ddfisher
Copy link
Collaborator

ddfisher commented Feb 8, 2017

That a good point -- I guess we can't translate type to Type[Any] everywhere. I'm not sure this is sufficient, though -- will type and Type interact properly with meet, join, and TypeVars?

@elazarg
Copy link
Contributor

elazarg commented Feb 8, 2017

I am not sure why we can't - we can always special case the places where Python happen "not to work the way it ought to" (we won't forget this information).

@ilevkivskyi
Copy link
Member Author

ilevkivskyi commented Feb 8, 2017

@ddfisher Surprisingly (or rather not) it is already treated correctly everywhere else where I found. For example, meet_types(Type[whatever], type) == Type[whatever], and joint_types(Type[whatever], type) == type. I added more tests to ensure this in a new commit.

@ilevkivskyi
Copy link
Member Author

@ddfisher Are you happy with this PR? Or should I make more changes? (If you don't have time now, just ignore this ping)

@ilevkivskyi
Copy link
Member Author

@ddfisher Sorry for pinging again. Are you happy with this PR now?

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

Successfully merging this pull request may close these issues.

3 participants