[red-knot] Type narrowing for assertions#17149
Merged
carljm merged 8 commits intoastral-sh:mainfrom Apr 10, 2025
Merged
Conversation
MatthewMckee4
commented
Apr 2, 2025
Contributor
Author
assert "", b"hi" # [assert-on-string-literal]This is panicking, not exactly sure why, and its because of |
Contributor
Author
|
Tests are failing because class ClassWithBool(list):
def __bool__(self):
return True
class ChildClassWithBool(ClassWithBool):
pass
assert len(ClassWithBool())
assert len(ChildClassWithBool())with thread '<unnamed>' panicked at /home/matthew/.cargo/git/checkouts/salsa-e6f3bb7c2a062968/d758691/src/function/fetch.rs:154:25:
dependency graph cycle querying member_lookup_with_policy_(Id(2816)); set cycle_fn/cycle_initial to fixpoint iterate
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Rayon: detected unexpected panic; aborting
Aborted (core dumped)But, weirdly, if i comment out either assert statement, this runs just fine, it only panics when both of the assert statements are there |
Contributor
Author
|
If any is able to look into this that would be great. The bug is happening around ruff/crates/red_knot_python_semantic/src/types/class.rs Lines 253 to 256 in 4f924bb |
Contributor
|
Contributor
Author
|
I think the error is coming from me changing the |
MatthewMckee4
requested review from
AlexWaygood,
carljm,
dcreager and
sharkdp
as code owners
Contributor
|
A few comments here. Generally looks good! The cycle failure here in the corpus tests is a symptom of a larger problem, which I discussed (along with some possible fixes) in Discord. We could probably paper over it by adding fixpoint cycle handling to |
Contributor
Author
|
@carljm did you mean to add review comments? |
carljm
reviewed
Apr 5, 2025
Contributor
carljm
left a comment
carljm
left a comment
There was a problem hiding this comment.
Oh oops, yeah I did! Mostly looks great though.
Contributor
Author
|
Thanks! |
Contributor
Author
|
Something isnt implemented for some literal values from typing import Literal
def _(x: Literal[1, 2, 3], y: Literal[1, 2, 3]):
assert x is 2
reveal_type(x) # revealed: Literal[2]
assert y == 2
reveal_type(y) # revealed: Literal[1, 2, 3]This passes |
Contributor
Author
|
Maybe i remove this test and make a new PR for literal ints, bools strings and bytes? |
Contributor
|
Nah just leave it as is, it's fine. |
carljm
force-pushed
the
narrowing-for-assertions
branch
2 times, most recently
from
8963635 to
9429c8d
Compare
Contributor
|
Rebased this on top of #17326, which eliminates the cycle panic previously seen here. That PR can't land until salsa-rs/salsa#790 lands in Salsa. |
Contributor
|
carljm
reviewed
Apr 10, 2025
Co-authored-by: Carl Meyer <carl@oddbird.net>
carljm
force-pushed
the
narrowing-for-assertions
branch
from
2031cc2 to
488c6f7
Compare
AlexWaygood
approved these changes
Apr 10, 2025
carljm
added a commit
that referenced
this pull request
Apr 10, 2025
dcreager
added a commit
that referenced
this pull request
Apr 10, 2025
* main: (30 commits) [red-knot] Silence `unresolved-import` in unreachable code (#17336) red_knot_python_semantic: move TODO comment red_knot_python_semantic: rename `lint()` and `report()` ruff_db: use `Annotation::get_message` in more places red_knot_python_semantic: tweak docs on building reporter builders red_knot_python_semantic: remove the "old" secondary message type red_knot_python_semantic: replace one use of "old" secondary diagnostic messages red_knot_python_semantic: update revealed type snapshots ruff_db: tweak how the revealed type diagnostic is rendered red_knot: add explicit test for concise `reveal_type` diagnostic red_knot_python_semantic: remove `InferContext::report_diagnostic` red_knot_python_semantic: add "reporter" API Bump 0.11.5 (#17337) [red-knot] Silence `unresolved-attribute` in unreachable code (#17305) Revert "[red-knot] Type narrowing for assertions (#17149)" (#17335) [red-knot] Type narrowing for assertions (#17149) [red-knot] avoid unnecessary evaluation of visibility constraint on definitely-unbound symbol (#17326) update cargo-dist (#17325) [red-knot] Fix double hovers/inlays in playground (#17334) [red-knot] Track reachability of scopes (#17332) ...
carljm
added a commit
that referenced
this pull request
Apr 11, 2025
## Summary Fixes #17147 ## Test Plan Add new narrow/assert.md test file --------- Co-authored-by: Carl Meyer <carl@astral.sh>
carljm
added a commit
that referenced
this pull request
Apr 18, 2025
## Summary Fixes #17147 ## Test Plan Add new narrow/assert.md test file --------- Co-authored-by: Carl Meyer <carl@astral.sh>
carljm
added a commit
that referenced
this pull request
Apr 18, 2025
## Summary Fixes #17147 ## Test Plan Add new narrow/assert.md test file --------- Co-authored-by: Carl Meyer <carl@astral.sh>
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
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
Fixes #17147
Test Plan
Add new narrow/assert.md test file