Fixes #4040, #5378 - signal has no connect member

[adam-grant-hendry]

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

PR #900 contained a comment with a suggested fix that solved the problem experienced by users.

To reiterate, for PySide2 5.15.2+ and PySide6, pylint complains that signals have no connect member, even though all objects that inherit from QObject have a connect signal. Issue #900 explains PySide2 version 5.15.2 changed the Signal class to implement the descriptor protocol, hence pylint began detecting signals as FunctionDefs instead of ClassDefs causing them not to be detected and patched anymore. The issue persisted in PySide6, hence the comment that solved the problem was made.

Unfortunately, the reviewer and original submitter are no longer active with the project, so it was requested a new PR be made referencing the old one that contains the fix.

Type of Changes

	Type
✓	🐛 Bug fix

Related Issue

See original PR #900

Closes pylint-dev/pylint#4040 and closes pylint-dev/pylint#5378

[coveralls]

Pull Request Test Coverage Report for Build 2559882861

• 5 of 5 (100.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.02%) to 92.321%

---

Totals
Change from base Build 2558067714:	0.02%
Covered Lines:	9401
Relevant Lines:	10183

---

💛 - Coveralls

[jacobtylerwalls]

Thanks for resurrecting this stalled issue! Would you be able to prepare a version of this PR that avoids unrelated formatting changes?

[adam-grant-hendry]

Thanks for resurrecting this stalled issue! Would you be able to prepare a version of this PR that avoids unrelated formatting changes?

Yes, apologies for that. That should be fixed in my last commit.

[Pierre-Sassoulas]

Thank you for the cleanup, and thank you for ignoring the generated doc, we'tr not giving enough love to the doc in astroid and it shows 😅 Do you think there could be a way to test that without installing dependencies before tests ?

[adam-grant-hendry]

Thank you for the cleanup, and thank you for ignoring the generated doc, we'tr not giving enough love to the doc in astroid and it shows 😅 Do you think there could be a way to test that without installing dependencies before tests ?

Do you mean test that the generated docs in doc/_build don't get tracked by git?

[Pierre-Sassoulas]

I was thinking about test for the qt brain.

[DanielNoord]

Sorry for pushing to your branch directly. I saw we had a very similar function so I thought it made sense just to merge the two and keep the detection of Signal in one place.

Would you be able to write a test for this? The rest of the qt brain is also covered, so there should be some pre-existing tests.

[jacobtylerwalls]

Would you be able to write a test for this? The rest of the qt brain is also covered, so there should be some pre-existing tests.

Yes, it is covered, but only with PyQt6 as the Qt library, not PySide2 or 6. Would you propose to add PySide6 as an additional test requirement?

whitespace changes fixed

[DanielNoord]

Would you be able to write a test for this? The rest of the qt brain is also covered, so there should be some pre-existing tests.

Yes, it is covered, but only with PyQt6 as the Qt library, not PySide2 or 6. Would you propose to add PySide6 as an additional test requirement?

That's perhaps a bit much. Should we add coverage pragma's then?

[adam-grant-hendry]

@DanielNoord @jacobtylerwalls

I can confirm on my machine that the following passes for PyQt5 and PyQt6, but fails for PySide2 and PySide6 unless the #900 fix is implemented:

tests/unitest_brain_qt.py

# Licensed under the LGPL: https://www.gnu.org/licenses/old-licenses/lgpl-2.1.en.html
# For details: https://github.com/PyCQA/astroid/blob/main/LICENSE
# Copyright (c) https://github.com/PyCQA/astroid/blob/main/CONTRIBUTORS.txt

from importlib.util import find_spec

import pytest

from astroid import Uninferable, extract_node
from astroid.bases import UnboundMethod
from astroid.manager import AstroidManager
from astroid.nodes import FunctionDef

HAS_PYQT5 = find_spec("PyQt5")
HAS_PYSIDE2 = find_spec("PySide2")
HAS_PYQT6 = find_spec("PyQt6")
HAS_PYSIDE6 = find_spec("PySide6")

...

@pytest.mark.skipif(
    any(
        (
            HAS_PYQT5 is None,
            HAS_PYSIDE2 is None,
            HAS_PYQT6 is None,
            HAS_PYSIDE6 is None,
        ),
    ),
    reason="These tests require the PyQt5, PySide2, PyQt6, and PySide6 libraries.",
)
class TestBrainQt_ConnectSignalMember:
    AstroidManager.brain["extension_package_whitelist"] = {
        "PyQt5",
        "PySide2",
        "PyQt6",
        "PySide6",
    }

    @staticmethod
    @pytest.mark.parametrize("qt_binding", ["PyQt5", "PySide2", "PyQt6", "PySide6"])
    def test_connect_signal_detected(
        qt_binding: str,
    ) -> None:
        """Test signals have .connect() signal.

        This is a regression test for:
            - https://github.com/PyCQA/pylint/issues/4040
            - https://github.com/PyCQA/pylint/issues/5378

        See PR: https://github.com/PyCQA/astroid/pull/1654

        Args:
            qt_binding(str): Python Qt binding (one of PyQt5, PySide2, PyQt6, or PySide6)
        """
        # Below does not work for some reason, though it would be more concise
        #
        # src = """
        # from binding import QtWidgets
        # app = QtWidgets.QApplication([])
        # app.focusChanged  #@
        # """
        #
        # src_lines = src.split("\n")
        #
        # for ndx, line in enumerate(src_lines):
        #     src_lines[ndx] = line.strip()
        #     if ndx == 0:
        #         src_lines[0] = src_lines[0].replace("binding", qt_binding)
        #
        # src = "\n".join(src_lines)

        if qt_binding == "PyQt5":
            src = """
            from PyQt5 import QtWidgets
            app = QtWidgets.QApplication([])
            app.focusChanged  #@
            """
        elif qt_binding == "PySide2":
            src = """
            from PySide2 import QtWidgets
            app = QtWidgets.QApplication([])
            app.focusChanged  #@
            """
        elif qt_binding == "PyQt6":
            src = """
            from PyQt6 import QtWidgets
            app = QtWidgets.QApplication([])
            app.focusChanged  #@
            """
        elif qt_binding == "PySide6":
            src = """
            from PySide6 import QtWidgets
            app = QtWidgets.QApplication([])
            app.focusChanged  #@
            """
        else:
            pytest.skip(f"{qt_binding} is not a Python Qt library.")

        node = extract_node(src)
        attribute_node = node.inferred()[0]
        if attribute_node is Uninferable:
            pytest.skip(f"{qt_binding} C bindings may not be installed?")
        assert isinstance(attribute_node.instance_attrs["connect"][0], FunctionDef)

[adam-grant-hendry]

I'll push my test up for your review.

[adam-grant-hendry]

Sorry for pushing to your branch directly. I saw we had a very similar function so I thought it made sense just to merge the two and keep the detection of Signal in one place.

Would you be able to write a test for this? The rest of the qt brain is also covered, so there should be some pre-existing tests.

Quite alright, no worries. I'll fix the docstring too.

[Pierre-Sassoulas]

LGTM, nice merge request !

[adam-grant-hendry]

I was thinking about test for the qt brain.

Ah yes, I see. Yes, I've added a regression test that should works for PyQt5, PySide2, PyQt6, and PySide6. It requires adding the following to requirements_test_brain.txt though, just FYI, so unfortunately to your other question no: additional dependencies must be installed:

requirements_test_brain.txt

...
PyQt5>=5.15.2
PySide2>=5.15.2
...
PySide6

[adam-grant-hendry]

@DanielNoord @Pierre-Sassoulas @jacobtylerwalls It looks as though for Python 3.11-dev that PySide2 only goes up to 5.13.2 and that the Linux CI tests are failing due to segfaults.

How would you like to proceed?

[adam-grant-hendry]

Sorry for pushing to your branch directly. I saw we had a very similar function so I thought it made sense just to merge the two and keep the detection of Signal in one place.
Would you be able to write a test for this? The rest of the qt brain is also covered, so there should be some pre-existing tests.

Quite alright, no worries. I'll fix the docstring too.

I also hope I haven't inadvertently overwritten any of your suggestions. I don't believe I have, but let me know if something got missed. I had to rebase a couple times to pull in your latest changes.

[DanielNoord]

I also hope I haven't inadvertently overwritten any of your suggestions. I don't believe I have, but let me know if something got missed. I had to rebase a couple times to pull in your latest changes.

Sadly you did 😄 But no worries, I'll repush. You're also more than welcome to use git pull and don't rebase. For small, related changes such as this we tend to squash merge anyway!

[adam-grant-hendry]

@Pierre-Sassoulas Python 3.7 to 3.9 continue to segfault. I notice RuntimeWarning messages in 3.10, which I've encountered before:

shiboken6, which creates the python bindings for Qt C++ source, imports from a zip file into the top-level directory at runtime. These files are deleted after running, but coverage attempts to look at their source after they're gone, causing
warnings to appear. Namely, it looks for these modules/files:

project_dir/pysrcript
project_dir/shibokensupport
project_dir/signature_bootstrap.py

To avoid this error, source can be specified to the package subdirectory (i.e. astroid/). However, this can also be avoided by explicitly omitting these folders in the omit section for coverage.

See nedbat/coveragepy#1392

[adam-grant-hendry]

I also hope I haven't inadvertently overwritten any of your suggestions. I don't believe I have, but let me know if something got missed. I had to rebase a couple times to pull in your latest changes.

Sadly you did 😄 But no worries, I'll repush. You're also more than welcome to use git pull and don't rebase. For small, related changes such as this we tend to squash merge anyway!

I'm so sorry about that!

[adam-grant-hendry]

@Pierre-Sassoulas Python 3.7 to 3.9 continue to segfault. I notice RuntimeWarning messages in 3.10, which I've encountered before:

shiboken6, which creates the python bindings for Qt C++ source, imports from a zip file into the top-level directory at runtime. These files are deleted after running, but coverage attempts to look at their source after they're gone, causing warnings to appear. Namely, it looks for these modules/files:

project_dir/pysrcript project_dir/shibokensupport project_dir/signature_bootstrap.py

To avoid this error, source can be specified to the package subdirectory (i.e. astroid/). However, this can also be avoided by explicitly omitting these folders in the omit section for coverage.

See nedbat/coveragepy#1392

@DanielNoord Perhaps you'd like to review this as well. I'm not sure what's causing the segfault errors I'm seeing.

[DanielNoord]

I haven't followed the discussion, but these test (that I originally asked for 😓) seem to be causing a lot of issues.

I would be fine with your confirmation that the latest version of this PR fixes the issue and adding some pragma: no cover comments on the code that is not covered. This seems like an awful lot of trouble for a simple str() in list() check.

[adam-grant-hendry]

Oh this is much cleaner, thank you! I like it.

[adam-grant-hendry]

I haven't followed the discussion, but these test (that I originally asked for 😓) seem to be causing a lot of issues.

I would be fine with your confirmation that the latest version of this PR fixes the issue and adding some pragma: no cover comments on the code that is not covered. This seems like an awful lot of trouble for a simple str() in list() check.

Oh nice! Let me try one more thing though. I noticed I accidentally altered the original class TestBrainQt to skip if HAS_PYSIDE6 when it should be HAS_PYQT6.

I'd like to run the CI one more time and if that doesn't fix it, I'll remove the test, changes to requirements_test_brain.txt, and add the # pragma: no covers.

[adam-grant-hendry]

I haven't followed the discussion, but these test (that I originally asked for 😓) seem to be causing a lot of issues.
I would be fine with your confirmation that the latest version of this PR fixes the issue and adding some pragma: no cover comments on the code that is not covered. This seems like an awful lot of trouble for a simple str() in list() check.

Oh nice! Let me try one more thing though. I noticed I accidentally altered the original class TestBrainQt to skip if HAS_PYSIDE6 when it should be HAS_PYQT6.

I'd like to run the CI one more time and if that doesn't fix it, I'll remove the test, changes to requirements_test_brain.txt, and add the # pragma: no covers.

Okay, unfortunately that didn't work, so I reverted the test and requirements_test_brain.txt and added the # pragma: no covers. Hopefully CI should pass now.

[adam-grant-hendry]

@DanielNoord Would you be able to see if CI will now honor the # pragma: no covers?

[adam-grant-hendry]

Sorry for pushing to your branch directly. I saw we had a very similar function so I thought it made sense just to merge the two and keep the detection of Signal in one place.

Would you be able to write a test for this? The rest of the qt brain is also covered, so there should be some pre-existing tests.

I believe this was completed, but is still being shown as not resolved (FYI).

[Pierre-Sassoulas]

Sorry for the tests that were so hard to do. Thank you for all the work and taking remarks into account @adam-grant-hendry.