Skip to content

Fix astroid error for custom next method#7622

Merged
Pierre-Sassoulas merged 7 commits into
pylint-dev:mainfrom
clavedeluna:7610-next
Nov 3, 2022
Merged

Fix astroid error for custom next method#7622
Pierre-Sassoulas merged 7 commits into
pylint-dev:mainfrom
clavedeluna:7610-next

Conversation

@clavedeluna
Copy link
Copy Markdown
Contributor

@clavedeluna clavedeluna commented Oct 14, 2022

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
Copy link
Copy Markdown

coveralls commented Oct 14, 2022

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 Coverage Status
Change from base Build 3367355028: 0.0%
Covered Lines: 17241
Relevant Lines: 18076

💛 - Coveralls

@github-actions

This comment has been minimized.

@Pierre-Sassoulas 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 Pierre-Sassoulas added this to the 2.15.5 milestone Oct 14, 2022
Comment thread pylint/checkers/refactoring/refactoring_checker.py Outdated
@github-actions

This comment has been minimized.

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

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:
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Because I hadn't thought of it :)

Good point! Seems doable, no?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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


inferred = utils.safe_infer(node.func)
if getattr(inferred, "name", "") == "next":
if inferred.qname() == "builtins.next":
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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


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

Choose a reason for hiding this comment

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

This can be reverted.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Nov 2, 2022

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

Effect on pytest:
The following messages are now emitted:

Details
  1. 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
  2. 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
  3. 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
  4. 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
  5. 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
  6. 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
  7. 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
  8. 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
  9. 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

Copy link
Copy Markdown
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.

LGTM 👍

@Pierre-Sassoulas Pierre-Sassoulas merged commit c8cd335 into pylint-dev:main Nov 3, 2022
@Pierre-Sassoulas 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
* 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
* short-circuit if next method doesnt have args
* check for builtins.next qname
* add inference confidence level
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Backported Crash 💥 A bug that makes pylint crash Needs review 🔍 Needs to be reviewed by one or multiple more persons

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Crash when running pylint against the webpy repo

5 participants