Skip to content

Support instantiation of type[SomeTypeVar] -> SomeTypeVar#14791

Open
sterliakov wants to merge 4 commits intopython:masterfrom
sterliakov:st_bugfix-typevar-type-call
Open

Support instantiation of type[SomeTypeVar] -> SomeTypeVar#14791
sterliakov wants to merge 4 commits intopython:masterfrom
sterliakov:st_bugfix-typevar-type-call

Conversation

@sterliakov
Copy link
Collaborator

@sterliakov sterliakov commented Feb 26, 2023

Fixes #14790.

This PR enables replacing the result of check when calling type[SomeTypeVar] with original SomeTypeVar, if the result is compatible.

@github-actions

This comment has been minimized.

1 similar comment
@github-actions
Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

def f2(x: Type[T2]) -> T2:
return x(1)

def return_indirect(x: Type[T2]) -> T2:
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need indirect tests? 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because return some_immediate_expression_or_literal() and return some_var_with_already_fixed_type are not 100% equivalent (consider Literal or TypedDict return, like this), and I'd like to be sure here. It may have no special treatment now (and it doesn't), but we may have a case in future where similar stricter inference is necessary.

# If it was `type[SomeTypeVar]`, successful instantiation produces SomeTypeVar
return callee.item, callee_type
else:
return result_type, callee_type
Copy link
Collaborator

Choose a reason for hiding this comment

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

Have you investigated why the return type is inferred incorrectly in check_call? It may be better to fix this there instead. Right now this feels like we may have a bug in type inference and we are papering over it by patching the result afterwards. It's still possible that there is some fundamental reason why we can't do the right thing in check_call and this approach is the best option, but I'd prefer to better understand if that is the case.

Copy link
Collaborator Author

@sterliakov sterliakov Mar 7, 2023

Choose a reason for hiding this comment

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

Yes, I considered this, but it looks like this scenario is specific to one context. check_call is not responsible here, the issue arises when analyze_type_type_callee is called with TypeVar(bound=A|B) (tvar with union upper bound). This case is expanded to a Union of two callables corresponding to constructors of union items (line 1589). Currently analyze_type_type_callee gives

Union[def (x: builtins.str) -> a.A, def (x: builtins.int) -> a.B]

Alternative solution would be to handle UnionType there, but it looks less clear to me: we'll have then several TypeVar instances with same ids (so that they still correspond to original TypeVar), but different upper bounds, like the following:

Union[def (x: builtins.str) -> T`-1, def (x: builtins.int) -> T`-1]

where first T`-1 is bound to A and second - to B. This is at least confusing and, I guess, error prone.

The mentioned problem is related to specifically Type[...] instantiation, and it's easier to say that Type[SomeTVar] gives SomeTVar after successful instantiation. This is a "hack" mentioned in comment in analyze_type_type_callee: an upper bound is analyzed instead of typevar itself.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hm, I'm no longer sure that the latter solution is worse.

@JukkaL Is there any precedent of creating TypeVar copies with same id and different bounds? Is there any fundamental reason not to do so? Should I open a separate issue with this question?

With such approach we'll have proper typevar narrowing for free in cases like

_T = TypeVar('_T', bound=int|str)

def foo(x: _T) -> _T:
    if isinstance(x, int): return x
    else: return x

(this currently fails in if due to narrowing that discards TypeVar part)

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.

Class factory with type[TypeVar] with upper bound not checked correctly (false positive)

3 participants