[ty] filter out pre-loop bindings from loop headers#23536
Merged
oconnor663 merged 1 commit intomainfrom Mar 3, 2026
Merged
Conversation
Typing conformance resultsNo changes detected ✅ |
|
Memory usage reportSummary
Significant changesClick to expand detailed breakdowntrio
flake8
sphinx
prefect
|
|
| Lint rule | Added | Removed | Changed |
|---|---|---|---|
invalid-assignment |
0 | 1 | 0 |
| Total | 0 | 1 | 0 |
eab1bb6 to
495557c
Compare
Contributor
Author
|
Rebased and applied the cleanup described here. |
Contributor
|
I think the two ecosystem changes are worth looking into. It took me a while to distill this MRE out of alerta, so I'm posting it here in case it's useful. It produces one additional diagnostic on class Alert: ...
def f(*args, **kwargs):
pass
def some_condition(*args, **kwargs) -> bool:
return False
def process_action(alert: Alert, action: str):
updated = None
while some_condition(alert):
if some_condition(1):
updated = f(alert, action)
elif some_condition(2):
updated = f(alert, action)
if isinstance(updated, Alert):
updated = updated, action, 1
if isinstance(updated, tuple):
if len(updated) == 3:
alert, action, timeout = updated
elif len(updated) == 2:
alert, action = updatedIt would be good to understand if that diagnostic should go away. And if that example is not strange enough, the diagnostic change in manticore is even weirder: - manticore/core/smtlib/visitors.py:641:58: error[unsupported-operator] Operator `-` is not supported between objects of type `None | Unknown` and `None | Unknown`
+ manticore/core/smtlib/visitors.py:641:58: error[unsupported-operator] Operator `-` is not supported between two objects of type `None | Unknown` |
sharkdp
approved these changes
Feb 25, 2026
Contributor
sharkdp
left a comment
There was a problem hiding this comment.
Thank you — the change sounds very plausible and the implementation looks good. I can certainly imagine that this solves some bugs as well. It might therefore be interesting to understand the ecosystem changes better. Maybe there is a useful regression test that we could add?
If those cases seem too exotic though, I'm definitely in favor of merging this, even without a deep analysis. This definitely seems more correct.
Contributor
|
I'm out for a few days, but the review here is finished, so I don't think we need to assign someone new(?). If there are significant changes, please unassign me and close/open if you feel it's necessary to review those. |
Contributor
Author
|
Will do, thanks David. |
495557c to
2a7d651
Compare
Contributor
Author
|
The def process(alert: int):
while alert:
if not alert:
updated = 0
if isinstance(updated, int):
updated = updated, 1
if isinstance(updated, tuple):
if len(updated) == 2:
alert, _ = updated
elif len(updated) == 1:
(alert,) = updatedWith this change it's clean (the type of This seems to be a really complicated cycle, probably something like the one described in this mdtest case where an instance of |
carljm
added a commit
that referenced
this pull request
Mar 3, 2026
* main: [ty] Apply narrowing to walrus values (#23687) [`perflint`] Extend `PERF102` to comprehensions and generators (#23473) [ty] Fix GitHub-annotations mdtest output format (#23694) [ty] Reduce the number of potentially-flaky projects (#23698) [`pydocstyle`] Fix numpy section ordering (`D420`) (#23685) [ty] Move method-related types to a submodule (#23691) [ty] Avoid the mandatory "ecosystem-analyzer workflow run cancelled" notification every time you make a PR (#23695) [ty] Move `Type::subtyping_is_always_reflexive` to `types::relation` (#23692) Update conformance suite commit hash (#23693) [ty] Add mdtest suite for `typing.Concatenate` (#23554) [ty] filter out pre-loop bindings from loop headers (#23536)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reading through @mtshiba's #23520 (specifically
if def != definitionhere) made it clear that the way I included pre-loop bindings in loop headers in #22794 is kind of gross. Apart from being fully redundant (loop headers don't shadow pre-existing bindings), it also means that loop headers tracks themselves, which presumably creates pointless cycles that need to be resolved. Since definition IDs are issued in source code order, it's relatively easy to filter out all the definitions that existed before the loop began, including the loop headers. Anecdotally this seems to give a ~10% speedup inisort, which is the repo that motivated #23520. I'll be curious to see if the ecosystem report shows changed diagnostics, but I'm not sure it will.Before landing this: