Skip to content

Refactor Qt brain transforms to handle PyQt and PySide #2863

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Refactor Qt brain transforms to handle PyQt and PySide #2863

wants to merge 1 commit into from

Conversation

emmanuel-ferdman
Copy link
Contributor

Type of Changes

Type
βœ“ πŸ› Bug fix

Description

This PR rewrites astroid/brain/brain_qt.py to correctly infer Qt signals for both PyQt and PySide, regardless of how the binding exposes them: Function-style signals (descriptor) or Class-style signals. Main changes:

  • Reorganized astroid/brain/brain_qt.py to share explicit PyQt/PySide signal templates, restoring connect/disconnect/emit inference for both toolkits. I believe it also improves readability of code.
  • Handle function-style vs class-style signals: Some bindings (e.g., PySide β‰₯ 5.15.2 / PySide6) expose Signal via the descriptor protocol, so it infers as a FunctionDef. The transform now catches that path and attaches the expected members. See Example 1 (subclassed QTimer.timeout) and Example 2 (class-style Signal() instance) below. Other bindings (e.g., PyQt5/6) expose pyqtSignal as a class, so it infers as a ClassDef. We register a matching class transform there as well.
  • Enabled the PyQt6 brain tests across every supported Python version (Previously it was only for 3.10).

Please note that I tried to add tests to PySide6 but it looks like PyQt6 and PySide6 cannot coexist in a single Linux process because their wheels bundle different Qt 6 library revisions (similiary to concolution in work made in PR #1654). Due to this limiation, I added pragma: no cover for the PySide code branches.

Previously failing examples - Example 1:

from PySide6.QtCore import QTimer


class CustomTimer(QTimer):
    def __init__(self) -> None:
        super().__init__()
        self.timeout.connect(self.on_timeout)

    def on_timeout(self) -> None:
        pass


def build_timer() -> CustomTimer:
    timer = CustomTimer()
    timer.timeout.disconnect()
    return timer

Used to emit:

input.py:8:8: E1101: Method '<no-name>' has no 'connect' member (no-member)
input.py:16:4: E1101: Method '<no-name>' has no 'disconnect' member (no-member)

Example 2:

from PySide6.QtCore import Signal


sig = Signal()
sig.connect(lambda: None)

Used to emit:

input.py:5:0: E1101: Instance of 'Signal' has no 'connect' member (no-member)

The same pattern affected PyQt5 and PyQt6 as well (with their appropriate API/syntax).

Fixes #2850.

@codecov Codecov
Copy link

codecov bot commented Oct 14, 2025 •
edited
Loading

Codecov Report

βœ… All modified and coverable lines are covered by tests.
βœ… Project coverage is 93.40%. Comparing base (ab119c2) to head (dc6d093).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2863      +/-   ##
==========================================
+ Coverage   93.35%   93.40%   +0.05%     
==========================================
  Files          92       92              
  Lines       11190    11209      +19     
==========================================
+ Hits        10446    10470      +24     
+ Misses        744      739       -5
Flag Coverage Ξ”
linux 93.27% <100.00%> (+0.05%) ⬆️
pypy 93.40% <100.00%> (+0.05%) ⬆️
windows 93.38% <100.00%> (+0.05%) ⬆️

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

Files with missing lines Coverage Ξ”
astroid/brain/brain_qt.py 100.00% <100.00%> (+17.24%) ⬆️
πŸš€ New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Pierre-Sassoulas Pierre-Sassoulas added the Brain 🧠 Needs a brain tip label Oct 15, 2025
@Pierre-Sassoulas Pierre-Sassoulas added this to the 4.1.0 milestone Oct 15, 2025
Pierre-Sassoulas
Pierre-Sassoulas reviewed Oct 15, 2025
View reviewed changes
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 test matrix for both pyqt 5 and 6 would better work in a separated repository, but we only have an idea of creating astroid plugin repository at this point. Question for other maintainers : This might be the occasion to do a proof of concept for astroid plugin ?

tests/brain/test_qt.py
Comment on lines -19 to -20
# TODO: enable for Python 3.12 as soon as PyQt6 release is compatible
@pytest.mark.skipif(PY312_PLUS, reason="This test was segfaulting with Python 3.12.")
Copy link
Member

Choose a reason for hiding this comment

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

πŸ‘

.github/workflows/ci.yaml
Comment on lines 100 to 103
- name: Install Qt
if: ${{ matrix.python-version == '3.10' }}
run: |
sudo apt-get update
sudo apt-get install build-essential libgl1-mesa-dev
Copy link
Member

Choose a reason for hiding this comment

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

Should we add a matrix for pyqt 5 / 6 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Pierre-Sassoulas I think I can add a matrix for PyQt5/6 if you think that’s the right approach - though they share most of the same code paths now. I could also try adding a PySide6 matrix, which might be more valuable since it goes through a different branch than PyQt, but it would require installing different libraries in the ci.
Also, currently, we have PyQt6 in the requirements file. If we decide to go with a matrix, I’m not sure whether the right approach is to include both PyQt6 and PySide6 in the requirements or to install them dynamically during CI. Let me know what you prefer, and I’ll implement it.

Copy link
Member

Choose a reason for hiding this comment

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

Would you be open to create a proof of concept of astroid plugin ? I.e. a repo containing the pyqt brain that we add as an optional dependency to pylint or astroid later (*) and that we extensively test in it's own repo with as much dependency as required (pyqt from 1 to 6, anything is possible because the test won't slow down the main repo). If you are then the question on the other comment is moot because this is clearly the superior way to do thing, we're just limited by available maintainer time to do it.

(*) Not sure for this part the brain need astroid so if astroid has an optional dependency to it it's going to be a circular one ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Pierre-Sassoulas Sure! I’d be happy to look into this. Are there any existing Astroid plugins I could review to better understand how this should be implemented?

Copy link
Member

Choose a reason for hiding this comment

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

It's a proof of concept so it would be the first astroid plugin. Let's discuss in #1312 if required :)

@gillins gillins mentioned this pull request Oct 17, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Pierre-Sassoulas Pierre-Sassoulas Pierre-Sassoulas left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

Brain 🧠 Needs a brain tip

Projects

None yet

Milestone

4.1.0

Development

Successfully merging this pull request may close these issues.

astroid-4.0.0 does not resolve PySide6.QtCore.QTimer::timeout.connect

2 participants

@emmanuel-ferdman @Pierre-Sassoulas