-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[red-knot] Fix more edge cases for intersection simplification with LiteralString
and AlwaysTruthy
/AlwaysFalsy
#15496
Conversation
c7d4465
to
1408876
Compare
Briefly deleted my whole patch there by accident 😅 it's back now |
Once we understand differently ordered intersections as equivalent to each other (which I'm working on), we'll be able to add a property test that would have caught this. I've noted it down as a TODO for myself. |
No change on the codspeed benchmarks |
|
// `AlwaysTruthy & LiteralString` -> `LiteralString & ~Literal[""]` | ||
Type::LiteralString if self.positive.swap_remove(&Type::AlwaysTruthy) => { | ||
self.add_positive(db, Type::LiteralString); | ||
self.add_negative(db, Type::string_literal(db, "")); | ||
} | ||
// `AlwaysFalsy & LiteralString` -> `Literal[""]` | ||
Type::LiteralString if self.positive.swap_remove(&Type::AlwaysFalsy) => { | ||
self.add_positive(db, Type::string_literal(db, "")); | ||
} | ||
// `LiteralString & ~AlwaysTruthy` -> `LiteralString & AlwaysFalsy` -> `Literal[""]` | ||
Type::LiteralString if self.negative.swap_remove(&Type::AlwaysTruthy) => { | ||
self.add_positive(db, Type::string_literal(db, "")); | ||
} | ||
// `LiteralString & ~AlwaysFalsy` -> `LiteralString & ~Literal[""]` | ||
Type::LiteralString if self.negative.swap_remove(&Type::AlwaysFalsy) => { | ||
self.add_positive(db, Type::LiteralString); | ||
self.add_negative(db, Type::string_literal(db, "")); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's tempting to think that we could avoid some indirection here, e.g.
// `AlwaysTruthy & LiteralString` -> `LiteralString & ~Literal[""]` | |
Type::LiteralString if self.positive.swap_remove(&Type::AlwaysTruthy) => { | |
self.add_positive(db, Type::LiteralString); | |
self.add_negative(db, Type::string_literal(db, "")); | |
} | |
// `AlwaysFalsy & LiteralString` -> `Literal[""]` | |
Type::LiteralString if self.positive.swap_remove(&Type::AlwaysFalsy) => { | |
self.add_positive(db, Type::string_literal(db, "")); | |
} | |
// `LiteralString & ~AlwaysTruthy` -> `LiteralString & AlwaysFalsy` -> `Literal[""]` | |
Type::LiteralString if self.negative.swap_remove(&Type::AlwaysTruthy) => { | |
self.add_positive(db, Type::string_literal(db, "")); | |
} | |
// `LiteralString & ~AlwaysFalsy` -> `LiteralString & ~Literal[""]` | |
Type::LiteralString if self.negative.swap_remove(&Type::AlwaysFalsy) => { | |
self.add_positive(db, Type::LiteralString); | |
self.add_negative(db, Type::string_literal(db, "")); | |
} | |
// `AlwaysTruthy & LiteralString` -> `LiteralString & ~Literal[""]` | |
Type::LiteralString if self.positive.swap_remove(&Type::AlwaysTruthy) => { | |
self.positive.insert(Type::LiteralString); | |
self.negative.insert(Type::string_literal(db, "")); | |
} | |
// `AlwaysFalsy & LiteralString` -> `Literal[""]` | |
Type::LiteralString if self.positive.swap_remove(&Type::AlwaysFalsy) => { | |
self.positive.insert(Type::string_literal(db, "")); | |
} | |
// `LiteralString & ~AlwaysTruthy` -> `LiteralString & AlwaysFalsy` -> `Literal[""]` | |
Type::LiteralString if self.negative.swap_remove(&Type::AlwaysTruthy) => { | |
self.positive.insert(Type::string_literal(db, "")); | |
} | |
// `LiteralString & ~AlwaysFalsy` -> `LiteralString & ~Literal[""]` | |
Type::LiteralString if self.negative.swap_remove(&Type::AlwaysFalsy) => { | |
self.positive.insert(Type::LiteralString); | |
self.negative.insert(Type::string_literal(db, "")); | |
} |
but this causes many property test failures, because we skip the simplifications done in the fallback branch of this function where redundant supertypes are removed if subtypes exist in the intersection, and where the entire intersection simplifies to Never
if it contains a pair of disjoint types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
Summary
Re-reading the diff in bcf0a71, I realised that there were still one or two cases I failed to handle properly. Namely, we currently simplify
LiteralString & AlwaysTruthy
toLiteralString & ~Literal[""]
, but we do not do the same simplification forAlwaysTruthy & LiteralString
. Similarly, we currently simplifyLiteralString & ~AlwaysFalsy
toLiteralString & ~Literal[""]
, but we don't do the same for~AlwaysFalsy & LiteralString
.This PR fixes those cases, and moves all
LiteralString
handling to the top of theadd_positive()
method so that it's easier to think through the various cases and see that they're both complete and correct. The default diff that GitHub shows is a bit messy because the refactoring toadd_positive
means that the indentation increases by one level for most of the function. Everything is much easier to review if you select GitHub's option to view the diff without whitespace changes, however.Test Plan
cargo test -p red_knot_python_semantic
QUICKCHECK_TESTS=200000 cargo test --release -p red_knot_python_semantic -- --ignored types::property_tests::stable
uvx pre-commit run -a