-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[red-knot] Fix .to_instance()
for union types
#13319
Conversation
|
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.
// TODO: converting to `Unknown` here is probably correct, | ||
// but should result in a diagnostic reporting the use of an unbound name: |
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 diagnostic should be handled by just visiting all Name exprs with Load context and checking if their type is unbound, it shouldn't be handled in every method of Type
where we see an Unbound
. So I'm not sure we need a TODO for it here.
(TBH I'm getting more and more convinced that Unbound
should not be a Type
variant at all.)
Type::Class(class) => Type::Instance(*class), | ||
_ => Type::Unknown, // TODO type errors | ||
Type::Union(union) => union.map(db, |element| element.to_instance(db)), | ||
// TODO: we can probably do better here: --Alex |
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 - it should just be a map over the elements resulting in a new intersection, as far as positive elements of the intersection go. But negative elements make it a bit tricky. I think maybe they would just get discarded, but I need to think about it more. Doesn't need to be handled in this PR, anyway.
No description provided.