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 signature-differs / arguments-differ with typing.overload #5264

Open
cdce8p opened this issue Nov 5, 2021 · 2 comments
Open
Labels
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 Needs PR This issue is accepted, sufficiently specified and now needs an implementation

Comments

@cdce8p
Copy link
Member

cdce8p commented Nov 5, 2021

Bug description

Noticed that one while working on pylint-dev/astroid#1217 with @DanielNoord.

The logic for signature-differs / arguments-differ can't handle function redefinitions as its common with typing.overload. In particular I narrowed it down to two issue:

  1. All function definitions seem to be check, even those decorated with overload.
  2. The point of comparison is always the first definition, even if that isn't the true signature.

--
The examples require Python 3.8 (due to the use of typing.Literal). Not that they aren'T necessarily typed correctly. Just for illustration proposes.

Example 1

# pylint: disable=unused-argument,no-self-use,missing-docstring
from typing import Literal, Union, overload

class Parent:
    @overload
    def statement(self) -> int:
        ...

    @overload
    def statement(self, future: Literal[True]) -> str:
        ...

    def statement(self, future: Literal[None, True] = None) -> Union[int, str]:
        pass


class Child(Parent):
    # Signature comparison happens with first definition - the overload
    # Should use the actual function instead!
    def statement(self, future=None):  # arguments-differ (false-positive)
        pass

Example 2

# pylint: disable=unused-argument,no-self-use,missing-docstring
from typing import Literal, Union, overload

class Parent2:
    def statement(self, future: Literal[None, True] = None) -> Union[int, str]:
        pass

class Child2(Parent2):
    @overload
    def statement(self, future: Literal[None] = ...) -> int:
        ...

    @overload
    def statement(self, future: Literal[True]) -> str:  # signature-differs (false-positive)
        # Even overloads are checked although these aren't required
        # to have the correct signature
        ...

    def statement(self, future: Literal[None, True] = None) -> Union[int, str]:
        pass

Configuration

No response

Command used

pylint test.py

Pylint output

W0221: Number of parameters was 1 in 'Parent.statement' and is now 2 in overridden 'Child.statement' method (arguments-differ)

W0222: Signature differs from overridden 'statement' method (signature-differs)

Expected behavior

no errors

Pylint version

2.11.2-dev0

OS / Environment

No response

Additional dependencies

No response

@cdce8p cdce8p added Bug 🪲 Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling 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 labels Nov 5, 2021
@cdce8p cdce8p added the C: arguments-differ Issues related to 'arguments-differ' and 'signature-differs' checks label Nov 17, 2021
@DanielNoord
Copy link
Collaborator

DanielNoord commented Dec 16, 2021

@cdce8p I narrowed down the issue with selecting the first overload instead of the actual signature due to the __getitem__ method of LocalsDictNodeNG in astroid. See:
https://github.com/PyCQA/astroid/blob/e3730a7aaf35030edcafbb5ec0509fb8ff070e33/astroid/nodes/scoped_nodes.py#L316-L324

This returns self.locals[item][0]. For overload this doesn't make sense and should probably be self.locals[item][-1]. I wonder if any other checks actually need to use [0]: if there are multiple definitions of a method in a class definition won't the last one be used anyway?

Do we want to fix this in astroid by changing the __getitem__ method (either by checking if decorated with overload, or changing for all methods) or do we want to create a workaround in pylint?

Pierre-Sassoulas pushed a commit to pylint-dev/astroid that referenced this issue May 12, 2022
* qt brain: Make slot argument optional for disconnect()

Discussed here:
#1531 (comment)

PyQt supports calling .disconnect() without any arguments in order to
disconnect all slots:
https://www.riverbankcomputing.com/static/Docs/PyQt6/signals_slots.html#disconnect

Strictly speaking, slot=None is a wrong call, as actually passing None
does not work:
python-qt-tools/PyQt5-stubs#103

However, pylint/astroid does not support overloads needed to properly
express this: pylint-dev/pylint#5264

So, while this is "wrong", it's less wrong than before - without this
change, pylint expects a mandatory argument, thus raising a
false-positive here:

    from PyQt5.QtCore import QTimer
    t = QTimer()
    t.timeout.connect(lambda: None)
    t.timeout.disconnect()

despite running fine, pylint complains:

    test.py:4:0: E1120: No value for argument 'slot' in method call (no-value-for-parameter)

while with this change, things work fine.
Pierre-Sassoulas pushed a commit to pylint-dev/astroid that referenced this issue Jun 13, 2022
* qt brain: Make slot argument optional for disconnect()

Discussed here:
#1531 (comment)

PyQt supports calling .disconnect() without any arguments in order to
disconnect all slots:
https://www.riverbankcomputing.com/static/Docs/PyQt6/signals_slots.html#disconnect

Strictly speaking, slot=None is a wrong call, as actually passing None
does not work:
python-qt-tools/PyQt5-stubs#103

However, pylint/astroid does not support overloads needed to properly
express this: pylint-dev/pylint#5264

So, while this is "wrong", it's less wrong than before - without this
change, pylint expects a mandatory argument, thus raising a
false-positive here:

    from PyQt5.QtCore import QTimer
    t = QTimer()
    t.timeout.connect(lambda: None)
    t.timeout.disconnect()

despite running fine, pylint complains:

    test.py:4:0: E1120: No value for argument 'slot' in method call (no-value-for-parameter)

while with this change, things work fine.
Pierre-Sassoulas pushed a commit to pylint-dev/astroid that referenced this issue Jun 13, 2022
* qt brain: Make slot argument optional for disconnect()

Discussed here:
#1531 (comment)

PyQt supports calling .disconnect() without any arguments in order to
disconnect all slots:
https://www.riverbankcomputing.com/static/Docs/PyQt6/signals_slots.html#disconnect

Strictly speaking, slot=None is a wrong call, as actually passing None
does not work:
python-qt-tools/PyQt5-stubs#103

However, pylint/astroid does not support overloads needed to properly
express this: pylint-dev/pylint#5264

So, while this is "wrong", it's less wrong than before - without this
change, pylint expects a mandatory argument, thus raising a
false-positive here:

    from PyQt5.QtCore import QTimer
    t = QTimer()
    t.timeout.connect(lambda: None)
    t.timeout.disconnect()

despite running fine, pylint complains:

    test.py:4:0: E1120: No value for argument 'slot' in method call (no-value-for-parameter)

while with this change, things work fine.
@Pierre-Sassoulas Pierre-Sassoulas added Needs PR This issue is accepted, sufficiently specified and now needs an implementation and removed Bug 🪲 labels Jul 5, 2022
@Pierre-Sassoulas
Copy link
Member

The first example was fixed in 2.15.0-dev0, not the second one.

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 False Positive 🦟 A message is emitted but nothing is wrong with the code Needs PR This issue is accepted, sufficiently specified and now needs an implementation
Projects
None yet
Development

No branches or pull requests

3 participants