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

Make used-before-assignment consider classes in method annotation and defaults #5184

Merged
merged 11 commits into from
Oct 23, 2021
Merged
5 changes: 5 additions & 0 deletions ChangeLog
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,11 @@ Release date: TBA

Closes #4031

* ``used-before-assignment`` now correctly considers references to classes as type annotation
or default values in first-level methods

Closes #3771

* Fix bug with importing namespace packages with relative imports

Closes #2967 and #5131
Expand Down
5 changes: 5 additions & 0 deletions doc/whatsnew/2.12.rst
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,11 @@ Other Changes

Closes #4031

* ``used-before-assignment`` now correctly considers references to classes as type annotation
or default values in first-level methods

Closes #3771

* ``mising-param-doc`` now correctly parses asterisks for variable length and
keyword parameters

Expand Down
24 changes: 24 additions & 0 deletions pylint/checkers/variables.py
Original file line number Diff line number Diff line change
Expand Up @@ -1195,6 +1195,11 @@ def visit_name(self, node: nodes.Name) -> None:
)
elif self._is_only_type_assignment(node, defstmt):
self.add_message("undefined-variable", node=node, args=node.name)
elif self._is_first_level_self_reference(node, defstmt):
self.add_message(
"used-before-assignment", node=node, args=node.name
)
break
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This for loop spanning 200 lines asks for a refactor. Maybe you have an idea on how we could simplify this ? I'm not saying we should refactor it now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's awful. I don't even understand half of what it is doing and variable names such as maybee0601 really don't help.
One of the problems is that it is doing something with consuming variables in different scopes which I don't really understand. We can probably investigate this at some point.

I'm just going through the open issues for undefined-variable as it seemed like a good addition to 2.12 to improve that checker. For now I tend to add new single-purpose methods and elifs, that should help refactoring later.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's awful. I

Glad to see I'm not the only one 😄 Yes the consuming + continue + break makes the refactor quite hard. Also it's really hard to understand what's happening, good job fixing issues despite that 🎉

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The complete visit_name function has just gotten too complicated. Not sure we'll ever be able to fix it though. It's really easy to break things if something goes wrong during the refactor. Lot's of testing needed before then.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is also because the way consumption works is quite difficult to understand. It took me a while to understand how the actual looping over scopes works. If we were to improve that (or it explanation) I think we would already be in a much better place.


current_consumer.mark_as_consumed(node.name, found_nodes)
# check it's not a loop variable used outside the loop
Expand Down Expand Up @@ -1569,6 +1574,25 @@ def _is_only_type_assignment(node: nodes.Name, defstmt: nodes.Statement) -> bool
return False
return True

@staticmethod
def _is_first_level_self_reference(
node: nodes.Name, defstmt: nodes.Statement
) -> bool:
"""Check if a first level method's annotation or default values
refers to its own class. See tests/functional/u/use/used_before_assignement.py
for additional examples.
"""
if isinstance(defstmt, nodes.ClassDef) and node.frame().parent == defstmt:
# Check if used as type annotation
DanielNoord marked this conversation as resolved.
Show resolved Hide resolved
if isinstance(node.parent, nodes.Arguments):
return True
cdce8p marked this conversation as resolved.
Show resolved Hide resolved
# Check if used as default value by calling the class
if isinstance(node.parent, nodes.Call) and isinstance(
node.parent.parent, nodes.Arguments
):
return True
return False

def _ignore_class_scope(self, node):
"""
Return True if the node is in a local class scope, as an assignment.
Expand Down
20 changes: 19 additions & 1 deletion tests/functional/u/use/used_before_assignement.py
Original file line number Diff line number Diff line change
@@ -1,8 +1,26 @@
"""pylint doesn't see the NameError in this module"""
#pylint: disable=consider-using-f-string
#pylint: disable=consider-using-f-string, missing-function-docstring
__revision__ = None

MSG = "hello %s" % MSG # [used-before-assignment]

MSG2 = ("hello %s" %
MSG2) # [used-before-assignment]


class MyClass:
"""Type annotation or default values for first level methods can't refer to their own class"""
def incorrect_method(self, other: MyClass) -> bool: # [used-before-assignment]
return self == other
cdce8p marked this conversation as resolved.
Show resolved Hide resolved

def second_incorrect_method(self, other = MyClass()) -> bool: # [used-before-assignment]
return self == other

def correct_method(self, other: "MyClass") -> bool:
return self == other

def second_correct_method(self) -> bool:
def inner_method(self, other: MyClass) -> bool:
return self == other

return inner_method(self, MyClass())
6 changes: 4 additions & 2 deletions tests/functional/u/use/used_before_assignement.txt
Original file line number Diff line number Diff line change
@@ -1,2 +1,4 @@
used-before-assignment:5:19::Using variable 'MSG' before assignment
used-before-assignment:8:8::Using variable 'MSG2' before assignment
used-before-assignment:5:19::Using variable 'MSG' before assignment:HIGH
used-before-assignment:8:8::Using variable 'MSG2' before assignment:HIGH
used-before-assignment:13:38:MyClass.incorrect_method:Using variable 'MyClass' before assignment:HIGH
used-before-assignment:16:46:MyClass.second_incorrect_method:Using variable 'MyClass' before assignment:HIGH