Repository navigation
Narrow literals in the negative case even with custom equality - #18574
A5rocks wants to merge 3 commits into
Conversation
Sorry, something went wrong.
| if should_narrow_by_identity: | ||
| return True | ||
| else: | ||
| return not p_t.fallback.type.is_enum |
There was a problem hiding this comment.
Alternative: I could try has_no_custom_eq_checks(p_t.fallback) maybe?
Sorry, something went wrong.
There was a problem hiding this comment.
I don't know any other literals that could possibly have custom __eq__, amd enums are complex enough to have some unexpected corner cases - this check looks reasonable to me as-is. Maybe rewrite as return should_narrow_by_identity or not p_t.fallback.type.is_enum to make it more clear than if/else? And maybe even the whole ladder as just return isinstance(p_t, LiteralType) and (... or ...)?
Sorry, something went wrong.
This comment has been minimized.
This comment has been minimized.
| if should_narrow_by_identity: | ||
| return True | ||
| else: | ||
| return not p_t.fallback.type.is_enum |
There was a problem hiding this comment.
I don't know any other literals that could possibly have custom __eq__, amd enums are complex enough to have some unexpected corner cases - this check looks reasonable to me as-is. Maybe rewrite as return should_narrow_by_identity or not p_t.fallback.type.is_enum to make it more clear than if/else? And maybe even the whole ladder as just return isinstance(p_t, LiteralType) and (... or ...)?
Sorry, something went wrong.
| @@ -845,7 +845,7 @@ x1: Union[Custom, Literal[1], Literal[2]] | |||
| if x1 == 1: | |||
| reveal_type(x1) # N: Revealed type is "Union[__main__.Custom, Literal[1], Literal[2]]" | |||
There was a problem hiding this comment.
Why isn't Literal[2] filtered out in positive case? If x1 is that literal, it certainly won't be equal to 1, so narrowing is safe, isn't it? Do we check for custom __eq__ on the whole union?
Sorry, something went wrong.
There was a problem hiding this comment.
Yeah we note that custom equality exists and if so give up on narrowing currently (iirc, it's been a few months...). I guess it could be fine grained, this PR just notes that Literal[1] wouldn't fail checks.
Sorry, something went wrong.
There was a problem hiding this comment.
Actually, iterating over every component type of a union and applying this logic might make sense, and would also fix this bug. But that sounds more complicated than just this.
Sorry, something went wrong.
|
Do you still have energy to work on this? This negative narrowing looks quite helpful |
Sorry, something went wrong.
|
Yeah, I just typically put things off and then end up not doing them! |
Sorry, something went wrong.
|
This effects our codebase. Would love to have it merged. :^) |
Sorry, something went wrong.
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
Sorry, something went wrong.
Relates to this comment from A5rocks: python#20492 (comment) Fixes python#20532 Fixes python#18029 Fixes python#17229 Fixes python#16465 Closes python#18574
Fixes #16465
Fixes #18029
Fixes #17229
Generally, people cannot replace a literal's equality. So negative narrowing is still safe. (not the case for enum literals, so those don't get this treatment)