[red-knot] Fix more edge cases for intersection simplification with LiteralString and AlwaysTruthy/AlwaysFalsy#15496
Merged
AlexWaygood merged 1 commit intomainfrom Jan 15, 2025
Merged
Conversation
AlexWaygood
requested review from
MichaReiser,
carljm and
sharkdp
as code owners
AlexWaygood
force-pushed
the
alex/more-intersections
branch
from
c7d4465 to
1408876
Compare
Member
Author
|
Briefly deleted my whole patch there by accident 😅 it's back now |
Member
Author
|
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. |
Member
Author
|
No change on the codspeed benchmarks |
Contributor
|
AlexWaygood
commented
Jan 15, 2025
Comment on lines
+259
to
+276
| // `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, "")); | ||
| } |
Member
Author
There was a problem hiding this comment.
it's tempting to think that we could avoid some indirection here, e.g.
Suggested change
| // `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.
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
2 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
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 & AlwaysTruthytoLiteralString & ~Literal[""], but we do not do the same simplification forAlwaysTruthy & LiteralString. Similarly, we currently simplifyLiteralString & ~AlwaysFalsytoLiteralString & ~Literal[""], but we don't do the same for~AlwaysFalsy & LiteralString.This PR fixes those cases, and moves all
LiteralStringhandling 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_positivemeans 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_semanticQUICKCHECK_TESTS=200000 cargo test --release -p red_knot_python_semantic -- --ignored types::property_tests::stableuvx pre-commit run -a