[ty] equality narrowing on enums that don't override __eq__ or __ne__
#20285
Conversation
|
Thank you for your contribution. It looks like a better format here would be a GitHub issue on the https://github.com/astral-sh/ty repo? Or is there a particular reason that you are opening a PR at this stage? |
|
If you plan to do more changes in this draft PR, that's totally fine. I just thought you were looking for feedback on that document you added here. In that case, I'll assume that we wait for this to move to in-review. |
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
Renkai
requested review from
AlexWaygood,
carljm,
dcreager and
sharkdp
as code owners
Renkai
changed the title
|
|
Thanks! Could you possibly update your PR description to describe this change a little better? |
carljm
left a comment
carljm
left a comment
There was a problem hiding this comment.
This is looking really good! I think it gets the correct semantics. A few comments on the implementation.
| let check_dunder = |dunder_name, allowed_return_value| { | ||
| // Note that we do explicitly exclude dunder methods on `object`, `int` and `str` here. | ||
| // The reason for this is that we know that these dunder methods behave in a predictable way. | ||
| // Only custom dunder methods need to be examined here, as they might break single-valuedness | ||
| // by always returning `False`, for example. | ||
| let call_result = self.try_call_dunder_with_policy( | ||
| db, | ||
| dunder_name, | ||
| &mut CallArguments::positional([Type::unknown()]), | ||
| MemberLookupPolicy::MRO_NO_OBJECT_FALLBACK | ||
| | MemberLookupPolicy::MRO_NO_INT_OR_STR_LOOKUP, | ||
| ); | ||
| let call_result = call_result.as_ref(); | ||
| call_result.is_ok_and(|bindings| { | ||
| bindings.return_type(db) == Type::BooleanLiteral(allowed_return_value) | ||
| }) || call_result | ||
| .is_err_and(|err| matches!(err, CallDunderError::MethodNotAvailable)) | ||
| }; | ||
|
|
||
| check_dunder("__eq__", true) && check_dunder("__ne__", false) |
There was a problem hiding this comment.
We shouldn't duplicate all this code from is_single_valued; we should extract it as a separate method, e.g. Type::overrides_equality. (It doesn't even have to be specific to enums, even if we only call it for enums currently.)
| /// Return true if this is an enum literal or enum class instance that doesn't override __eq__ or __ne__ | ||
| fn is_simple_enum(&self, db: &'db dyn Db) -> bool { | ||
| let is_enum = match self { | ||
| Type::EnumLiteral(_) => true, |
There was a problem hiding this comment.
No mdtests fail if we remove this arm. I think that's because we only use this method as an additional check beyond is_single_valued, and enum literals are already always single-valued.
I think we can probably remove this is_simple_enum method altogether in favor of an is_enum method, and use ty.is_enum() && !ty.overrides_equality() everywhere the PR currently uses ty.is_simple_enum(). This isn't quite as concise but I think it's clearer, as its meaning is more self-evident, where the meaning of "simple enum" is not.
There was a problem hiding this comment.
Thanks for the review! I fixed it followed by your instruction, but cargo clippy forced me to use !element.is_enum(self.db) || element.overrides_equality(self.db) instead, hope it's still good to you.
carljm
changed the title
__eq__ or __ne__
…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
4 participants
It's a TODO clean of PR #20164 that solved issue astral-sh/ty#939