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.

Sign up for GitHub

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

Fix crash while iteraring over a class attribute #7386

Merged
merged 3 commits into from
Aug 31, 2022
Merged

Fix crash while iteraring over a class attribute #7386

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5fe8a2c
Fix crash while iteraring over a class attribute
DanielNoord Aug 31, 2022
2b3e733
Update doc/whatsnew/fragments/7380.bugfix
DanielNoord Aug 31, 2022
339abe2
Review
DanielNoord Aug 31, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions doc/whatsnew/fragments/7380.bugfix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Fixed a crash with ``for`` loops that iterate over class attributes.

Closes #7380
8 changes: 6 additions & 2 deletions pylint/checkers/modified_iterating_checker.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ def _modified_iterating_check(
msg_id = "modified-iterating-dict"
elif isinstance(inferred, nodes.Set):
msg_id = "modified-iterating-set"
elif not isinstance(iter_obj, nodes.Name):
elif not isinstance(iter_obj, (nodes.Name, nodes.Attribute)):
pass
elif self._modified_iterating_list_cond(node, iter_obj):
msg_id = "modified-iterating-list"
Expand All @@ -90,10 +90,14 @@ def _modified_iterating_check(
elif self._modified_iterating_set_cond(node, iter_obj):
msg_id = "modified-iterating-set"
if msg_id:
if isinstance(iter_obj, nodes.Attribute):
obj_name = iter_obj.attrname
else:
obj_name = iter_obj.name
self.add_message(
msg_id,
node=node,
args=(iter_obj.name,),
args=(obj_name,),
confidence=interfaces.INFERENCE,
)

Expand Down
14 changes: 13 additions & 1 deletion tests/functional/m/modified_iterating.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
i = 1
for item in my_dict:
item_list[0] = i # for coverage, see reference at /pull/5628#discussion_r792181642
my_dict[i] = 1 # [modified-iterating-dict]
my_dict[i] = 1 # [modified-iterating-dict]
i += 1

i = 1
Expand Down Expand Up @@ -93,3 +93,15 @@ def update_existing_key():
for key in my_dict:
new_key = key.lower()
my_dict[new_key] = 1 # [modified-iterating-dict]


class MyClass: # pylint: disable=too-few-public-methods
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the only class in the test file. you can add the disable flag at the top of the file

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is personal preference I think. I like disabling at the occurrence as it allows more freedom in checking for some message further down the file.

But I'll change it here 😄

"""Regression test for https://github.com/PyCQA/pylint/issues/7380"""

def __init__(self) -> None:
self.attribute = [1, 2, 3]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
self.attribute = [1, 2, 3]
self.iterated_attributes = [1, 2, 3]


def my_method(self):
"""This should raise as we are deleting."""
for var in self.attribute:
del var # [modified-iterating-list]
Comment on lines +104 to +107
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
def my_method(self):
"""This should raise as we are deleting."""
for var in self.attribute:
del var # [modified-iterating-list]
def modifying_list_(self):
# should be fine to modify a copy
for var in self.iterated_attributes.copy():
del var # [modified-iterating-list]
# should raise a message when modifying
for var in self.iterated_attributes:
del var # [modified-iterating-list]

Also check the false positive case. Fix the .txt file accordingly.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think we should hold off on actually fixing any false negatives and positives here. As I said, the current code really doesn't work well with class attributes as we depend on functions like .append being attributes of the iterable object. This complicates things massively when actually looking for attributes.

Copy link
Contributor

Choose a reason for hiding this comment

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

@DanielNoord yeah i re-read it and understood that the case which is not working is the class case.
I don't fully understand the append example, how can you put list.append in a for loop?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

for x in a_list:
    a_list.append(x)

The issue here is that append is an Attribute node on a_list just as attribute is an Attribute node in the following example:

class MyClass:
    def __init__():
        self.attribute = [1,2,3]

    def method(self):
        for x in self.attribute:
            del x

Astroid only recognises that they are nodes that come after a period, not that they are attributes in the traditional sense of the word.
This complicates the logic we're currently using in the checker. I tried to fix it, but it turned out to be a pretty large refactor which probably should be taken in multiple steps for ease of reviewing.
To make sure we no longer crash I wanted to push this fix at least.

Copy link
Contributor

Choose a reason for hiding this comment

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

You mean like when iterating through a dictionary d : iterating on d.keys is an Attribute which is actually a fix the our error of iterating through d? something like that? so your current solution will throw false-postive 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.

I think you'll understand what you mean when you add the example I gave in the opening post to the functional tests. If you run pytest then you'll see that some of the isinstance(node, nodes.Attribute) test you are using to pick up d.append will now also pick up MyClass.attribute. This causes some issues and methods such as _modified_iterating_list_cond will need to be changed to allow checking both type of nodes.

1 change: 1 addition & 0 deletions tests/functional/m/modified_iterating.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,3 +13,4 @@ modified-iterating-list:64:4:64:23::Iterated list 'item_list' is being modified
modified-iterating-list:67:12:67:31::Iterated list 'item_list' is being modified inside for loop body, consider iterating through a copy of it instead.:INFERENCE
modified-iterating-list:69:16:69:35::Iterated list 'item_list' is being modified inside for loop body, consider iterating through a copy of it instead.:INFERENCE
modified-iterating-dict:95:8:95:28:update_existing_key:Iterated dict 'my_dict' is being modified inside for loop body, iterate through a copy of it instead.:INFERENCE
modified-iterating-list:107:12:107:19:MyClass.my_method:Iterated list 'attribute' is being modified inside for loop body, consider iterating through a copy of it instead.:INFERENCE