Fix astroid error for custom `next` method #7622

clavedeluna · 2022-10-14T16:22:01Z

Type of Changes

	Type
✓	🐛 Bug fix

Description

#7610 provided code that shows an edge case. When someone writes a method called def next():, pylint assumed this func will always have args.

Closes #7610

coveralls · 2022-10-14T16:41:19Z

Pull Request Test Coverage Report for Build 3377462790

3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 95.381%

Totals
Change from base Build 3367355028:	0.0%
Covered Lines:	17241
Relevant Lines:	18076

💛 - Coveralls

pylint/checkers/refactoring/refactoring_checker.py

DanielNoord · 2022-10-31T19:30:16Z

pylint/checkers/refactoring/refactoring_checker.py

@@ -1130,8 +1130,8 @@ def _looks_like_infinite_iterator(param: nodes.NodeNG) -> bool:
                return inferred.qname() in KNOWN_INFINITE_ITERATORS
            return False

-        if isinstance(node.func, nodes.Attribute):
-            # A next() method, which is now what we want.
+        if isinstance(node.func, nodes.Attribute) or not node.args:


Why don't we fix L1142? That if seem to disregard the len(args) == 0 case?

The way the code is written is pretty subtle.. let me reproduce and explain:

has_sentinel_value = len(node.args) > 1 if ( isinstance(frame, nodes.FunctionDef) and frame.is_generator() and not has_sentinel_value and not utils.node_ignores_exception(node, StopIteration) and not _looks_like_infinite_iterator(node.args[0]) ):

has_sentinel_value would be true if there are 2+ args, and False if there are 0 or 1 args. In our specific case of args 0, because this check and not has_sentinel_value is True, then we continue on to check the other two checks of the if statement and hit and not _looks_like_infinite_iterator(node.args[0]), which will fail because we just said args is len 0.

Adding or not node.args: higher up just short circuits everything cleanly.

If you have nicer code that would perform the same thing, do let me know.

Okay, but this changes still doesn't really fix the issue that is described. It just prevents the issue from happening by checking for another heuristic.

Why don't we actually check the qname of inferred to be builtins.next? That seems much more robust?

Because I hadn't thought of it :)

Good point! Seems doable, no?

Yeah I think an isinstance check and then a qname call on that line should be doable!

I thought it was impossible to do that because of this: #7622 (comment)

DanielNoord · 2022-10-31T20:29:29Z

pylint/checkers/refactoring/refactoring_checker.py

            return

        inferred = utils.safe_infer(node.func)
-        if getattr(inferred, "name", "") == "next":
+        if inferred.qname() == "builtins.next":


I think we should use an isinstance as well, to make sure the qname method actually exists.

DanielNoord · 2022-10-31T20:29:34Z

pylint/checkers/refactoring/refactoring_checker.py

@@ -1131,11 +1131,11 @@ def _looks_like_infinite_iterator(param: nodes.NodeNG) -> bool:
            return False

        if isinstance(node.func, nodes.Attribute):
-            # A next() method, which is now what we want.
+            # Only want the builtin next method.


This can be reverted.

github-actions · 2022-11-02T12:48:17Z

🤖 Effect of this PR on checked open source code: 🤖

Effect on pytest:
The following messages are now emitted:

inconsistent-return-statements:
Either all return statements in a function should return an expression, or none of them should.
https://github.com/pytest-dev/pytest/blob/50b232b0cb39490776da2658ae69c669ce263050/src/_pytest/pytester.py#L301
no-name-in-module:
No name 'NOSE_SUPPORT_METHOD' in module '_pytest.deprecated'
https://github.com/pytest-dev/pytest/blob/50b232b0cb39490776da2658ae69c669ce263050/src/_pytest/python.py#L62
no-name-in-module:
No name 'PytestReturnNotNoneWarning' in module '_pytest.warning_types'
https://github.com/pytest-dev/pytest/blob/50b232b0cb39490776da2658ae69c669ce263050/src/_pytest/python.py#L81
inconsistent-return-statements:
Either all return statements in a function should return an expression, or none of them should.
https://github.com/pytest-dev/pytest/blob/50b232b0cb39490776da2658ae69c669ce263050/src/_pytest/python.py#L1085
inconsistent-return-statements:
Either all return statements in a function should return an expression, or none of them should.
https://github.com/pytest-dev/pytest/blob/50b232b0cb39490776da2658ae69c669ce263050/src/_pytest/fixtures.py#L130
inconsistent-return-statements:
Either all return statements in a function should return an expression, or none of them should.
https://github.com/pytest-dev/pytest/blob/50b232b0cb39490776da2658ae69c669ce263050/src/_pytest/python_api.py#L798
no-name-in-module:
No name 'NOSE_SUPPORT' in module '_pytest.deprecated'
https://github.com/pytest-dev/pytest/blob/50b232b0cb39490776da2658ae69c669ce263050/src/_pytest/nose.py#L5
no-name-in-module:
No name 'warn_explicit_for' in module '_pytest.warning_types'
https://github.com/pytest-dev/pytest/blob/50b232b0cb39490776da2658ae69c669ce263050/src/_pytest/config/__init__.py#L62
no-member:
Module '_pytest.deprecated' has no 'HOOK_LEGACY_MARKING' member
https://github.com/pytest-dev/pytest/blob/50b232b0cb39490776da2658ae69c669ce263050/src/_pytest/config/__init__.py#L369

This comment was generated for commit abfd9ee

Pierre-Sassoulas

LGTM 👍

* short-circuit if next method doesnt have args * check for builtins.next qname * add inference confidence level

clavedeluna force-pushed the 7610-next branch from 6de523d to 50a11a1 Compare October 14, 2022 16:32

This comment has been minimized.

Sign in to view

Pierre-Sassoulas added Needs backport Needs to be cherry-picked on the current patch version by a pylint's maintainer Crash 💥 A bug that makes pylint crash labels Oct 14, 2022

Pierre-Sassoulas added this to the 2.15.5 milestone Oct 14, 2022

Pierre-Sassoulas reviewed Oct 14, 2022

View reviewed changes

pylint/checkers/refactoring/refactoring_checker.py Outdated Show resolved Hide resolved

clavedeluna force-pushed the 7610-next branch from bc28896 to 4882213 Compare October 19, 2022 12:24

cdce8p modified the milestones: 2.15.5, 2.15.6 Oct 21, 2022

This comment has been minimized.

Sign in to view

Pierre-Sassoulas previously approved these changes Oct 23, 2022

View reviewed changes

Pierre-Sassoulas added the Needs review 🔍 Needs to be reviewed by one or multiple more persons label Oct 23, 2022

Pierre-Sassoulas requested a review from DanielNoord October 31, 2022 13:37

DanielNoord reviewed Oct 31, 2022

View reviewed changes

clavedeluna dismissed Pierre-Sassoulas’s stale review via cfe371b October 31, 2022 20:28

clavedeluna force-pushed the 7610-next branch from 4882213 to cfe371b Compare October 31, 2022 20:28

clavedeluna requested a review from DanielNoord October 31, 2022 20:28

DanielNoord reviewed Oct 31, 2022

View reviewed changes

clavedeluna added 7 commits November 2, 2022 09:08

short-circuit if next method doesnt have args

e6f95b9

add frag

87ec26e

update tests

2966ce9

add test for next with args

19d6e28

check for builtins.next qname

20f9b41

add inference confidence level

499eedd

check isinstance

abfd9ee

clavedeluna force-pushed the 7610-next branch from 260f13e to abfd9ee Compare November 2, 2022 12:16

clavedeluna requested a review from DanielNoord November 3, 2022 11:26

DanielNoord approved these changes Nov 3, 2022

View reviewed changes

Pierre-Sassoulas approved these changes Nov 3, 2022

View reviewed changes

Pierre-Sassoulas merged commit c8cd335 into pylint-dev:main Nov 3, 2022

Pierre-Sassoulas added Backported and removed Needs backport Needs to be cherry-picked on the current patch version by a pylint's maintainer labels Nov 16, 2022

Pierre-Sassoulas pushed a commit that referenced this pull request Nov 16, 2022

Fix astroid error for custom next method (#7622)

5668e56

* short-circuit if next method doesnt have args * check for builtins.next qname * add inference confidence level

Pierre-Sassoulas pushed a commit that referenced this pull request Nov 17, 2022

Fix astroid error for custom next method (#7622)

8b0bb16

* short-circuit if next method doesnt have args * check for builtins.next qname * add inference confidence level

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix astroid error for custom `next` method #7622

Fix astroid error for custom `next` method #7622

clavedeluna commented Oct 14, 2022 •

edited

coveralls commented Oct 14, 2022 •

edited

This comment has been minimized.

This comment has been minimized.

DanielNoord Oct 31, 2022

clavedeluna Oct 31, 2022

DanielNoord Oct 31, 2022

clavedeluna Oct 31, 2022

DanielNoord Oct 31, 2022

Pierre-Sassoulas Oct 31, 2022

DanielNoord Oct 31, 2022

DanielNoord Oct 31, 2022

github-actions bot commented Nov 2, 2022

Pierre-Sassoulas left a comment

Fix astroid error for custom next method #7622

Fix astroid error for custom next method #7622

Conversation

clavedeluna commented Oct 14, 2022 • edited

Type of Changes

Description

coveralls commented Oct 14, 2022 • edited

Pull Request Test Coverage Report for Build 3377462790

💛 - Coveralls

This comment has been minimized.

This comment has been minimized.

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

Choose a reason for hiding this comment

github-actions bot commented Nov 2, 2022

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

Fix astroid error for custom `next` method #7622

Fix astroid error for custom `next` method #7622

clavedeluna commented Oct 14, 2022 •

edited

coveralls commented Oct 14, 2022 •

edited