-
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
Conversation
|
I want to see more logs to decide where to start, but it seems doesn't show more logs than Any thing wrong with my parameters? @AlexWaygood It could be great if you can provide more tricks |
can have some logs, can we also make them available for |
… type checking This new method checks if a type is a union containing single-valued types, booleans, or literal strings, improving type handling in the semantic model.
…rowingConstraintsBuilder` This update introduces a debug log statement in the `evaluate_expr_in` method to trace type evaluations. Additionally, it implements the `evaluate_expr_not_in` method, enhancing type handling for expressions that check membership exclusion in union types. This improves the semantic model's capability to manage complex type scenarios.
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
|
carljm
left a comment
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.
This looks great, thank you! I have a few comments: some that I think should definitely be addressed in this PR (avoiding tripled code that can be simplified, adding at least some tests for bool / LiteralString and enums) and some that could be addressed in this PR but could also be follow-ups (actually supporting bool/LiteralString/enum, the unification with == narrowing).
| .any(|ty| ty.is_single_valued(db) || ty.is_bool(db) || ty.is_literal_string()) | ||
| }) || self.is_bool(db) | ||
| || self.is_literal_string() |
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 include bool and LiteralString here as types that make a type be a "union with single valued", but where we actually use this method, we only really handle unions, and we don't do any special handling for bool or LiteralString. So I don't think there's currently any benefit to including them here.
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 bool, LiteralString and enum types on the LHS -- I think that would illuminate cases that we'd want to handle. They could stay TODOs in this PR, if they aren't easy to handle.
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.
https://github.com/astral-sh/ruff/pull/20164/files#diff-4d769c34388fdd9011945264bed8fb2199b60ffefcbaa09d0804e4e6c347fd4dR134
added some TODOs in the in.md. If the tests looks good to you, I will take a look tomorrow to see if it's doable for me in the near future.
| } else { | ||
| None | ||
| } | ||
| } else if lhs_ty.is_union_with_single_valued(self.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.
I think that our handling of == narrowing is also missing this same logic, when the LHS isn't all single-valued types, but does contain some single-valued types that could be eliminated.
Really, == and in narrowing are closely related: == is equivalent to in with just a single element on the RHS. Right now are implementations of them are totally separate. Is it possible that we could add support for this case to our == narrowing, and then re-implement our in narrowing in terms of repeated application of our == narrowing?
One last thought: rather than using all of is_single_valued and is_union_of_single_valued and now also is_union_with_single_valued, where the latter two each also have complicated semantics involving special handling of bool and LiteralString (and probably also should include enum types, but don't), I think it might be clearer if we just eliminate those methods and use direct matches on lhs_ty here?
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.
I tried use the in/not_in to replace the eq/not_eq, got many errors(11) include
crates/ty_python_semantic/resources/mdtest/scopes/global.md:84 unexpected error: [invalid-assignment] "Object of type `int | None` is not assignable to `int`"
crates/ty_python_semantic/resources/mdtest/type_compendium/integer_literals.md:77 unmatched assertion: revealed: int & ~Literal[54165]
crates/ty_python_semantic/resources/mdtest/type_compendium/integer_literals.md:77 unexpected error: 21 [revealed-type] "Revealed type: `int`"
crates/ty_python_semantic/resources/mdtest/narrow/while.md:57 unmatched assertion: revealed: Literal[3]
crates/ty_python_semantic/resources/mdtest/narrow/while.md:57 unexpected error: 21 [revealed-type] "Revealed type: `Literal[1, 2, 3]`"
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.
| <!-- TODO: Add enum tests --> | ||
| <!-- |
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.
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.
Looks good, thank you! Pushed some updates and will go ahead and merge.
I think the highest priority follow-up would be to add enum support and address those TODOs. If you are interested, I think this would not be too hard. In principle it is just adding enums (along with bool and LiteralString) in is_union_of_single_valued and is_union_with_single_valued, and in the union logic in evaluate_expr_in and evaluate_expr_not_in. The one wrinkle is that we only consider enums single-valued if they don't override __eq__ or __ne__. This logic is currently buried inside the EnumLiteral branch of Type::is_single_valued; we would need to pull it out into a top-level Type::overrides_eq_or_ne method, since we would also need to call it from is_union_of_single_valued etc. (Should probably also add some tests showing we don't do this narrowing for an enum class that does override __eq__ or __ne__.)
…e__` (#20285) Add equality narrowing for enums, if they don't override `__eq__` or `__ne__` in an unsafe way. Follow-up to PR #20164 Fixes astral-sh/ty#939
…owing (astral-sh#20164) solves astral-sh/ty#939 --------- Co-authored-by: Carl Meyer <[email protected]>
…e__` (astral-sh#20285) Add equality narrowing for enums, if they don't override `__eq__` or `__ne__` in an unsafe way. Follow-up to PR astral-sh#20164 Fixes astral-sh/ty#939
solves astral-sh/ty#939