Skip to content

Fix crash while iteraring over a class attribute #7386

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

Merged
DanielNoord 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 all commits
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 @@
Update ``modified_iterating`` checker to fix a crash with ``for`` loops on empty list.

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
16 changes: 14 additions & 2 deletions tests/functional/m/modified_iterating.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
"""Tests for iterating-modified messages"""
# pylint: disable=not-callable,unnecessary-comprehension
# pylint: disable=not-callable,unnecessary-comprehension,too-few-public-methods

import copy

Expand All @@ -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:
"""Regression test for https://github.com/PyCQA/pylint/issues/7380"""

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

@orSolocate orSolocate Aug 31, 2022

Copy link
Copy Markdown
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

@orSolocate orSolocate Aug 31, 2022

Copy link
Copy Markdown
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.

@DanielNoord DanielNoord Aug 31, 2022

Copy link
Copy Markdown
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.

@orSolocate orSolocate Aug 31, 2022

Copy link
Copy Markdown
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?

@DanielNoord DanielNoord Aug 31, 2022

Copy link
Copy Markdown
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.

@orSolocate orSolocate Aug 31, 2022

Copy link
Copy Markdown
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?

@DanielNoord DanielNoord Aug 31, 2022

Copy link
Copy Markdown
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