PyCQA / flake8-bugbear

A plugin for Flake8 finding likely bugs and design problems in your program. Contains warnings that don't belong in pyflakes and pycodestyle.
MIT License
1.07k stars 107 forks source link

Possible B020 false positive with instance attribute #248

Open MrkGrgsn opened 2 years ago

MrkGrgsn commented 2 years ago

This code:

class MyModel:
    value: int

class A:
    model_instance = MyModel()
    test_suite = [1, 2, 3]

    def method(self):
        for self.model_instance.value in self.test_suite:
            print(self.model_instance.value)

results in a B020, but it looks like a false positive to me since self.test_suite is not one of the iterable values. Admittedly it is a slightly unusual and ambiguous case since self is being modified and the plugin can't know what changes setting self.model_instance is making to the object and the value of self.test_suite.

The actual use case is a test where a Django model object is created once in the test set up and, in each iteration, the loop modifies model attributes, passes the model to a function, and tests the result.

cooperlees commented 2 years ago

I agree this seems a false positive, but it's very creative class instance iteration here. It's also a class variable, so all instances would modify the same value. Dangerous stuff.

To potentially fix this noise will need some thought. I'm not even sure if we can. Let's see what others think.

MrkGrgsn commented 2 years ago

This one's not my code! It's definitely creative and I had to look twice because I'd not seen such a pattern before.

Note that the B020 is raised if it's an instance variable also, in fact it is in my case but I was trying to make the example simpler.

The real case is like this.

class MyTestCase(TestCase):
    test_suite = [1, 2, 3]

    def setUp(self):
        self.model_instance = MyModel.objects.get(...)

    def test_method(self):
        for self.model_instance.value in self.test_suite:
            print(self.model_instance.value)

I don't see a particularly compelling reason for it to be written this way. An alternative, as an example, would be for the test_method to instantiate a new MyModel each iteration without writing to the DB. But even if it should be written a different way it still concerns me that B020 is an inaccurate description of the style issue.

spaceone commented 1 year ago

Introducing another code for this would be helpful (or just ignoring) IMHO.