more enum-related speedups#12032
Conversation
sobolevn
left a comment
sobolevn
left a comment
There was a problem hiding this comment.
Thank you!
I think it will be fair to say that I don't understand what is going on in this PR, but I hope to help you by fixing some minor things 😄
mypy/meet.py
Outdated
| return ( | ||
| isinstance(x, Instance) and x.type.is_enum and | ||
| isinstance(y, UnionType) and | ||
| all(isinstance(z, LiteralType) and z.fallback.type == x.type # type: ignore[misc] |
There was a problem hiding this comment.
Why do we need type: ignore here? 🤔
There was a problem hiding this comment.
because when mypy typechecks itself it complains about using isinstance on values of type Type, suggesting running them through get_proper_type first (which I don't think is appropriate here) or using this type: ignore
mypy/meet.py
Outdated
| return ( | ||
| isinstance(x, LiteralType) and isinstance(y, UnionType) and any( | ||
| isinstance(z, LiteralType) and z == x # type: ignore[misc] | ||
| for z in y.items |
mypy/meet.py
Outdated
| # and crucially, we want to do that *fast* in case the enum is large | ||
| # so we do it before expanding variants below to avoid O(n**2) behavior | ||
| if ( | ||
| is_enum_overlapping_union(left, right) or |
There was a problem hiding this comment.
style nit: we use \n or is_enum_overlapping_union(...
mypy/subtypes.py
Outdated
| # avoid redundant check for union of literals | ||
| for item in left.items: | ||
| if mypy.typeops.is_simple_literal(item): | ||
| assert isinstance(item, LiteralType) # type: ignore[misc] |
There was a problem hiding this comment.
It might be a good idea to declare item = get_proper_type(item)
There was a problem hiding this comment.
or I suppose I might be able to use the new TypeGuard feature here?
There was a problem hiding this comment.
Yes 🙂
But, I am not sure that mypyc supports it.
There was a problem hiding this comment.
black is compiled with mypyc and you added TypeGuard to it @sobolevn so it's at least worth a shot imo 🙂
There was a problem hiding this comment.
well OK it turns out I misremembered sobolevn for someone else, but anyway the first half of my still stands 😅
This comment has been minimized.
This comment has been minimized.
|
Any chance to get this merged? |
| return applied | ||
|
|
||
|
|
||
| def try_restrict_literal_union(t: UnionType, s: Type) -> Optional[List[Type]]: |
There was a problem hiding this comment.
Could you add a docstring to this function?
As a followup to python#9394 address a few more O(n**2) behaviors caused by decomposing enums into unions of literals.
huguesb
force-pushed
the
pr-enum-speedups
branch
from
2aa1337 to
774b95c
Compare
|
Rebased on top of master after recent union-related changes |
This comment has been minimized.
This comment has been minimized.
mypy/meet.py
Outdated
| all(x.type == p.fallback.type | ||
| for p in (get_proper_type(z) for z in y.relevant_items()) | ||
| if isinstance(p, LiteralType)) |
There was a problem hiding this comment.
| all(x.type == p.fallback.type | |
| for p in (get_proper_type(z) for z in y.relevant_items()) | |
| if isinstance(p, LiteralType)) | |
| all(isinstance(p, LiteralType)) and x.type == p.fallback.type | |
| for p in (get_proper_type(z) for z in y.relevant_items())) |
Should it be this instead? As I understand it, this function checks that x is an enum and y is a union containing only members of x.
It would be helpful to add a docstring to this function.
There was a problem hiding this comment.
Upon looking at this again, I think the previous implementation was wrong since we only care about partial overlap. Fixed and documented accordingly.
mypy/sametypes.py
Outdated
| def visit_union_type(self, left: UnionType) -> bool: | ||
| if isinstance(self.right, UnionType): | ||
| # fast path for simple literals | ||
| def _extract_literals(u: UnionType) -> Tuple[Set[Type], List[Type]]: |
There was a problem hiding this comment.
I believe nested functions don't perform well with mypyc, can you make this a global function instead?
mypy/subtypes.py
Outdated
|
|
||
|
|
||
| def try_restrict_literal_union(t: UnionType, s: Type) -> Optional[List[Type]]: | ||
| """Helper function for restrict_subtype_away, allowing a fast path for Union of simple literals""" |
There was a problem hiding this comment.
This docstring doesn't say what the function does, which is the part I was having trouble with. What are the parameters and what does it return?
|
According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉 |
JelleZijlstra
left a comment
JelleZijlstra
left a comment
There was a problem hiding this comment.
Thanks for the clarifications!
As a followup to #9394 address a few more O(n**2) behaviors caused by decomposing enums into unions of literals.
…apping with enum (#18897) Fixes #18895. The original implementation of that block was introduced as a performance optimization in #12032. It's in fact incorrect: it produces overly optimistic meets, assuming that *any* match among union items makes them *all* relevant. As discussed in #18895, this actually results in unexpected `meet` behaviour, as demonstrated by ```python from enum import Enum from typing_extensions import TypeIs, Literal class Model(str, Enum): A = 'a' B = 'a' def is_model_a(model: str) -> TypeIs[Literal[Model.A, "foo"]]: return True def handle(model: Model) -> None: if is_model_a(model): reveal_type(model) # N: Revealed type is "Union[Literal[__main__.Model.A], Literal['foo']]" def is_int_or_list(model: object) -> TypeIs[int | list[int]]: return True def compare(x: int | str) -> None: if is_int_or_list(x): reveal_type(x) # N: Revealed type is "builtins.int" ``` This patch restores filtering of union members, but keeps it running before the expensive `is_overlapping_types` check involving expansion.
5 participants
As a followup to #9394 address a few more O(n**2) behaviors
caused by decomposing enums into unions of literals.