Fix bug in assignments #68
Conversation
| 'x -> ()']) | ||
|
|
||
| def test_call_assignment(self): | ||
| code = "NameError().name = 't'" |
There was a problem hiding this comment.
Here the ast.Call instance will never be analyzed because it's not part of the supported nodes for assignments.
There was a problem hiding this comment.
Yeah, it's not bound to any identifier... We could create temporary identifier for unbound expression that get assigned, but let's keep that for another patch.
There was a problem hiding this comment.
But it won't be visited at all, so the ast.Call instance uses won't be part of the chains at all, even if we could argue that it's still a user of NameError, the same manner NameError won't be added to the uses of the builtins NameError symbol. This is why I think we should unconditionally visit this part.
beniget/beniget.py
Outdated
| elif isinstance(elt, (ast.Subscript, ast.Attribute)): | ||
| self.visit(elt) |
There was a problem hiding this comment.
We could remove these isinstance checks and simply call visit() for all other node types?
There was a problem hiding this comment.
I like the conservative checks we are currently doing. That way if another case pop ups we don't have a default (and maybe unexpected) behavior.
There was a problem hiding this comment.
See my other comment about the node being completely ignored if we don’t include a catch all case
|
@paugier , maybe you have a moment to look at the pending PRs ? Thanks a lot, |
beniget/beniget.py
Outdated
| elif isinstance(elt, (ast.Subscript, ast.Attribute)): | ||
| self.visit(elt) |
There was a problem hiding this comment.
I like the conservative checks we are currently doing. That way if another case pop ups we don't have a default (and maybe unexpected) behavior.
beniget/beniget.py
Outdated
| self.visit(elt) | ||
| elif isinstance(elt, ast.Starred) and isinstance(elt.value, ast.Name): |
There was a problem hiding this comment.
group that with if isinstance(elt, ast.Name): above as it's the same body?
There was a problem hiding this comment.
It's not exactly the same code, though.
tests/test_chains.py
Outdated
| break | ||
| ''' | ||
| self.checkChains(code, ['curr -> (curr -> (), curr -> (Call -> ()))', | ||
| 'parts -> (Starred -> (), parts -> (), parts -> ())']*2) |
There was a problem hiding this comment.
I'm a bit puzzled by the Starred here.
There was a problem hiding this comment.
Can you develop ?
There was a problem hiding this comment.
Why is the Starred in the def-use chain? It's not a user, right?
There was a problem hiding this comment.
OK, I dive a bit and I now understand the situation a bit better. When we use * to expand a container as part of an argument call, the Starred is clearly a user. And it' has a Load context. When it's used to unpack a tuple, it has a Store context and it's just used to specify the nature of the operation. It is debatable whether that's a use or not... I'd rather see that as a property of the def, but we don't have that concept. I don't see it as a new Def either. Let me submit a new PR for that particular behavior
| 'x -> ()']) | ||
|
|
||
| def test_call_assignment(self): | ||
| code = "NameError().name = 't'" |
There was a problem hiding this comment.
Yeah, it's not bound to any identifier... We could create temporary identifier for unbound expression that get assigned, but let's keep that for another patch.
|
Can you rebase that one on the |
Fixes #66