Explicit types for bindables ("Tighten Types" continued) #2238
Conversation
|
Let's make sure to add "Fixes #2222" to the commit message when squash-merging this. |
|
I hope I can review #1748 on Monday or over the weekend. It's a big change and has been open for a long time so let's land it first. |
Conflicts: mypy/binder.py mypy/checker.py mypy/checkexpr.py mypy/nodes.py
There was a problem hiding this comment.
This looks decent except for one thing I don't quite get in a quick scan. @rwbarton what's your opinion of this PR?
| if not var.type: | ||
| if var.type: | ||
| return var.type | ||
| else: |
There was a problem hiding this comment.
Just double-checking that what you changed here isn't an accidental merge conflict? You've done more here than just revers the sense of the check and swap the then/else clauses.
There was a problem hiding this comment.
This part is related to #2222 - binder.get() does not accept Var; val is always None.
There was a problem hiding this comment.
BTW, I would like to extract literal-handling to a couple of functions, and avoid putting them in the Expression classes. I already have the branch ready. Will you consider it? It does not solve anything, but I think it's nice to have this information in a single place, instead of scattered around.
There was a problem hiding this comment.
It would be worth calling out this "change" explicitly in the commit message, since it does not obviously look like a mere refactoring.
mypy/nodes.py
Outdated
| self.literal_hash = ((cast(Any, 'Comparison'),) + tuple(operators) + | ||
| tuple(o.literal_hash for o in operands)) | ||
| self.literal_hash = (('Comparison',) + tuple(operators) + | ||
| tuple(o.literal_hash for o in operands)) # type: ignore |
There was a problem hiding this comment.
Explain the type ignores here and below?
There was a problem hiding this comment.
This was supposed to be another way to spell the same thing, but it's not since Tuple[Any, ...] is still informative. I should revert this.
|
I've got no time this week, but hopefully eventually one of us will get to
it. Honestly if you want to help with high priority stuff, looking at the
tail end of #1748 would be more helpful.
|
|
I still intend to get around to looking at this soon. |
|
I'm sorry, I've lost track of this. Are you waiting for me or am I waiting for you? |
|
I am waiting |
mypy/binder.py
Outdated
| @@ -189,19 +197,20 @@ def pop_frame(self, can_skip: bool, fall_through: int) -> Frame: | |||
|
|
|||
| return result | |||
|
|
|||
| def get_declaration(self, expr: Node) -> Type: | |||
| def get_declaration(self, expr: BindableExpression) -> Type: | |||
| assert not isinstance(expr, SymbolTableNode) | |||
There was a problem hiding this comment.
This assert can't trigger given the definition of BindableExpression right? Or am I misunderstanding?
There was a problem hiding this comment.
It's a leftover from an old check,
if isinstance(node, (RefExpr, SymbolTableNode)) and isinstance(node.node, Var):
I should remove it.
|
This looks all right to me! I'll wait maybe a day for @rwbarton to speak up. |
|
|
||
| BindableTypes = (IndexExpr, MemberExpr, NameExpr) | ||
| BindableExpression = Union[IndexExpr, MemberExpr, NameExpr] | ||
|
|
There was a problem hiding this comment.
I don't see the point of introducing this BindableExpression. It seems to duplicate information with which Expressions have (non-None) literals. It should be fine to just use Expression everywhere.
There was a problem hiding this comment.
Lists of literals have non-None literals.
There was a problem hiding this comment.
Here's the branch with literal() and literal_hash() functions:
https://github.com/elazarg/mypy/blob/literal_hash/mypy/nodes.py#L2351-L2421
mypy/binder.py
Outdated
| @@ -103,19 +109,21 @@ def _get(self, key: Key, index: int=-1) -> Type: | |||
| return self.frames[i][key] | |||
| return None | |||
|
|
|||
| def push(self, node: Node, typ: Type) -> None: | |||
| if not node.literal: | |||
| def put_if_bindable(self, expr: Expression, typ: Type) -> None: | |||
There was a problem hiding this comment.
I like the change to put, but _if_bindable doesn't add any information (there's no contrasting put_even_if_not_bindable) and is pretty long.
Basically, put is just the way for the rest of the type checker to pass information into the binder, and I think of it as the binder's business whether it bothers to record a piece of information it is given or not.
There was a problem hiding this comment.
I think it adds no information only if you know exactly how the binder works. When I read this code I had no idea that some expressions can be simply ignored. I do not like long names, but shorter names feels misleading.
There was a problem hiding this comment.
Of course it is the binder's business, but the reader should know that it is possible.
There was a problem hiding this comment.
A docstring (or general documentation on the binder) seems like a better vehicle for that.
mypy/binder.py
Outdated
| if not node.literal: | ||
| def put_if_bindable(self, expr: Expression, typ: Type) -> None: | ||
| if not isinstance(expr, BindableTypes): | ||
| return |
There was a problem hiding this comment.
I am not really sure about the need for this.
mypy/binder.py
Outdated
| type: Type, | ||
| declared_type: Type, | ||
| restrict_any: bool = False) -> None: | ||
| def assign_type_if_bindable(self, expr: Expression, |
There was a problem hiding this comment.
Again, _if_bindable seems extraneous.
mypy/binder.py
Outdated
| declared_type: Type, | ||
| restrict_any: bool = False) -> None: | ||
| if not isinstance(expr, BindableTypes): | ||
| return None |
There was a problem hiding this comment.
This check doesn't seem necessary.
| if not var.type: | ||
| if var.type: | ||
| return var.type | ||
| else: |
There was a problem hiding this comment.
It would be worth calling out this "change" explicitly in the commit message, since it does not obviously look like a mere refactoring.
|
I kind of feel like the addition of If you were to move |
|
I have moved You say "this information is already contained in nodes.py" - I believe you that the information is technically available, but I still can't say where and how, precisely. (Of course it could be just me). Only during the work on this PR did I understand what does the binder do, and I wanted to make this information explicit (using "if_bindable" and "put"). I agree that the decision what is bindable is not inherent to the types themselves and could be changed transparently, but there should be such explicit concept, with a dedicated filtering function as documentation. The current policy is not just about "having a literal value", so |
|
@rwbarton and @elazarg, what is needed to get agreement on this PR to be merged? Personally I was okay with it before, but it seems @rwbarton wants some changes. Maybe @elazarg should just make the requested changes? Personally I agree with @rwbarton that the '_if_bindable' suffix you added to some method names is unneeded -- maybe that's the last thing standing between approval? There's also a merge conflict when I try to apply this (never mind what GitHub says). |
|
@gvanrossum I reverted the |
|
@rwbarton, will it help if I'll add the Besides, I really like these functions. |
Conflicts: mypy/checker.py mypy/nodes.py
I'd like to merge this if it still works, but am a bit worried about it being stale, so I'm triggering the tests.
|
@elazarg Can you rebase and address the now-failing tests? We're not waiting for Reid. |
|
Sure. |
| def assign_type(self, expr: Expression, | ||
| type: Type, | ||
| declared_type: Type, | ||
| restrict_any: bool = False) -> None: | ||
| # TODO: replace with isinstance(expr, BindableTypes) |
There was a problem hiding this comment.
Can you file an issue for that regression? What does reveal_type(BindableTypes) show at this point?
There was a problem hiding this comment.
Filed: #3068. It will show Expression.
|
Thanks, I found nothing suspicious running this against our internal codebase. Hopefully @JukkaL's pending PRs aren't too much affected. |
Currently the only IndexExpr, MemberExpr and NameExpr are bound through ConditionalTypeBinder. This information is made explicit, although it's still kept inside binder, since I think it's not unlikely to change. (I was wrong thinking this is about lvalues. It is not really related to syntax classes).
The key of Frame is an object instead of Any - I'd vote for str or int - and so is literal_hash, which is moved from Node to Expression. (I think
literal_hashandliteralshould be standalone functions instead of fields, since it's an implementation detail of the binder and possibly other mappings, and most of the time it does not interest the reader of the syntax classes).While at it,
push()is changed toput_if_bindable(). IMHO it is much more precise about what's actually promised. Similarlyassign_type()becomesassign_type_if_bindable().