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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

(馃悶) bad unnecessary-dunder-call report for object.__getattribute__(self within a __getattribute__ implementation #9486

Closed
KotlinIsland opened this issue Jan 12, 2024 · 8 comments 路 Fixed by #9496
Assignees
Labels
bug Something isn't working

Comments

@KotlinIsland
Copy link
Contributor

class A:
    a = 1

    def __getattribute__(self, item):
        return object.__getattribute__(self, item)  # PLC2801 Unnecessary dunder call to `__getattribute__`. Access attribute directly or use getattr built-in function..

A().a

Ah yes, better use getattr built-in function..

class A:
    a = 1

    def __getattribute__(self, item):
        return getattr(self, item)

A().a
#   File "C:\Users\charlie_marsh(parody)\rust_based_python_typechecker\__init__.py", line 5, in __getattribute__
#    return getattr(self, item)
#           ^^^^^^^^^^^^^^^^^^^
#  [Previous line repeated 745 more times]
#RecursionError: maximum recursion depth exceeded
@KotlinIsland KotlinIsland changed the title (馃悶) bad unnecessary-dunder-call report for object.__getattribute__(self with a __getattribute__ implementation (馃悶) bad unnecessary-dunder-call report for object.__getattribute__(self within a __getattribute__ implementation Jan 12, 2024
@charliermarsh charliermarsh added the bug Something isn't working label Jan 12, 2024
@charliermarsh
Copy link
Member

What is going on with that filename lol

@randolf-scholz
Copy link

randolf-scholz commented Jan 12, 2024

Other example of a false-positive:

class A:
    def __repr__(self):
        return "Prints this text"

a = A()
object.__repr__(a)  # PLC2801

Generally, for T.__dunder__(obj, *args, **kwargs) to be redundant (assuming isinstance(T, type)), I think we necessarily need type(obj).__dunder__ is T.__dunder__ to be true?

@charliermarsh
Copy link
Member

Need to understand how pylint handles this.

@charliermarsh
Copy link
Member

So for one thing, they omit dunder calls within dunder methods.

@charliermarsh
Copy link
Member

It looks like they also skip dunder calls on non-instantiated classes? Which includes object?

@bastimeyer
Copy link

So for one thing, they omit dunder calls within dunder methods.

This is the main cause of PLC2801 being raised in our project after updating ruff to 0.1.13.

Another false positive report (IMO) after upgrading to the latest version is the explicit call of dunder methods in test cases. Imagine a __getitem__ method which gets tested for a specific exception in a with pytest.raises(...) block. Ruff now wants me to use the subscript notation (foo[bar] instead of foo.__getitem__(bar)), which is then reported as a "statement without an effect" by other linters (like the one built into pycharm for example). Calling the __getitem__ method explicitly should be ok in test cases and shouldn't require an annotation for suppressing PLC2801.

@charliermarsh
Copy link
Member

Yeah we can easily fix that part. The second part is harder and will require us to greatly limit the scope of the rule.

@charliermarsh
Copy link
Member

(I'll fix the first part now.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants