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.

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 ClassDef.igetattr on multiple attributes (not-an-iterable false-positive) #1864

Closed
wants to merge 1 commit into from

Conversation

Rogdham
Copy link

@Rogdham Rogdham commented Nov 6, 2022

Steps

  • For new features or bug fixes, add a ChangeLog entry describing what your PR does.
  • 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 fix

Related Issue

Closes #1015

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

@belm0
Copy link
Contributor

belm0 commented Nov 6, 2022

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.

@Rogdham
Copy link
Author

Rogdham commented Nov 6, 2022

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

@coveralls
Copy link

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

@jacobtylerwalls
Copy link
Member

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

@Rogdham
Copy link
Author

Rogdham commented Nov 6, 2022

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

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!

@jacobtylerwalls
Copy link
Member

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.

@nelfin
Copy link
Contributor

nelfin commented Nov 6, 2022

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.

@cdce8p cdce8p marked this pull request as draft November 13, 2022 10:54
@cdce8p cdce8p added the High effort 🏋 Difficult solution or problem to solve label Nov 13, 2022
@jacobtylerwalls
Copy link
Member

Superseded in #1173

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug 🪳 High effort 🏋 Difficult solution or problem to solve
Projects
None yet
Development

Successfully merging this pull request may close these issues.

false not-an-iterable for class method with @overload (2.5.7 regression)
7 participants