-
-
Notifications
You must be signed in to change notification settings - Fork 3k
[mypyc] feat: specialize isinstance for tuple of primitive types #19949
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
[mypyc] feat: specialize isinstance for tuple of primitive types #19949
Conversation
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
…obTheBuidler/mypy into isinstance-tuple-of-primitives
Not really sure where a run test best fits. Maybe the IR test is straightforward enough? |
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 about adding a run test to run-misc.test
?
from typing import Any | ||
|
||
def is_instance(x: Any) -> bool: | ||
return isinstance(x, (str, int, bytes)) |
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.
Test also single-item tuple as an edge case.
Both of those just as run tests, or IR too? |
Added run tests for various cases. no additional IR tests at this time. |
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.
Thanks for adding the tests! Looks mostly good now, just one thing remains.
mypyc/irbuild/specialize.py
Outdated
is_last = i == len(descs) - 1 | ||
next_block = fail_block if is_last else BasicBlock() | ||
builder.add_bool_branch( | ||
builder.primitive_op(desc, [obj], expr.line), pass_block, next_block # type: ignore [arg-type] |
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.
Can you avoid the # type: ignore
? If there is some typing issue, I'd prefer a cast
since it's more specific and tells which argument is the problematic one.
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.
Done. It was because descs
has type list[PrimitiveDescription | None]
I'm assuming the if None in descs: return None
can probably be considered as a TypeGuard of sorts by mypy but that's above my paygrade
for more information, see https://pre-commit.ci
is_last = i == len(descs) - 1 | ||
next_block = fail_block if is_last else BasicBlock() | ||
builder.add_bool_branch( | ||
builder.primitive_op(cast(PrimitiveDescription, desc), [obj], expr.line), |
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.
Here a slightly better way to narrow the type would be to use assert desc is not None
before the call, but this is fine as well.
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.
Thanks!
This PR specializes isinstance calls where the type argument is a tuple of primitive types.
We can skip tuple creation and the associated refcounting, and daisy-chain the primitive checks with an early exit option at each step.