[ty] filter out pre-loop bindings from loop headers#23536
Open
oconnor663 wants to merge 1 commit intomainfrom
Open
[ty] filter out pre-loop bindings from loop headers#23536oconnor663 wants to merge 1 commit intomainfrom
oconnor663 wants to merge 1 commit intomainfrom
Conversation
oconnor663
requested review from
AlexWaygood,
carljm,
dcreager,
ibraheemdev and
sharkdp
as code owners
Typing conformance resultsNo changes detected ✅ |
|
Memory usage reportSummary
Significant changesClick to expand detailed breakdowntrio
flake8
sphinx
prefect
|
|
| Lint rule | Added | Removed | Changed |
|---|---|---|---|
invalid-await |
0 | 40 | 0 |
invalid-assignment |
0 | 1 | 0 |
invalid-return-type |
0 | 1 | 0 |
unsupported-operator |
0 | 0 | 1 |
| Total | 0 | 42 | 1 |
oconnor663
force-pushed
the
jack/loop_header_bindings_in_loop_only
branch
from
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
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.
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: