Update brain_qt to work with pyside2 5.15.2

[g-rocket]

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

We have some logic in brain_qt to detect Qt Signals and patch connect, disconnect, and emit methods on to them.
However, in pyside2 5.15.2, they changed some things so now Qt Signals are detected as FunctionDefs instead
of ClassDefs, and as a result we're not detecting and patching them anymore. I add some new logic to patch these methods into any function in a module named PySide2 with a __class__ named Signal. I've tested and this seems to work for me, and it should be tightly scoped enough to not break anything else. Unfortunately, I can't find a way to restrict it based on what module the Signal class was defined in, as asteroid doesn't seem to know that. See pylint-dev/pylint#4040 for more discussion.

Also -- not sure if I formatted the changelog right; I assume I missed the boat for 2.5?

Type of Changes

	Type
✓	🐛 Bug fix

Related Issue

Closes pylint-dev/pylint#4040

[hippo91]

@g-rocket thanks for this PR and sorry for the delay.
I'm really busy right now and understanding the changes in PySide2 was quite hard.
I eventually understood what was happening between version 5.15.1 and 5.15.2.
As you said the Signal methods are no more inferred as SIgnal instances but as FunctionDef.
I believe it is due to the fact that starting with PySide2 5.15.2 the Signal class does in fact implements the descriptor protocol. Thus astroid when building its ast declares the node to be a FunctionDef with an attibute __class__ pointing to Signal.
You can find this mechanism in astroid's raw_building module at lines 365 - (222 - 232) - (65-75).
I have checked and the _add_dunder_class is called only in the case of method descriptor, which is our case of interest here.
Thus your assumption to test the __class__ attribute seems to be finely scoped. I have no problem with it.
However i think the treatment of such node may be modified to better reflect what the interpreter is really using.
For example in a ipython console you get:

In [1]: from PySide2.QtWidgets import QApplication

In [2]: from PySide2.QtCore import Signal

In [3]: QApplication.focusChanged
Out[3]: <PySide2.QtCore.Signal at 0x7f33d08e4b50>

Thus to me it makes sense that the corresponding focusChanged node should be inferred as an instance of the Signal class.
That is why i wonder if something like:

def transform_pyside_signal_descriptor(node):
    src = """
    class NotPySideSignal(object):
        def connect(self, receiver, type=None):
            pass
        def disconnect(self, receiver):
            pass
        def emit(self, *args):
            pass
    """
    inst = extract_node(src).instantiate_class()
    node.parent.locals[node.name] = [inst]

is not more pertinent.
As my understanding of PySide2 is extremely limited i would have your opinion on it.
Especially can you confirm that Signal class has just the three methods connect, disconnect and emit?

[hippo91]

Should not be PySide2 ?
Moreover you can be a little bit more explicit by saying that this is for taking into account the fact that Signal implements the descriptor protocol. (see the discussion below).

[hippo91]

I think it would be more readable if you avoid a lambda here and defines a specific function for that.

[jordanbray]

I needed a similar patch. This request almost works for PySide6 as well. (Note that PySide6 is the next version after PySide2).

I saw the review as well, and I believe this version of the end of the file works for everyone. I can submit a new pull request if this request is dead.

def _looks_like_pyside2_or_6_signal(node):
    """Detect a PySide2 or PySide6 signal.  These changed locations as of QT 5ish apparently."""

    is_pyside_node = node.qname().partition(".")[0] in ("PySide2", "PySide6")
    is_named_signal = any(
        cls.qname() == "Signal" for cls in node.instance_attrs.get("__class__", [])
    )

    return is_pyside_node and is_named_signal


register_module_extender(MANAGER, "PyQt4.QtCore", pyqt4_qtcore_transform)
MANAGER.register_transform(nodes.FunctionDef, transform_pyqt_signal, _looks_like_signal)
MANAGER.register_transform(
    nodes.ClassDef,
    transform_pyside_signal,
    lambda node: node.qname() in ("PySide.QtCore.Signal", "PySide2.QtCore.Signal"),
)

MANAGER.register_transform(
    nodes.FunctionDef,
    transform_pyside_signal,
    _looks_like_pyside2_or_6_signal
)

[g-rocket]

Sorry it's been so long without updates. I have absolutely no pyside2 expertise (just work with a project that happens to use it that I want to be able to lint) and have been struggling to confirm that transform_pyside_signal is correct (i.e. Signal should have only the three methods listed there). As far as I can tell it seems to be and it has been working with older versions just fine so I see no reason to block this until we can figure that out.

@jordanbray I'm happy to update the PR to your version, but I think @hippo91 wants us to also play around with how transform_pyside_signal works on the inferred-as-function signals to ensure that the extra methods are added to the right thing.

[jordanbray]

I'll answer your questions below, but first let me ask my own? In order to get rid of the ugly redefinition of the Signal class, how would I change this to transform all PySide6.QtCore.Signal into a PySide6.QtCore.SignalInstance? The class defined above is a duplicate of the PySide6.QtCore.SignalInstance. I looked around the code and couldn't find an example for me to copy, lol.

Ok, onto answering your questions:

• Is the class definition transform_pyside_signal correct?

It appears so. The docs for that page are down, but at least in PySide6, you can view the cached version here

https://webcache.googleusercontent.com/search?q=cache:YIuX1QtrVqcJ:https://doc.qt.io/qtforpython/PySide6/QtCore/Signal.html+&cd=1&hl=en&ct=clnk&gl=us&client=firefox-b-1-d

As you can see, it exposes only those 3 functions.

• Why is there a register_transform(node.ClassDef ...) and a register_transform(nodes.FunctionDef, ...)?

It appears this is because they changed the syntax. I have not proven this, but here is a link to what the old syntax was vs the new syntax. https://wiki.qt.io/Qt_for_Python_Signals_and_Slots#Traditional_syntax:_SIGNAL_.28.29_and_SLOT.28.29

• Why can't we infer the emit, connect and disconnect?

I'm not sure how to do it. The Signal has a different type depending on how you infer it. Each class has a static signal variable of type PySide6.QtCore.Signal that refers to the signal, and each instance of the class has a very different variable with the same name, which is of type PySide6.QtCore.SignalInstance.

Example:

>>> from PySide6.QtWidgets import QApplication, QPushButton
>>> a = QApplication([]) # boilerplate
>>> b = QPushButton("Test")
>>> b.clicked
<PySide6.QtCore.SignalInstance object at 0x7f66c8cf3210>
>>> QPushButton.clicked
<PySide6.QtCore.Signal object at 0x7f66c8dada10>
>>> 'emit' in dir(QPushButton.clicked)
False
>>> 'emit' in dir(b.clicked)
True
>>>

How do we handle this? Well' see my above question. I'd be happy to submit an update given a gentle nudge in the correct direction.

[hippo91]

@jordanbray thanks for your remark. I'm afraid i do not understand what you are asking concerning the Signal class versus SignalInstance one. Can you explain me a little bit more please?

[jordanbray]

So, in the code I posted, the code you tried to merge, and the old PySIde astroid config, the "magic trick" that made pylint stop complaining was this:

If you see a "Signal" object, pretend you actually saw a "NotPySideSignal" object, which is a class we define poorly.

Instead, I would like to implement the following magic trick:

If you see a "Signal" object, pretend you actually saw a "SignalInstance" object.

This means that at least we're defining the type to what it will be at runtime.

[hippo91]

@jordanbray thanks for your explanation. I understand now. This is probably possible. One trick could maybe to use extract_node function in such a way:

src = """
from PySide2 import SignalInstance
klass = SignalInstance(your_args)
klass
"""
return astroid.extract_node(src)

[adam-grant-hendry]

I needed a similar patch. This request almost works for PySide6 as well. (Note that PySide6 is the next version after PySide2).

I saw the review as well, and I believe this version of the end of the file works for everyone. I can submit a new pull request if this request is dead.

def _looks_like_pyside2_or_6_signal(node):
    """Detect a PySide2 or PySide6 signal.  These changed locations as of QT 5ish apparently."""

    is_pyside_node = node.qname().partition(".")[0] in ("PySide2", "PySide6")
    is_named_signal = any(
        cls.qname() == "Signal" for cls in node.instance_attrs.get("__class__", [])
    )

    return is_pyside_node and is_named_signal


register_module_extender(MANAGER, "PyQt4.QtCore", pyqt4_qtcore_transform)
MANAGER.register_transform(nodes.FunctionDef, transform_pyqt_signal, _looks_like_signal)
MANAGER.register_transform(
    nodes.ClassDef,
    transform_pyside_signal,
    lambda node: node.qname() in ("PySide.QtCore.Signal", "PySide2.QtCore.Signal"),
)

MANAGER.register_transform(
    nodes.FunctionDef,
    transform_pyside_signal,
    _looks_like_pyside2_or_6_signal
)

I can confirm this worked for me.

What is the status of the patch? Can we move it forward?

[DanielNoord]

I needed a similar patch. This request almost works for PySide6 as well. (Note that PySide6 is the next version after PySide2).
I saw the review as well, and I believe this version of the end of the file works for everyone. I can submit a new pull request if this request is dead.

def _looks_like_pyside2_or_6_signal(node):
    """Detect a PySide2 or PySide6 signal.  These changed locations as of QT 5ish apparently."""

    is_pyside_node = node.qname().partition(".")[0] in ("PySide2", "PySide6")
    is_named_signal = any(
        cls.qname() == "Signal" for cls in node.instance_attrs.get("__class__", [])
    )

    return is_pyside_node and is_named_signal


register_module_extender(MANAGER, "PyQt4.QtCore", pyqt4_qtcore_transform)
MANAGER.register_transform(nodes.FunctionDef, transform_pyqt_signal, _looks_like_signal)
MANAGER.register_transform(
    nodes.ClassDef,
    transform_pyside_signal,
    lambda node: node.qname() in ("PySide.QtCore.Signal", "PySide2.QtCore.Signal"),
)

MANAGER.register_transform(
    nodes.FunctionDef,
    transform_pyside_signal,
    _looks_like_pyside2_or_6_signal
)

I can confirm this worked for me.

What is the status of the patch? Can we move it forward?

Both the reviewer and original submitter of this PR aren't really active within the project anymore. I think the best step forward would be a new PR so we can review that as is. Perhaps with a little reference to this PR in the PR description.

[adam-grant-hendry]

Both the reviewer and original submitter of this PR aren't really active within the project anymore. I think the best step forward would be a new PR so we can review that as is. Perhaps with a little reference to this PR in the PR description.

You got it! Happy to help. I'll submit a new PR with a reference link shortly.

[DanielNoord]

Closing this then! Thanks!

[adam-grant-hendry]

@DanielNoord Quick question. The CONTRIBUTORS.txt doesn't really have any guidance like a typical CONTRIBUTING file would.

Am I correct in assuming all the following should be considered when contributing? Are there any additional requirements?:

1. Create a fork of the repo, clone it, set the upstream, create a virtual environment, and do an editable install

PS> git clone https://github.com/forked-repo/astroid.git
PS> cd astroid
PS> git remote add upstream https://github.com/PyCQA/astroid
PS> py -m venv .venv && .venv/Scripts/Activate.ps1
PS> py -m pip install -e .

2. Install the requirements

PS> py -m pip install -r requirements_test.txt
PS> py -m pip install -r requirements_test_brain.txt
PS> py -m pip install -r requirements_test_min.txt
PS> py -m pip install -r requirements_test_pre_commit.txt

3. Install pre-commit

ps> pre-commit install

4. Create a separate branch following typical Conventional Commits branch naming. e.g.

fix/ = Bug fix
feat/ = New Feature
refactor/ = Refactor
etc.

PS> git checkout -b your/branch

5. Commit with appropriate details, including reference to appropriate PR(s).

PS> git add -A && git commit  # in your/branch
PS> git checkout master && git pull upstream master
PS> git push origin master
PS> git push -u origin your/branch 

6. Create a PR requesting to merge the forked branch into master/main

[Pierre-Sassoulas]

See https://github.com/PyCQA/pylint#contributing and https://pylint.pycqa.org/en/latest/development_guide/contributor_guide/index.html

[adam-grant-hendry]

See https://github.com/PyCQA/pylint#contributing and https://pylint.pycqa.org/en/latest/development_guide/contributor_guide/index.html

Thank you!

[adam-grant-hendry]

@DanielNoord @Pierre-Sassoulas Please see PR #1654