Make `FunctionDef.implicit_parameters` return 1 for methods #1531

jacobtylerwalls · 2022-04-26T18:29:45Z

Description

When a node is inferred as FunctionDef instead of BoundMethod, implicit_parameters() always returns 0, which can cause problems in pylint with no-value-for-parameter and such. We can at least fall back to the knowledge that it is bound and return 1.

Type of Changes

	Type
✓	🐛 Bug fix

Related Issue

Closes pylint-dev/pylint#6464 (because we will not be writing Qt tests there)

Todo: see if this closes any other pylint issues for no-value-for-parameter

Relatedly, make `FunctionDef.is_bound` return True for instance methods.

jacobtylerwalls · 2022-04-26T18:30:08Z

@The-Compiler does this resolve the false positive for you?

jacobtylerwalls · 2022-04-26T18:36:03Z

astroid/nodes/scoped_nodes/scoped_nodes.py

@@ -1642,7 +1645,7 @@ def is_bound(self):
            False otherwise.
        :rtype: bool
        """
-        return self.type == "classmethod"
+        return self.type in {"method", "classmethod"}


This method was untested according to coveralls.

coveralls · 2022-04-26T18:37:23Z

Pull Request Test Coverage Report for Build 2250456664

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

Totals
Change from base Build 2250330420:	0.02%
Covered Lines:	9135
Relevant Lines:	9969

💛 - Coveralls

Pierre-Sassoulas

LGTM !

jacobtylerwalls · 2022-04-27T13:58:42Z

Todo: see if this closes any other pylint issues for no-value-for-parameter

I can't say I checked every one, but I checked multiple and this didn't seem to fix any.

Pierre-Sassoulas · 2022-04-27T17:50:55Z

tests/unittest_brain_qt.py

+        if attribute_node is Uninferable:
+            pytest.skip("PyQt6 C bindings may not be installed?")


Don't we want to force the c binding to work correctly in at least one of the CI pipeline ? Otherwise this test can work fine online and fail for someone with QT locally. I think we could create a pytest mark for "qt" and a CI job with a proper installation.

Yes, it works in the "release tests" workflow.

Hmm, it means we're going to catch issues at release time right ? When/how are we launching this workflow ?

Just manually with a reminder in the release.md.

Installing qt takes 39s. Unless there's a way to cache it?

For 40s we might as well add it for each commit (one interpreter only ?). There's a lot of available runner for astroid and they run in a reasonable time (compared to pylint anyway).

Actually, it takes 8s, duh, because most of that linked run was setting up the env!

I'm confused - why does "Install Qt" actually only do sudo apt-get install build-essential libgl1-mesa-dev? You are installing PyQt from binary wheels, so that 40s step can probably just be dropped entirely.

We discovered that the PyQt6 wheel doesn't necessarily install the C bindings on ubuntu-latest, see failing run and comment.

Realized that one job was skipped if QT6 was not properly installed.

astroid/nodes/scoped_nodes/scoped_nodes.py

tests/unittest_brain_qt.py

The-Compiler · 2022-04-28T09:21:02Z

@The-Compiler does this resolve the false positive for you?

Almost! It still complains on two lines compared to 2.13.7:

https://github.com/qutebrowser/qutebrowser/blob/6fc30a1e851dbc07461ba50d4266bbcae40e1fb1/qutebrowser/misc/throttle.py#L88

and

https://github.com/qutebrowser/qutebrowser/blob/6fc30a1e851dbc07461ba50d4266bbcae40e1fb1/tests/unit/config/test_configtypes.py#L226

The former sounds like pylint/astroid thinks a slot is needed for .disconnect(), but that's not the case, it can be called without one. I think that's a bug in the Qt brain here:

https://github.com/PyCQA/astroid/blob/8fda6c667a11050d36b267ded157933bbafb8290/astroid/brain/brain_qt.py#L30

(should be slot=None, same for PySide most likely).

The latter gives me too-many-function-args and is probably an unrelated false-positive regression.

jacobtylerwalls · 2022-04-28T12:44:06Z

Thanks for testing. Feel free to open new issues or PRs. Apparently slot is optional, but you cannot pass an explicit None, see python-qt-tools/PyQt5-stubs#103. I'm a little hesitant to show that slot=None is the default value in that case. See also pylint-dev/pylint#5264 for general weakness in this area.

Discussed here: pylint-dev#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.

* 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.

Make FunctionDef.implicit_parameters return 1 for instance methods

124f5bd

Relatedly, make `FunctionDef.is_bound` return True for instance methods.

change container type

0540c55

jacobtylerwalls changed the title ~~Make FunctionDef.implicit_parameters return 1 for instance methods~~ Make FunctionDef.implicit_parameters return 1 for methods Apr 26, 2022

remove specific phrasing in changelog

0d200ae

Pierre-Sassoulas added the Bug 🪳 label Apr 26, 2022

generalize allowlist

0812abb

jacobtylerwalls commented Apr 26, 2022

View reviewed changes

Pierre-Sassoulas previously approved these changes Apr 26, 2022

View reviewed changes

jacobtylerwalls added this to the 2.11.4 milestone Apr 26, 2022

jacobtylerwalls added the needs-backport label Apr 26, 2022

jacobtylerwalls added the pylint-tested PRs that don't cause major regressions with pylint label Apr 27, 2022

Pierre-Sassoulas reviewed Apr 27, 2022

View reviewed changes

jacobtylerwalls mentioned this pull request Apr 27, 2022

Install Qt on 3.10 Linux runner #1533

Merged

DanielNoord reviewed Apr 28, 2022

View reviewed changes

astroid/nodes/scoped_nodes/scoped_nodes.py Outdated Show resolved Hide resolved

tests/unittest_brain_qt.py Outdated Show resolved Hide resolved

The-Compiler mentioned this pull request Apr 28, 2022

Fix instance_attrs building error in Qt brain #1505

Merged

add typing

a2b8438

jacobtylerwalls requested a review from DanielNoord April 28, 2022 13:17

DanielNoord approved these changes Apr 28, 2022

View reviewed changes

Merge branch 'main' into qt-no-value-for-param

ad3d987

Pierre-Sassoulas approved these changes Apr 30, 2022

View reviewed changes

jacobtylerwalls merged commit 9968101 into pylint-dev:main Apr 30, 2022

jacobtylerwalls deleted the qt-no-value-for-param branch April 30, 2022 15:57

Pierre-Sassoulas added Backported and removed needs-backport labels May 2, 2022

Pierre-Sassoulas pushed a commit that referenced this pull request May 2, 2022

Make FunctionDef.implicit_parameters return 1 for methods (#1531)

c4e11cb

Pierre-Sassoulas pushed a commit that referenced this pull request May 2, 2022

Make FunctionDef.implicit_parameters return 1 for methods (#1531)

be9235c

The-Compiler mentioned this pull request May 10, 2022

qt brain: Make slot argument optional for disconnect() #1550

Merged

2 tasks

jacobtylerwalls mentioned this pull request May 12, 2022

Incorrect too-many-function-args for a function defined and called within a class pylint-dev/pylint#6592

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make `FunctionDef.implicit_parameters` return 1 for methods #1531

Make `FunctionDef.implicit_parameters` return 1 for methods #1531

jacobtylerwalls commented Apr 26, 2022 •

edited

Loading

jacobtylerwalls commented Apr 26, 2022

jacobtylerwalls Apr 26, 2022

coveralls commented Apr 26, 2022 •

edited

Loading

Pierre-Sassoulas left a comment

jacobtylerwalls commented Apr 27, 2022

Pierre-Sassoulas Apr 27, 2022

jacobtylerwalls Apr 27, 2022

Pierre-Sassoulas Apr 27, 2022

jacobtylerwalls Apr 27, 2022

Pierre-Sassoulas Apr 27, 2022

jacobtylerwalls Apr 27, 2022

The-Compiler Apr 28, 2022

jacobtylerwalls Apr 28, 2022 •

edited

Loading

The-Compiler commented Apr 28, 2022 •

edited

Loading

jacobtylerwalls commented Apr 28, 2022

		if attribute_node is Uninferable:
		pytest.skip("PyQt6 C bindings may not be installed?")

Make FunctionDef.implicit_parameters return 1 for methods #1531

Make FunctionDef.implicit_parameters return 1 for methods #1531

Conversation

jacobtylerwalls commented Apr 26, 2022 • edited Loading

Description

Type of Changes

Related Issue

jacobtylerwalls commented Apr 26, 2022

Choose a reason for hiding this comment

coveralls commented Apr 26, 2022 • edited Loading

Pull Request Test Coverage Report for Build 2250456664

💛 - Coveralls

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

jacobtylerwalls commented Apr 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacobtylerwalls Apr 28, 2022 • edited Loading

Choose a reason for hiding this comment

The-Compiler commented Apr 28, 2022 • edited Loading

jacobtylerwalls commented Apr 28, 2022

Make `FunctionDef.implicit_parameters` return 1 for methods #1531

Make `FunctionDef.implicit_parameters` return 1 for methods #1531

jacobtylerwalls commented Apr 26, 2022 •

edited

Loading

coveralls commented Apr 26, 2022 •

edited

Loading

jacobtylerwalls Apr 28, 2022 •

edited

Loading

The-Compiler commented Apr 28, 2022 •

edited

Loading