Fix crash while iteraring over a class attribute

[DanielNoord]

• Add yourself to CONTRIBUTORS if you are a new contributor.
• Add a ChangeLog entry describing what your PR does.
• If it's a new feature, or an important bug fix, add a What's New entry in
  doc/whatsnew/<current release.rst>.
• Write a good description on what the PR does.

Type of Changes

	Type
✓	🐛 Bug fix

Description

Closes #7380

@orSolocate I haven't created an issue for this but:

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

   def my_other_method(self):
        """This should raise as we are appending."""
        for var in self.attribute:
            self.attribute.append(var)  # [modified-iterating-list]

Currently doesn't raise like it should.

This would require quite a bit of refactoring in the current checker as it isn't really suited for checking class attributes. I'm not sure and there is no obligation, but perhaps you find this interesting and want to look into it?

[coveralls]

Pull Request Test Coverage Report for Build 2966111299

• 4 of 4 (100.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.0008%) to 95.318%

---

Totals
Change from base Build 2962079951:	0.0008%
Covered Lines:	16939
Relevant Lines:	17771

---

💛 - Coveralls

[orSolocate]

@DanielNoord hey man consider these little improvements for this little addition to my checker.
Are you sure an Attribute will always be something to check here and not a false-positive? I trust your Python knowledge here.

[orSolocate]

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

[DanielNoord]

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 😄

[orSolocate]

Suggested change

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

[orSolocate]

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]

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]

@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]

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]

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]

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.

[Pierre-Sassoulas]

LGTM, let's merge once the discussion with @orSolocate is resolved :)

[orSolocate]

@Pierre-Sassoulas you guys can merge it. You can open a new issue to add support for class attributes with my checker and assign me :)

[github-actions]

🤖 According to the primer, this change has no effect on the checked open source code. 🤖🎉

This comment was generated for commit 339abe2