-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[typing
] Add find_assigned_value
helper func to typing.rs
to retrieve value of a given variable id
#8583
[typing
] Add find_assigned_value
helper func to typing.rs
to retrieve value of a given variable id
#8583
Conversation
ea27432
to
0016306
Compare
|
63a4731
to
1084c11
Compare
@charliermarsh I'm thinking the |
Expr::Tuple(ast::ExprTuple { elts, .. }) | ||
| Expr::List(ast::ExprList { elts, .. }) | ||
| Expr::Set(ast::ExprSet { elts, .. }) => { | ||
if let Some(target) = targets.iter().next() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens here in the case of multi-target assignments?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. It works for
x = y = 0
Added example of that to TRIO115 fixture
Does not work for
(x, y) = [a, b] = (0, 0)
Which will only manage to retrieve the values for x
and y
right now.
Would it be an idea to set up unit tests for this and the other new func in the module? If so, is there a leading example of non plugin unit tests I can have a look at?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it able to detect y
in x = y = 0
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we don't have great infrastructure for this. I'd probably just recommend adding more trio test cases as a means of testing this behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it able to detect
y
inx = y = 0
?
Yeah this one works. But if one of the targets is a collection there are issues. e.g.
a = (x, y) = (0, 0)
(a, b) = [c, d] = [0, 0]
Not entirely sure how to best fix that. Flag it as a multi assign if the top-level targets has a length larger than 1 and add a separate branch for that? Or are there single target scenarios that could be mistaken then?
I think there's one more case to consider: m_c = (m_d, m_e) = (0, 0)
trio.sleep(m_c) # OK
trio.sleep(m_d) # TRIO115
trio.sleep(m_e) # TRIO115 Right now, these all get matched to |
Ho missed this one when I posted #8583 (comment) Added it to TODOs. Gonna look at this again, see if there's a way to be aware of the multi target nature of the assign before drilling down. |
typing
] Add get_assigned_value
helper func to typing.rs
to retrieve value of a given variable id
typing
] Add find_assigned_value
helper func to typing.rs
to retrieve value of a given variable id
Summary
Adds
find_assigned_value
a function which gets the&Expr
assigned to a givenid
if one exists in the semantic model.Open TODOs:
binding.kind.is_unpacked_assignment()
: I am bit confused by this one. The snippet from its documentation does not appear to be counted as an unpacked assignment and the only ones I could find for which that was true were invalid Python like:to get the full value for a? Code currently just returns
None
for these assign typesTest Plan
Used the function in two rules:
TRIO115
PERF101
Expanded both their fixtures for explicit multi target check