KEMBAR78
Narrow type with typevar when the lower bound of its upper bound and that type is same by ethframe · Pull Request #10658 · python/mypy · GitHub
Skip to content

Conversation

ethframe
Copy link
Contributor

@ethframe ethframe commented Jun 16, 2021

Description

Fixes #10605

This PR extends type narrowing by a type from a generic argument. This is achieved by handling a specific case while narrowing some type with TypeVarType: if the lower bound of that type and upper_bound of type variable is the same type as upper_bound, then the type can be narrowed to that type variable.

@ethframe ethframe marked this pull request as draft June 17, 2021 05:04
@ethframe ethframe changed the title Return TypeVarType as the lower bound when upper_bound is a subtype of other type Narrow type with typevar when the lower bound of its upper bound and that type is same Jun 17, 2021
@ethframe ethframe marked this pull request as ready for review June 17, 2021 07:07
mypy/meet.py Outdated
# We'd need intersection types, so give up.
return declared
elif isinstance(narrowed, TypeVarType):
if is_same_type(narrowed.upper_bound, meet_types(narrowed.upper_bound, declared)):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does it work to check is_subtype(narrowed.upper_bound, declared) instead?

Mathematically, the two should be equivalent: x ≤ y if and only if x ∧ y = x (see https://en.wikipedia.org/wiki/Join_and_meet).

In practice, mypy's implementations fall short of mathematical perfection, and I think that is_subtype is more reliable than meet_types, and reads more naturally.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The difference from is_subtype is how the UnboundType is treated. When type of declared is UnboundType, is_subtype will always return True. But since the type of meet_type result in this case is either AnyType, NoneType or UninhabitedType, is_same_type will result in True only if narrowed.upper_bound is also of AnyType, NoneType, UninhabitedType or UnboundType. I thought it is better not to unconditionally narrow unbound type with type variable.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hm. OK, that seems plausible. Do you have an example where we get different behavior between the two? I think that would make it easier for me to consider the difference.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to come up with an example and while I was skimming through typeanal.py to figure out how one can get an unbound type during type checking phase, I've learned about this issue. Seems that there is no need to consider unbound types in the type checker, unless their presence leads to crashes. And since is_subtype works fine with unbound types, I think it is actually a good idea to rewrite the entire PR code like this:

elif isinstance(narrowed, TypeVarType) and is_subtype(narrowed.upper_bound, declared):
    return narrowed

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK, I agree!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@msullivan
Copy link
Collaborator

Sorry for the delay, merging!

@msullivan msullivan merged commit cae5d3c into python:master Jun 29, 2021
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.

regression: typevar with bound and type check causes narrowing to nothing

2 participants