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

Prefer last same-named function in a class rather than first in igetattr() #1173

Merged

Conversation

nelfin
Copy link
Contributor

@nelfin nelfin commented Sep 15, 2021

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

Ref #1015. When there are multiple statements defining some attribute
ClassDef.igetattr filters out later definitions if they are not in the
same scope as the first (to support cases like "self.a = 1; self.a = 2"
or "self.items = []; self.items += 1"). However, it checks the scope of
the first attribute against the parent scope of the later attributes.
For mundane statements this makes no difference, but for
scope-introducing statements such as FunctionDef these are not the same.

Type of Changes

Type
🐛 Bug fix

Related Issue

Closes #1015

@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.8.1 milestone Sep 15, 2021
Comment on lines 4036 to 4038
assert len(inferred) == 2
assert isinstance(inferred[0], nodes.Const)
assert isinstance(inferred[1], Generator)
Copy link
Member

@cdce8p cdce8p Sep 15, 2021

Choose a reason for hiding this comment

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

I'm wondering if

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

should be inferred as Const and Generator as Python will always use the last definition. Wouldn't it be better to only infer it as Generator?

--
Some test cases to be aware of

if some_test:
    def func():
        return 42
else:
    def func():
        return bool

# should still return two results for `func()`

Functools.singledispatch should still work:
https://docs.python.org/3/library/functools.html#functools.singledispatch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wouldn't it be better to only infer it as Generator?

My thinking leans the same way as it's the more "useful" definition.

Python will always use the last definition

Pretty much, but I wonder what to do about monkeypatching, or this (contrived) example:

class A:
    def foo(self):
        return 'first'
    bar = foo(object())
    def foo(self):
        return 'second'

A().foo()  # 'second'
A().bar  # 'first'

Copy link
Member

Choose a reason for hiding this comment

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

Just tested your example: This will already be inferred incorrectly. A().foo() ->'first'. Possibly another bug?

As for the issue at hand. Would it work to filter previous definitions only if no other attributes are in between?

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.

The case with two foo function one redefined are a little contrived and probably not seen a lot in real "sane" code. The @overload use case though seems interesting to test. Do you think this is really equivalent ?

@Pierre-Sassoulas Pierre-Sassoulas removed this from the 2.8.1 milestone Oct 5, 2021
@DanielNoord
Copy link
Collaborator

@nelfin Just going through some stale PRs. Have you given the questions raised here some thoughts? Perhaps somewhat related is my comment here which also relates to "picking the correct definition".

@jacobtylerwalls jacobtylerwalls added this to the 2.13.0 milestone May 1, 2022
@DanielNoord DanielNoord removed this from the 2.13.0 milestone Jul 2, 2022
@jacobtylerwalls jacobtylerwalls added the Needs take over 🛎️ Orignal implementer went away but the code could be salvaged. label Jun 27, 2023
Ref pylint-dev#1015. When there are multiple statements defining some attribute
ClassDef.igetattr filters out later definitions if they are not in the
same scope as the first (to support cases like "self.a = 1; self.a = 2"
or "self.items = []; self.items += 1"). However, it checks the scope of
the first attribute against the *parent scope* of the later attributes.
For mundane statements this makes no difference, but for
scope-introducing statements such as FunctionDef these are not the same.
@jacobtylerwalls jacobtylerwalls removed Work in progress Needs take over 🛎️ Orignal implementer went away but the code could be salvaged. labels Feb 25, 2024
@jacobtylerwalls jacobtylerwalls marked this pull request as ready for review February 25, 2024 00:18
@jacobtylerwalls jacobtylerwalls added this to the 3.2.0 milestone Feb 25, 2024
@jacobtylerwalls
Copy link
Member

We're closing tons of duplicates for this in pylint. I think special-casing functions (sans properties) is good enough.

Copy link

codecov bot commented Feb 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.75%. Comparing base (12ed435) to head (06a5f2b).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1173   +/-   ##
=======================================
  Coverage   92.75%   92.75%           
=======================================
  Files          94       94           
  Lines       11080    11084    +4     
=======================================
+ Hits        10277    10281    +4     
  Misses        803      803           
Flag Coverage Δ
linux 92.56% <100.00%> (+<0.01%) ⬆️
pypy 90.76% <100.00%> (+<0.01%) ⬆️
windows 92.34% <100.00%> (+<0.01%) ⬆️

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

Files Coverage Δ
astroid/nodes/scoped_nodes/scoped_nodes.py 92.52% <100.00%> (+0.02%) ⬆️

@jacobtylerwalls jacobtylerwalls changed the title Fix overzealous filtering of FunctionDef nodes in ClassDef.igetattr Prefer last function defined in a class rather than first in igetattr() Mar 1, 2024
@jacobtylerwalls jacobtylerwalls changed the title Prefer last function defined in a class rather than first in igetattr() Prefer last same-named function in a class rather than first in igetattr() Mar 1, 2024
@jacobtylerwalls jacobtylerwalls merged commit a7f5d5f into pylint-dev:main Mar 8, 2024
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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)
5 participants