astroid icon indicating copy to clipboard operation
astroid copied to clipboard

Fix `ClassDef.igetattr` on multiple attributes (`not-an-iterable` false-positive)

Open Rogdham opened this issue 2 years ago • 7 comments

Steps

  • [ ] For new features or bug fixes, add a ChangeLog entry describing what your PR does.
  • [x] Write a good description on what the PR does.

Description

This is an attempt at fixing #1015; however I really don't know astroid's code base so this is more like a shot in the dark, which needs a careful review!

In a few words:

class A:
    def foo(self): ...
    def foo(self):
        yield

for _ in A().foo():
    pass

Only the first implementation returning ... is considered, resulting in pylint complaining:

E1133: Non-iterable value A().foo() is used in an iterating context (not-an-iterable)

This is especially annoying when we define @overload variants for typing purposes.

Type of Changes

Type
:bug: Bug fix

Related Issue

Closes #1015

Ping @nelfin @belm0 who are probably interested in this.

Rogdham avatar Nov 06 '22 10:11 Rogdham

thank you for working on it

I think it will need unit test coverage. It's good to write the test and verify that it fails before applying the fix.

belm0 avatar Nov 06 '22 10:11 belm0

Thank you for the feedback, I have added a test and confirmed that it failed before the fix.

Rogdham avatar Nov 06 '22 11:11 Rogdham

Pull Request Test Coverage Report for Build 3404098486

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

Totals Coverage Status
Change from base Build 3393053917: 0.0007%
Covered Lines: 9865
Relevant Lines: 10691

💛 - Coveralls

coveralls avatar Nov 06 '22 12:11 coveralls

Thanks @Rogdham, did you give any thought to the comment on the previous attempt?

jacobtylerwalls avatar Nov 06 '22 12:11 jacobtylerwalls

Dammit I completely missed #1173 for some reason, this would have saved me some headache :grimacing:

Since their fix looks the same to me, should I just close this PR? They seems to know what they are talking about way more than I do!

Rogdham avatar Nov 06 '22 13:11 Rogdham

I'd see if the author responds soon, but since it's been over a year, feel free to pick up where they left off later this month.

jacobtylerwalls avatar Nov 06 '22 14:11 jacobtylerwalls

I never did figure out how to address those comments within the scope of that change and then I spent some time away from contributing to open source for a while. Feel free to cherry-pick those tests if you think they will help. If I remember correctly and understand this change, the comment that Klass.foo should only be inferred as Generator would necessitate a change here as len(attrs) should be 1 in that case, not 2.

nelfin avatar Nov 06 '22 17:11 nelfin

Superseded in #1173

jacobtylerwalls avatar May 04 '24 19:05 jacobtylerwalls