Repository navigation
Prevent quadratic runtime with generic overloads - #22393
hauntsaninja wants to merge 3 commits into
Conversation
4c0024b to
ce5d0ff
Compare
ce5d0ff to
a54ffb7
Compare
This comment has been minimized.
This comment has been minimized.
ffb8683 to
fb37fc1
Compare
for more information, see https://pre-commit.ci
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
ilevkivskyi
left a comment
ilevkivskyi
left a comment
There was a problem hiding this comment.
Thanks! General direction looks good, but have few comments.
|
|
||
| max_overload_count = 50 | ||
| max_expanded_count = 200 | ||
| if len(defn.items) >= max_overload_count: |
There was a problem hiding this comment.
Do we need to keep two separate limits? I think we should only have the expanded limit (and since type variables with values are rare, this would effectively act as total limit in most cases).
| sig_expanded = expand_callable_variants(detach_callable(sig, type_vars)) | ||
| expanded_signatures.append(sig_expanded) | ||
| expanded_count += len(sig_expanded) | ||
| del item |
There was a problem hiding this comment.
Why is this needed? If this collides with other loop variable below, I would just use a different name.
| signatures: list[CallableType | None] = [] | ||
| expanded_signatures: list[list[CallableType] | None] = [] | ||
| expanded_count = 0 | ||
| with state.strict_optional_set(True): |
There was a problem hiding this comment.
Do you really need this context manger here? I think code here can never reach to any is_subtype() calls.
| ctx_item = defn.items[i + j + 1] | ||
| assert isinstance(ctx_item, Decorator) | ||
| self.msg.overloaded_signature_will_never_match(i + 1, i + j + 2, ctx_item.func) | ||
| elif not is_descriptor_get and expanded_count < max_expanded_count: |
There was a problem hiding this comment.
It looks like unlike for the "old" limit (that does a sliding window) here we skip the check completely. This means that expanding the callables was wasteful. I think we then should simply pre-calculate the number of expanded items (as a sum of products of number of type variable values for each item), and only do the actual expansion if we are under the limit.
This will be more efficient, and will allow to simplify the logic (e.g. you will not need to accept callable or list of callables in is_unsafe_overlapping_overload_signatures()).
Fixes #20439