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

Make igetattr() idempotent #2208

Merged

Conversation

jacobtylerwalls
Copy link
Member

Type of Changes

Type
βœ“ πŸ› Bug fix

Description

See regression test. The second time an instance's attribute is inferred with the same context (which pylint has legitimate use cases for, see existing test that was improved!), we still need to be able to get a successful inference result. Instead, we were getting Uninferable due to a performance optimization added in 2d7a87b.

This addresses some reports of varying results when running pylint with --jobs.

Closes pylint-dev/pylint#4356
Refs #7

More info

The original inconsistency added in 2d7a87b targeted a report in #7, but we have no source for the original code being tested.

I attempted to guess what the bug report in #7 was. I came up with the following based on #7 (comment), and it lints fine:

class A:
    def __init__(self) -> None:
        self.transaction = self.one() or self.two() or self.three() or self.four() or self.five()
        self.transaction #@

    def one(self):
        self.transaction = trans
        trans = self.transaction

    def two(self):
        self.transaction = trans
        trans = self.transaction

    def three(self):
        self.transaction = trans
        trans = self.transaction

    def four(self):
        self.transaction = trans
        trans = self.transaction

    def five(self):
        self.transaction = trans
        trans = self.transaction

I don't want to commit this file to the tests, though, because it's a wild guess.

This addresses some reports of varying results when running pylint with ``--jobs. The original inconsistency
was due to a performance optimization in 2d7a87b, but
we have no source for the original bug report it targeted.
@jacobtylerwalls jacobtylerwalls added this to the 3.0.0a5 milestone Jun 9, 2023
@jacobtylerwalls jacobtylerwalls added the pylint-tested PRs that don't cause major regressions with pylint label Jun 9, 2023
@codecov
Copy link

codecov bot commented Jun 9, 2023

Codecov Report

Merging #2208 (df30870) into main (4493399) will increase coverage by 0.00%.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #2208   +/-   ##
=======================================
  Coverage   92.68%   92.68%           
=======================================
  Files          94       94           
  Lines       10820    10828    +8     
=======================================
+ Hits        10028    10036    +8     
  Misses        792      792           
Flag Coverage Ξ”
linux 92.44% <ΓΈ> (+<0.01%) ⬆️
pypy 87.64% <ΓΈ> (-0.01%) ⬇️
windows 92.27% <ΓΈ> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Ξ”
astroid/bases.py 87.90% <ΓΈ> (-0.07%) ⬇️

... and 1 file with indirect coverage changes

@Pierre-Sassoulas
Copy link
Member

It seems like the original reason to do that was for performance. Did you check the performance or just the tests suite result when testing in pylint ? (We're probably going to merge anyway as correctness is more important than performance especially if we can't parralelize without fixing this.. but is it really bad ?)

@jacobtylerwalls
Copy link
Member Author

It seems like the original reason to do that was for performance.

Right, but only for the pathological case where there are self-references created like in #7 (comment). So I wouldn't expect much of a performance impact.

Did you check the performance or just the tests suite result when testing in pylint ?

Just the test suite, and then also trying to reproduce the original pathological case, but couldn't. I just now linted a couple packages and there was no significant difference in the total time.

Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for checking !

@jacobtylerwalls jacobtylerwalls merged commit 1fbbf25 into pylint-dev:main Jun 10, 2023
20 checks passed
@jacobtylerwalls
Copy link
Member Author

Thanks for the review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug πŸͺ³ pylint-tested PRs that don't cause major regressions with pylint
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Different results with -j4 and -j8
2 participants