Issue 1951 Check that generator expressions are safe #2072
Conversation
CHANGELOG.md
Outdated
| @@ -109,6 +109,7 @@ Semantic versioning in our case means: | |||
| - Forbids inconsistent structuring of multiline comprehensions | |||
| - Forbids to use unpythonic getters and setters such as `get_attribute` or `set_attribute` | |||
| - Now `credits`, `license`, and `copyright` builtins are free to shadow | |||
| - Forbids the reassignment of variables that affect the generator expression | |||
There was a problem hiding this comment.
This would be released under 0.16.0, not 0.15.0
| # Correct | ||
| a = 1 | ||
| b = (a * i for i in range(5)) | ||
| c = (sum(b)) |
There was a problem hiding this comment.
| c = (sum(b)) | |
| c = sum(b) |
| generator expression or implement it before reassigning. | ||
|
|
||
| Example: | ||
| # Correct |
There was a problem hiding this comment.
Can you please use more meaningful names here? We try to write our correct examples as close to the real-world code as possible.
| unsafe_function = """ | ||
| first_value = 1 | ||
| expression = (first_value * index for index in range(5)) | ||
| first_value = 2 |
There was a problem hiding this comment.
What about changing index? Let's test that as well.
There was a problem hiding this comment.
I believe that the index cannot be changed as the variable is load within the generator expression. Even though, I'll include a test
| a = 2 | ||
| c = (sum(c)) | ||
|
|
||
| .. versionadded:: 0.15.0 |
| assignments = [ | ||
| sub_node | ||
| for sub_node in ast.walk(node) | ||
| if isinstance(sub_node, ast.Assign) |
There was a problem hiding this comment.
You only check for Assign, what about AugAssign like += 1 or assigning expression like (x := 1)? We need to support them as well.
There was a problem hiding this comment.
Sure, it needs to have AugAssign support, but it'll add too much complexity with the current Visitor.
In the case of the assigning expression, there is a Violation that prevents the usage of (x := 1) , WPS332 Found Walrus operator
| ]) | ||
|
|
||
| if self.affecting_variables: | ||
| self._check_gen_usage(node, assignments) |
There was a problem hiding this comment.
You are missing self.generic_visit(node) call at the bottom. It is required.
|
|
||
| def _get_inside_variables(self, node: ast.GeneratorExp) -> None: | ||
| sub_node = node.elt | ||
| self.inside_variables = [ |
There was a problem hiding this comment.
We already have this logic as a function: https://github.com/wemake-services/wemake-python-styleguide/blob/master/wemake_python_styleguide/logic/walk.py#L56
|
|
||
| return gen_positions | ||
|
|
||
| def _get_id_lineno(self, node: ast.Name): |
| def _get_id_lineno(self, node: ast.Name): | ||
| return (node.id, node.lineno) | ||
|
|
||
| def _check_gen_usage(self, node: ast.AST, assignments: List[ast.Assign]): |
|
Sure, I'll be working on them today, and I hope later today or tomorrow early commit the requested changes. 🤓 |
| a = 1 | ||
| b = (a * i for i in range(5)) | ||
| c = (sum(b)) | ||
| suffix = 'ish' |
|
I uploaded a new version of the UnsafeGeneratorExpressionVisitor that fixes the issues with complexity and style. But I'd like to add a new visitor that adds support for the AugAssign. I'll upload it in less than a day! 👍 |
This sounds like not the best idea. |
|
I figured out a new way to support |
| values = [1, 2, 3, 4, 5] | ||
| exponent = 5 | ||
| power_expr = (value * exponent for value in values) | ||
| exponent += 1 |
There was a problem hiding this comment.
What about changing values here? Like values.append(1)
Should we allow it?
There was a problem hiding this comment.
I think we should not because it affects the expected behavior of the expression. Can I create a new rule request to prevent it? 🤔
There was a problem hiding this comment.
I am not sure that it is "expected" because some people might be getting an error here. Let me give you an example:
>>> values = [1, 2, 3]
>>> listcomp = [a * 2 for a in values]
>>> values.append(0)
>>> values, listcomp
([1, 2, 3, 0], [2, 4, 6])Here's how it works with lists: no unexpected problems.
Now, the same with gen:
>>> values = [1, 2, 3]
>>> gen = (a * 2 for a in values)
>>> values.append(0)
>>> values, list(gen)
([1, 2, 3, 0], [2, 4, 6, 0])Now, we can see that changing values changes how gen works. It is not consistent with list comprehensions. If a persons just refactored list comprehension to be a generator expression with a code like this - there would be an error.
And we can catch it!
I recommend to try to visit ast.Name with usage / reassining context. It might be easier that our current appoach.
There was a problem hiding this comment.
Yes, I think that we should prevent it!
But, with the current visitor, it would be difficult to implement without making it complex because the ast.Name of values have ctx ast.Load in comparisons, functions or creating other list comprehensions and gens that not affect the gen.
>>> values = [1, 2, 3, 4, 5]
>>> gen = (a * 2 for a in values)
>>> if 2 in values:
>>> addition = sum(values)
>>> values, list(gen)
([1, 2, 3], [2, 4, 6]Then we should check that the ast.Attribute append of gen is not being used within assignment and usage. That would need at least 2 new methods visit_Attribute and _check_append but the current visitor is at maximum allowed method capacity.
Also, I believe that a new violation would make the error easier to understand as the current violation only explains the variables that affect a generator expression. The UnsafeAppendUsageViolation would point out the forbidden usage of append within assignment and call for lists that affect a generator expression.
There was a problem hiding this comment.
Will the current implementation work for:
self.x = 1
gen = (self.x * 2 for a in values)
self.x = 2
...?
That would need at least 2 new methods visit_Attribute and _check_append but the current visitor is at maximum allowed method capacity.
Don't worry about it for now, let's concentrate on making this work for all general and edge cases first, then we can refactor the working solution into a simplier version.
There was a problem hiding this comment.
Sure then, I'll be working on append support. 📝
On the other hand, the current implementation doesn't work with self variables. So, I'll update it as well with new tests 🤠
I have made things!
Checklist
CHANGELOG.mdRelated issues
🙏 Please, if you or your company is finding wemake-python-styleguide valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/wemake-python-styleguide. As a thank you, your profile/company logo will be added to our main README which receives hundreds of unique visitors per day.