Fix sentinel identity loss through generic substitution and inference - #21888
Fix sentinel identity loss through generic substitution and inference#21888edgarrmondragon wants to merge 2 commits into
Conversation
A sentinel's only way to identify itself is via its attached literal value, unlike e.g. enum members whose class already carries identity. That literal was being unconditionally stripped both when a TypeVar was substituted with a sentinel instance (e.g. `dict.get`'s overloaded default parameter) and when inferring the type of a plain variable assignment, collapsing every sentinel down to the same uninformative `sentinel` type. Preserve it in both cases. Fixes python#21866 Follow-up from python#21647 Signed-off-by: Edgar Ramírez Mondragón <edgarrm358@gmail.com>
|
|
||
| assert_type(ALIAS, sentinel) | ||
| # The value still identifies as the same sentinel... | ||
| assert_type(ALIAS, MISSING) |
There was a problem hiding this comment.
I think this is more useful now and still in line with #21647 (comment) and the spec:
https://typing.python.org/en/latest/spec/special-types.html#sentinels
There was a problem hiding this comment.
This makes sense to me, do you think something: sentinel = MISSING should pass or fail?
There was a problem hiding this comment.
I think it should pass. It does seem intuitive that a sentinel instance should be assignable to the sentinel type.
Related:
There was a problem hiding this comment.
Makes sense, could you add one more test case for that? (If there's not one already)
This comment has been minimized.
This comment has been minimized.
cdce8p
left a comment
cdce8p
left a comment
There was a problem hiding this comment.
Thanks for looking into it @edgarrmondragon! The changes make sense and look good to me.
A5rocks
left a comment
A5rocks
left a comment
There was a problem hiding this comment.
The fix makes sense to me.
| return t | ||
| if t.last_known_value is not None and t.last_known_value.is_sentinel_literal(): | ||
| # Sentinel values (PEP 661) have no other way to identify themselves than | ||
| # via their literal, unlike e.g. enum members, so it must be preserved. |
There was a problem hiding this comment.
Huh I'm a bit frightened about storing everything in last_known_value :-)
I assume this was already litigated in an early PR and there's no better cleaner method?
There was a problem hiding this comment.
No, I don't think it was litigated at all. There's perhaps a better way to do all of this for sentinel without playing whack-a-mole with last_known_value (e.g. ff4d71b), but it surely involves a large refactor to handle sentinels in a similar manner to enums and NewType.
|
|
||
| assert_type(ALIAS, sentinel) | ||
| # The value still identifies as the same sentinel... | ||
| assert_type(ALIAS, MISSING) |
There was a problem hiding this comment.
This makes sense to me, do you think something: sentinel = MISSING should pass or fail?
|
|
||
| [builtins fixtures/sentinel.pyi] | ||
|
|
||
| [case testSentinelReassignmentIsNotTypeAlias] |
There was a problem hiding this comment.
| [case testSentinelReassignmentIsTypeAlias] |
It sounds to me like saying reassignment isn't a type alias was dropped from the spec. Maybe I'm misreading!
There was a problem hiding this comment.
Hmm. I think the current name is technically correct but maybe a bit misleading.
Reassignment creates an instance of the same type, but doesn't create a valid type, i.e. the new variable is not a type alias.
Signed-off-by: Edgar Ramírez Mondragón <edgarrm358@gmail.com>
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
3 participants
A sentinel's only way to identify itself is via its attached literal value, unlike e.g. enum members whose class already carries identity. That literal was being unconditionally stripped both when a
TypeVarwas substituted with a sentinel instance (e.g.dict.get's overloaded default parameter) and when inferring the type of a plain variable assignment, collapsing every sentinel down to the same uninformativesentineltype. Preserve it in both cases.Fixes #21866
Follow-up from #21647