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

False positive in arguments-differ rule when overriding an instance method with a static one #6865

Closed
cpitclaudel opened this issue Jun 5, 2022 · 5 comments
Labels
C: arguments-differ Issues related to 'arguments-differ' and 'signature-differs' checks Discussion 🤔

Comments

@cpitclaudel
Copy link
Contributor

cpitclaudel commented Jun 5, 2022

Bug description

Pylint complains about the following sample:

class Base:
    def xyz(self, arg: int) -> int:
        raise NotImplementedError

class Derived(Base):
    def __init__(self):
        pass
    @staticmethod
    def xyz(arg: int) -> int: // arguments-differ
        return arg + 1

print(Derived().xyz(3))

This is the reverse of #1482 . While overriding a static method with a non-static one is unsafe (the overriden method cannot be called directly on the class), it looks like the reverse (this issue) is safe: Derived.xyz can be called on both Derived and Derived instances.

Configuration

No response

Command used

python test.py

Pylint output

************* Module test
/tmp/test.py:9:4: W0221: Number of parameters was 2 in 'Base.xyz' and is now 1 in overridden 'Derived.xyz' method (arguments-differ)

Expected behavior

There should be no error reported for this case.

Pylint version

pylint 2.14.0
astroid 2.11.5
Python 3.10.4 (main, Apr  2 2022, 09:04:19) [GCC 11.2.0]

OS / Environment

Ubuntu 22.04 LTS

Additional dependencies

No response

@cpitclaudel cpitclaudel added the Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling label Jun 5, 2022
@cdce8p cdce8p added Discussion 🤔 C: arguments-differ Issues related to 'arguments-differ' and 'signature-differs' checks False Positive 🦟 A message is emitted but nothing is wrong with the code and removed Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling False Positive 🦟 A message is emitted but nothing is wrong with the code labels Jun 6, 2022
@cdce8p
Copy link
Member

cdce8p commented Jun 6, 2022

Why would you want to add @staticmethod to the overridden instance methods? If you want to call the overridden method on the class itself, I would suggest to create a new method for it instead.

@cpitclaudel
Copy link
Contributor Author

Why would you want to add @staticmethod to the overridden instance methods?

As a signal to the reader that the self parameter is unused; is it a bad thing?

If you want to call the overridden method on the class itself, I would suggest to create a new method for it instead.

Or add a pylint: ignore comment, but those are both just workarounds, right?

@cdce8p
Copy link
Member

cdce8p commented Jun 7, 2022

Why would you want to add @staticmethod to the overridden instance methods?

As a signal to the reader that the self parameter is unused; is it a bad thing?

I personally don't think it should be used that way. Think of @staticmethod more as a concept. Is the method something which should effect all instances or will it always be called on the class itself: MyClass.some_method()?

If it's just one implementation that happens to not use self, using @staticmethod just to hide the attribute is the wrong solution. We decided to disable the no-self-use checker recently because it encouraged that exact (wrong) behavior. #5502

If you want to call the overridden method on the class itself, I would suggest to create a new method for it instead.

Or add a pylint: ignore comment, but those are both just workarounds, right?

Creating a new method isn't a "workaround", at least not if you think of it conceptually (see above). By their nature, true static- and instance methods are best if they are separate.

@cdce8p cdce8p closed this as not planned Won't fix, can't repro, duplicate, stale Jun 7, 2022
@cpitclaudel
Copy link
Contributor Author

will it always be called on the class itself: MyClass.some_method()?

That isn't right? Unlike, say, C#, Python can call static method through instances.

If it's just one implementation that happens to not use self, using @staticmethod just to hide the attribute is the wrong solution.

BUt it doesn't have to be that: instead it can be that a whole sub-hierarchy of the base class (multiple derived classes with a common parent) are not supposed to use self; in that case, putting @staticmethod on the derived implementation is the right thing to do, no?

@cdce8p
Copy link
Member

cdce8p commented Jun 9, 2022

will it always be called on the class itself: MyClass.some_method()?

That isn't right? Unlike, say, C#, Python can call static method through instances.

Not everything you can do in Python is good practice ;)

If it's just one implementation that happens to not use self, using @staticmethod just to hide the attribute is the wrong solution.

BUt it doesn't have to be that: instead it can be that a whole sub-hierarchy of the base class (multiple derived classes with a common parent) are not supposed to use self; in that case, putting @staticmethod on the derived implementation is the right thing to do, no?

I disagree. That's probably a bad code design. If a method is deals with a specific instance, it shouldn't use self. Even if the concrete implementation happens to not use self.

In any case, you can still use pylint: disable=arguments-differ if you want to do it differently.

nmlorg added a commit to nmlorg/metabot that referenced this issue Jul 8, 2023
Silence the rest of the recent pylint changes (most notably see pylint-dev/pylint#6865).
nmlorg added a commit to nmlorg/metabot.calendars.google that referenced this issue Jun 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: arguments-differ Issues related to 'arguments-differ' and 'signature-differs' checks Discussion 🤔
Projects
None yet
Development

No branches or pull requests

2 participants