-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[ty] Eagerly evaluate types.UnionType elements as type expressions
#21531
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
Conversation
| builder = builder.add(if inferred_as.type_expression() { | ||
| *element | ||
| } else { | ||
| element.in_type_expression(db, scope_id, typevar_binding_context)? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The previous version was also only able to surface the first error. I'm not sure if this is a problem, or something that we can live with.
| UnionTypeInstance::PEP604(instance) => { | ||
| // Cloning here is cheap if the result is a `Type` (which is `Copy`). It's more | ||
| // expensive if there are errors. | ||
| instance.union_type(db).clone() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@carljm In the end, this didn't require a new salsa query. Instead, we're now eagerly building that Type::Union in case there were no errors, and interning the result. And then we only need to copy the result here when we see a use of this implicit type alias in a type expression.
This means that we do some extra work if someone defines IntOrStr = int | str and then never uses it as a type. But that feels okay to me?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that seems reasonable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's potentially more problematic is the fact that when we have a LargeUnion = C1 | C2 | … | Cn, we would build union_type field into a salsa query.
41258a6 to
d058e94
Compare
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
d058e94 to
41ecb33
Compare
|
41ecb33 to
3dc7c48
Compare
crates/ty_python_semantic/resources/mdtest/narrow/isinstance.md
Outdated
Show resolved
Hide resolved
3dc7c48 to
08a1f4e
Compare
| reveal_type(callable_or_int) # revealed: ((str, /) -> bytes) | int | ||
| # TODO should be Unknown | int | ||
| reveal_type(type_var_or_int) # revealed: T@_ | int | ||
| reveal_type(type_var_or_int) # revealed: typing.TypeVar | int |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We still don't support generic implicit type aliases, but this is a step in the right direction. T@_ is just wrong.
| // TODO: add support for PEP 604 union types on the right hand side of `isinstance` | ||
| // and `issubclass`, for example `isinstance(x, str | (int | float))`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was recently added
AlexWaygood
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!

Summary
Eagerly evaluate the elements of a PEP 604 union in value position (e.g.
IntOrStr = int | str) as type expressions and store the result (the correspondingType::Unionif all elements are valid type expressions, or the first encounteredInvalidTypeExpressionError) on theUnionTypeInstance, such that theType::Union(…)does not need to be recomputed every time the implicit type alias is used in a type annotation.This might lead to performance improvements for large unions, but is also necessary for correctness, because the elements of the union might refer to type variables that need to be looked up in the scope of the type alias, not at the usage site.
Test Plan
New Markdown tests