Narrow literals in the negative case even with custom equality #18574
Conversation
| 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?
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 ...)?
This comment has been minimized.
This comment has been minimized.
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
| 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 ...)?
| @@ -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?
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.
|
Do you still have energy to work on this? This negative narrowing looks quite helpful |
|
Yeah, I just typically put things off and then end up not doing them! |
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)