Skip to content
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

Merged
merged 19 commits into from
Dec 13, 2023

Conversation

qdegraaf
Copy link
Contributor

@qdegraaf qdegraaf commented Nov 9, 2023

Summary

Adds find_assigned_value a function which gets the &Expr assigned to a given id if one exists in the semantic model.

Open TODOs:

  • Handle 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:
x, y = 1 
  • How to handle AugAssign. Can we combine statements like:
(a, b) = [(1, 2, 3), (4,)]
a += (6, 7)

to get the full value for a? Code currently just returns None for these assign types

  • Multi target assigns
m_c = (m_d, m_e) = (0, 0)
trio.sleep(m_c)  # OK
trio.sleep(m_d)  # TRIO115
trio.sleep(m_e)  # TRIO115

Test Plan

Used the function in two rules:

  • TRIO115
  • PERF101

Expanded both their fixtures for explicit multi target check

Copy link
Contributor

github-actions bot commented Nov 9, 2023

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@qdegraaf qdegraaf marked this pull request as ready for review November 13, 2023 12:43
@qdegraaf
Copy link
Contributor Author

@charliermarsh I'm thinking the binding.kind.is_unpacked_assignment() can be left as a TODO for later when it is necessary? Let me know if that is acceptable. I think it would be good to review current set-up and logic first regardless, before expanding further.

Expr::Tuple(ast::ExprTuple { elts, .. })
| Expr::List(ast::ExprList { elts, .. })
| Expr::Set(ast::ExprSet { elts, .. }) => {
if let Some(target) = targets.iter().next() {
Copy link
Member

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?

Copy link
Contributor Author

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?

Copy link
Member

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?

Copy link
Member

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.

Copy link
Contributor Author

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?

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?

@charliermarsh
Copy link
Member

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 (0, 0).

@qdegraaf
Copy link
Contributor Author

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 (0, 0).

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.

@qdegraaf qdegraaf changed the title [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 Nov 24, 2023
@qdegraaf qdegraaf marked this pull request as draft November 30, 2023 17:53
@charliermarsh charliermarsh marked this pull request as ready for review December 13, 2023 00:19
@charliermarsh charliermarsh enabled auto-merge (squash) December 13, 2023 00:19
@charliermarsh charliermarsh merged commit 8314c8b into astral-sh:main Dec 13, 2023
16 checks passed
@qdegraaf qdegraaf deleted the feat/refactorbindingcheck branch January 2, 2024 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants