Skip to content

Fix E1102 / not-callable false positive for property that returns a lambda function conditionally #6068

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

Merged

Conversation

tushar-deepsource
Copy link
Contributor

Type of Changes

Type
🐛 Bug fix

Description

Update E1102 to only be raised when no inferred value is callable.

Resolves #5931

@coveralls
Copy link

coveralls commented Mar 31, 2022

Pull Request Test Coverage Report for Build 2081161263

  • 6 of 6 (100.0%) changed or added relevant lines in 1 file are covered.
  • 16 unchanged lines in 5 files lost coverage.
  • Overall coverage decreased (-0.03%) to 94.273%

Files with Coverage Reduction New Missed Lines %
pylint/checkers/base/docstring_checker.py 1 96.67%
pylint/checkers/typecheck.py 1 95.2%
pylint/config/arguments_manager.py 1 97.5%
pylint/config/config_initialization.py 3 93.18%
pylint/config/init.py 10 70.77%
Totals Coverage Status
Change from base Build 2079086702: -0.03%
Covered Lines: 15505
Relevant Lines: 16447

💛 - Coveralls

@Pierre-Sassoulas Pierre-Sassoulas added Control flow Requires control flow understanding False Positive 🦟 A message is emitted but nothing is wrong with the code labels Mar 31, 2022
@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.13.5 milestone Mar 31, 2022
@srijan-paul
Copy link

zamn :^)

@Pierre-Sassoulas Pierre-Sassoulas changed the title Fix E1102 false positive Fix E1102 / not-callable false positive for property that return a lambda function conditionnally Apr 1, 2022
Copy link
Member

@jacobtylerwalls jacobtylerwalls left a comment

Choose a reason for hiding this comment

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

nice! one suggestion to loosen this even more.

@tushar-deepsource
Copy link
Contributor Author

tushar-deepsource commented Apr 1, 2022

@jacobtylerwalls loosening it to decline any instance of Uninferable is a bit too strict.

Here's an example from the test case:

class PropertyTest(object):
    """ class """

    def __init__(self):
        self.attr = 4

    @property
    def test(self):
        """ Get the attribute """
        return self.attr

    @test.setter
    def test(self, value):
        """ Set the attribute """
        self.attr = value

PROP = PropertyTest()
PROP.test(40) # [not-callable]

This is the result of infer_call_result:

[<Const.int l.44 at 0x10eb29b10>, Uninferable]

Since we inferred one Const.int, I think that's enough evidence to raise this issue here.

@jacobtylerwalls
Copy link
Member

Thanks for looking into it. Darn.

I still think long-term we could consider making that change, though. There's no restriction on what you can set with the property. You can set something that's callable, and then not-callable becomes sort of wrong. But no one is asking for that change right now :-)

@DanielNoord
Copy link
Collaborator

@tushar-deepsource Coming into this pretty late but I was taking a look at stale PRs and found #5593. Could you quickly check if this solves that issue?

@tushar-deepsource
Copy link
Contributor Author

@DanielNoord This change only affects properties, so no it doesn't solve that issue.

Copy link
Collaborator

@DanielNoord DanielNoord left a comment

Choose a reason for hiding this comment

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

Only needs a changelog entry!

Didn't see this was only for properties. But that makes sense :)

@tushar-deepsource
Copy link
Contributor Author

@DanielNoord hopefully this is correct?

Copy link
Collaborator

@DanielNoord DanielNoord left a comment

Choose a reason for hiding this comment

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

Almost 😄

But thanks @tushar-deepsource! Wonderful fix!

Co-authored-by: Daniël van Noord <[email protected]>
@jacobtylerwalls jacobtylerwalls changed the title Fix E1102 / not-callable false positive for property that return a lambda function conditionnally Fix E1102 / not-callable false positive for property that returns a lambda function conditionally Apr 1, 2022
@jacobtylerwalls jacobtylerwalls added the Needs backport Needs to be cherry-picked on the current patch version by a pylint's maintainer label Apr 1, 2022
@jacobtylerwalls
Copy link
Member

Are we moving the changelog entry to 2.13.5?

@Pierre-Sassoulas Pierre-Sassoulas merged commit 79f3e1d into pylint-dev:main Apr 2, 2022
@Pierre-Sassoulas Pierre-Sassoulas added Backported and removed Needs backport Needs to be cherry-picked on the current patch version by a pylint's maintainer labels Apr 4, 2022
Pierre-Sassoulas added a commit that referenced this pull request Apr 4, 2022
… a lambda function conditionally (#6068)

Co-authored-by: Jacob Walls <[email protected]>
Co-authored-by: Daniël van Noord <[email protected]>
Co-authored-by: Pierre Sassoulas <[email protected]>
@tushar-deepsource tushar-deepsource deleted the false-positive-e1102 branch April 5, 2022 12:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backported Control flow Requires control flow understanding False Positive 🦟 A message is emitted but nothing is wrong with the code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

False positive: not-callable for property that returns a lambda function conditionally
6 participants