[red-knot] Add cycle handling to narrow constraints queries #17209
Conversation
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
cake-monotone
requested review from
AlexWaygood,
carljm,
dcreager and
sharkdp
as code owners
Contributor
|
carljm
approved these changes
Apr 4, 2025
Contributor
carljm
left a comment
carljm
left a comment
There was a problem hiding this comment.
This is great, thank you!
We discussed just today that ideally we would prefer to add minimized reproductions of the cycle to our test suite (not just add the full project in the ecosystem check) whenever we add cycle handling to a new query. This ensures we understand the cause of the cycle (and it really should be a cycle), and helps protects against regressions in a way that is easier to debug than a full ecosystem check failure.
Do you feel up for doing that? I realize it's a lot of extra work to minimize the reproduction case.
If not, I'm also open to merging this and creating an issue to make a minimized repro as follow-up.
Contributor
Author
|
I think it makes sense to handle that as a follow-up. The main reason is that we need a test feature for cycle checking. We need a test that ignores all diagnostics and only checks for panic. If we decide to use #[panic_check_only]
`filename.py`:
```py
something_wrong_code
```I actually wrote panic-inducing code like the one below, but adding from __future__ import annotations
class CycleIssue:
def f(self, a: A, b: B, c: C):
aa: AA = self.make_aa(a, FALLBACK)
bb: BB = self.make_bb(b, FALLBACK)
cc: CC = self.make_cc(c, FALLBACK)
is_aa_not_fallback = aa is not FALLBACK
is_bb_not_fallback = bb is not FALLBACK
if is_aa_not_fallback and is_bb_not_fallback:
if aa == bb:
return None
if cc.val is RAISE:
raise Exception1(aa, bb)
if cc.val is DROP_NEW:
return None
elif is_aa_not_fallback:
if cc.key is RAISE:
raise Exception2(aa)
if cc.key is DROP_NEW:
return None
elif is_bb_not_fallback:
if cc.val is RAISE:
raise Exception3(bb)
if cc.val is DROP_NEW:
return None
return aa, bb, cc |
Contributor
|
We actually have this test feature already, it's just not part of mdtest. We place sample test files in But I am fine merging this and adding corpus tests for these panics as a follow-up. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
Summary
This PR fixes the cycle issue that was causing problems in the
support superPR.Affected queries
all_narrowing_constraints_for_expressionall_negative_narrowing_constraints_for_expression--
Additionally,
bidictandwerkzeughave been added to the project-selection list inmypy_primer.This PR also addresses the panics that occurred while analyzing those packages:
bidict: panic triggered byall_narrowing_constraints_for_expressionwerkzeug: panic triggered byall_negative_narrowing_constraints_for_expressionI think the mypy-primer results for this PR can serve as sufficient test :)