Declare literal_hash as Optional[Key]#3658
Conversation
mypy/binder.py
Outdated
|
|
||
|
|
||
| class Frame(Dict[Key, Type]): | ||
| class Frame(Dict[Optional[Key], Type]): |
There was a problem hiding this comment.
Does the base class really have to use Optional[Key] as its key type? Does None really occur as a valid key?
There was a problem hiding this comment.
You can't just remove the Optional here. I'm not sure that the value actually occurs (it does not make much sense to me), but the type is used in 5 different places*,
I tried to avoid adding if key is not None to keep the change minimal.
(*) lines 109, 116, 151, 270, 271
There was a problem hiding this comment.
I think the actual semantics is that keys should not be None. Therefore it should not be Optional here. Also, saving few lines of code does not justify adding potentially misleading hints. Strict optional typically touches subtle things, thus everything should be written as "right" as possible.
mypy/binder.py
Outdated
|
|
||
|
|
||
| class Frame(Dict[Key, Type]): | ||
| class Frame(Dict[Optional[Key], Type]): |
There was a problem hiding this comment.
I think the actual semantics is that keys should not be None. Therefore it should not be Optional here. Also, saving few lines of code does not justify adding potentially misleading hints. Strict optional typically touches subtle things, thus everything should be written as "right" as possible.
mypy/binder.py
Outdated
|
|
||
|
|
||
| class DeclarationsFrame(Dict[Key, Optional[Type]]): | ||
| class DeclarationsFrame(Dict[Optional[Key], Optional[Type]]): |
There was a problem hiding this comment.
This Optional is also not needed IMO.
mypy/binder.py
Outdated
| self.declarations[key] = get_declaration(expr) | ||
| self._add_dependencies(key) | ||
| if key is not None: | ||
| self._add_dependencies(key) |
There was a problem hiding this comment.
This change actually looks reasonable.
There was a problem hiding this comment.
I cannot comment there, but you can just add similar things for return self._get(expr.literal_hash) etc., or assert expr.literal_hash is not None where appropriate.
|
Maybe it's better to simply initialize the field to |
ilevkivskyi
left a comment
ilevkivskyi
left a comment
There was a problem hiding this comment.
Looks good now, just one more small suggestion.
mypy/binder.py
Outdated
| if not expr.literal: | ||
| return | ||
| key = expr.literal_hash | ||
| assert key is not None |
There was a problem hiding this comment.
I would prefer to see a descriptive assertion message here, maybe "Internal error: binder tried to put non-literal", ditto for get and cleanse.
3 participants
literal_hash = None # type: Keyis an actual initialization and the None value is accessed, so it should be declared that way. Strict-optional mode does not catch it since it looks like a declaration.(Surfaced in #3071 here)