-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[ty]eliminate definitely-impossible types from union in equality narrowing #20164
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
Changes from 14 commits
b373ee2
cfc80c4
046939f
4a7cd62
445dd63
f82b66c
a0bba1c
b31353a
752d179
6de458c
8a99037
b31540e
a10bc0c
cf160f0
7470d25
0462016
96ada71
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1054,6 +1054,16 @@ impl<'db> Type<'db> { | |
| || self.is_literal_string() | ||
| } | ||
|
|
||
| pub(crate) fn is_union_with_single_valued(&self, db: &'db dyn Db) -> bool { | ||
| self.into_union().is_some_and(|union| { | ||
| union | ||
| .elements(db) | ||
| .iter() | ||
| .any(|ty| ty.is_single_valued(db) || ty.is_bool(db) || ty.is_literal_string()) | ||
| }) || self.is_bool(db) | ||
| || self.is_literal_string() | ||
|
Comment on lines
+1062
to
+1064
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We include And probably enums should be considered as well as an effective "union of single-valued types" both here and above. Maybe we could at least add some tests that have
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. https://github.com/astral-sh/ruff/pull/20164/files#diff-4d769c34388fdd9011945264bed8fb2199b60ffefcbaa09d0804e4e6c347fd4dR134 |
||
| } | ||
|
|
||
| pub(crate) fn into_string_literal(self) -> Option<StringLiteralType<'db>> { | ||
| match self { | ||
| Type::StringLiteral(string_literal) => Some(string_literal), | ||
|
|
@@ -9956,14 +9966,6 @@ impl<'db> StringLiteralType<'db> { | |
| pub(crate) fn python_len(self, db: &'db dyn Db) -> usize { | ||
| self.value(db).chars().count() | ||
| } | ||
|
|
||
| /// Return an iterator over each character in the string literal. | ||
| /// as would be returned by Python's `iter()`. | ||
| pub(crate) fn iter_each_char(self, db: &'db dyn Db) -> impl Iterator<Item = Self> { | ||
| self.value(db) | ||
| .chars() | ||
| .map(|c| StringLiteralType::new(db, c.to_string().into_boxed_str())) | ||
| } | ||
| } | ||
|
|
||
| /// # Ordering | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -617,22 +617,78 @@ impl<'db, 'ast> NarrowingConstraintsBuilder<'db, 'ast> { | |
|
|
||
| fn evaluate_expr_in(&mut self, lhs_ty: Type<'db>, rhs_ty: Type<'db>) -> Option<Type<'db>> { | ||
| if lhs_ty.is_single_valued(self.db) || lhs_ty.is_union_of_single_valued(self.db) { | ||
| if let Type::StringLiteral(string_literal) = rhs_ty { | ||
| Some(UnionType::from_elements( | ||
| self.db, | ||
| string_literal | ||
| .iter_each_char(self.db) | ||
| .map(Type::StringLiteral), | ||
| )) | ||
| } else if let Some(tuple_spec) = rhs_ty.tuple_instance_spec(self.db) { | ||
| // N.B. Strictly speaking this is unsound, since a tuple subclass might override `__contains__` | ||
| // but we'd still apply the narrowing here. This seems unlikely, however, and narrowing is | ||
| // generally unsound in numerous ways anyway (attribute narrowing, subscript, narrowing, | ||
| // narrowing of globals, etc.). So this doesn't seem worth worrying about too much. | ||
| Some(UnionType::from_elements(self.db, tuple_spec.all_elements())) | ||
| } else { | ||
| None | ||
| rhs_ty | ||
| .try_iterate(self.db) | ||
| .ok() | ||
| .map(|iterable| iterable.homogeneous_element_type(self.db)) | ||
| } else if lhs_ty.is_union_with_single_valued(self.db) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that our handling of Really, One last thought: rather than using all of
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tried use the in/not_in to replace the eq/not_eq, got many errors(11) include Maybe we shall create a new thread to discuss about how to combine in and eq, and the proposed behavior change of existing eq function. |
||
| let rhs_values = rhs_ty | ||
| .try_iterate(self.db) | ||
| .ok()? | ||
| .homogeneous_element_type(self.db); | ||
|
|
||
| let mut builder = UnionBuilder::new(self.db); | ||
|
|
||
| // Add the narrowed values from the RHS first, to keep literals before broader types. | ||
| builder = builder.add(rhs_values); | ||
|
|
||
| if let Some(lhs_union) = lhs_ty.into_union() { | ||
| for element in lhs_union.elements(self.db) { | ||
| // Keep only the non-single-valued portion of the original type. | ||
| if !element.is_single_valued(self.db) && !element.is_literal_string() { | ||
| builder = builder.add(*element); | ||
| } | ||
| } | ||
| } | ||
| Some(builder.build()) | ||
| } else { | ||
| None | ||
| } | ||
| } | ||
|
|
||
| fn evaluate_expr_not_in(&mut self, lhs_ty: Type<'db>, rhs_ty: Type<'db>) -> Option<Type<'db>> { | ||
| let rhs_values = rhs_ty | ||
| .try_iterate(self.db) | ||
| .ok()? | ||
| .homogeneous_element_type(self.db); | ||
|
|
||
| if lhs_ty.is_single_valued(self.db) || lhs_ty.is_union_of_single_valued(self.db) { | ||
| // Exclude the RHS values from the entire (single-valued) LHS domain. | ||
| let complement = IntersectionBuilder::new(self.db) | ||
| .add_positive(lhs_ty) | ||
| .add_negative(rhs_values) | ||
| .build(); | ||
| Some(complement) | ||
| } else if lhs_ty.is_union_with_single_valued(self.db) { | ||
| // Split LHS into single-valued portion and the rest. Exclude RHS values from the | ||
| // single-valued portion, keep the rest intact. | ||
| let mut single_builder = UnionBuilder::new(self.db); | ||
| let mut rest_builder = UnionBuilder::new(self.db); | ||
|
|
||
| if let Some(lhs_union) = lhs_ty.into_union() { | ||
| for element in lhs_union.elements(self.db) { | ||
| if element.is_single_valued(self.db) || element.is_literal_string() { | ||
| single_builder = single_builder.add(*element); | ||
| } else { | ||
| rest_builder = rest_builder.add(*element); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| let single_union = single_builder.build(); | ||
| let rest_union = rest_builder.build(); | ||
|
|
||
| let narrowed_single = IntersectionBuilder::new(self.db) | ||
| .add_positive(single_union) | ||
| .add_negative(rhs_values) | ||
| .build(); | ||
|
|
||
| // Keep order: first literal complement, then broader arms. | ||
| let result = UnionBuilder::new(self.db) | ||
| .add(narrowed_single) | ||
| .add(rest_union) | ||
| .build(); | ||
| Some(result) | ||
| } else { | ||
| None | ||
| } | ||
|
|
@@ -660,9 +716,7 @@ impl<'db, 'ast> NarrowingConstraintsBuilder<'db, 'ast> { | |
| ast::CmpOp::Eq => self.evaluate_expr_eq(lhs_ty, rhs_ty), | ||
| ast::CmpOp::NotEq => self.evaluate_expr_ne(lhs_ty, rhs_ty), | ||
| ast::CmpOp::In => self.evaluate_expr_in(lhs_ty, rhs_ty), | ||
| ast::CmpOp::NotIn => self | ||
| .evaluate_expr_in(lhs_ty, rhs_ty) | ||
| .map(|ty| ty.negate(self.db)), | ||
| ast::CmpOp::NotIn => self.evaluate_expr_not_in(lhs_ty, rhs_ty), | ||
| _ => None, | ||
| } | ||
| } | ||
|
|
||
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.
We don't use HTML comments for TODOs, we are fine with TODOs showing up in the rendered output. In fact we use HTML comments for special markers like
<!-- snapshot-diagnostics -->, and for that reason to avoid typos we make unknown HTML comments an error, which is why tests are failing on this PR.I'll fix this so you can see how we usually handle TODOs.