Skip to content
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.

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

Fix RecursionError in infer_call_result() #2432

Merged
merged 4 commits into from
May 14, 2024

Conversation

jacobtylerwalls
Copy link
Member

Type of Changes

Type
βœ“ πŸ› Bug fix

Description

Closes pylint-dev/pylint#9139

Copy link

codecov bot commented May 12, 2024

Codecov Report

All modified and coverable lines are covered by tests βœ…

Project coverage is 92.78%. Comparing base (0ccc2e2) to head (5dfa9e2).
Report is 2 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #2432   +/-   ##
=======================================
  Coverage   92.78%   92.78%           
=======================================
  Files          94       94           
  Lines       11098    11102    +4     
=======================================
+ Hits        10297    10301    +4     
  Misses        801      801           
Flag Coverage Ξ”
linux 92.59% <100.00%> (+<0.01%) ⬆️
pypy 92.78% <100.00%> (+<0.01%) ⬆️
windows 92.68% <100.00%> (+<0.01%) ⬆️

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

Files Coverage Ξ”
astroid/bases.py 88.32% <100.00%> (+0.12%) ⬆️

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.

I see nothing wrong here, but I would lie if I say I understand what was happening here :D would you mind explaining (very slowly) ?

@jacobtylerwalls
Copy link
Member Author

jacobtylerwalls commented May 13, 2024

Of course! I noticed that the recursion error reports in pylint seemed to have a line from infer_call_result() in common in the tracebacks, so I looked to see if there was an addressable issue there. I found that pytorch was doing something elaborate with __call__ that was causing astroid to get stuck in a loop, evaluating the result of a call to __call__.

I edited the test to show this more clearly. Inferring the result of A.__call__() requires... inferring the result of A(), which requires inferring the result of A.__call__(), ...

If you comment out the code change and run the test, you'll see the recursive calls in the pytest output:

astroid/bases.py:334: in infer_call_result
    for res in node.infer_call_result(caller, context):
astroid/bases.py:334: in infer_call_result
    for res in node.infer_call_result(caller, context):
astroid/bases.py:334: in infer_call_result
    for res in node.infer_call_result(caller, context):
astroid/bases.py:334: in infer_call_result
    for res in node.infer_call_result(caller, context):
astroid/bases.py:334: in infer_call_result
    for res in node.infer_call_result(caller, context):
astroid/bases.py:334: in infer_call_result
    for res in node.infer_call_result(caller, context):
astroid/bases.py:334: in infer_call_result
    for res in node.infer_call_result(caller, context):
astroid/bases.py:334: in infer_call_result
    for res in node.infer_call_result(caller, context):
astroid/bases.py:334: in infer_call_result
    for res in node.infer_call_result(caller, context):
astroid/bases.py:334: in infer_call_result
    for res in node.infer_call_result(caller, context):
astroid/bases.py:334: in infer_call_result
    for res in node.infer_call_result(caller, context):
astroid/bases.py:334: in infer_call_result
    for res in node.infer_call_result(caller, context):
astroid/bases.py:334: in infer_call_result
    for res in node.infer_call_result(caller, context):
astroid/bases.py:334: in infer_call_result
    for res in node.infer_call_result(caller, context):
astroid/bases.py:334: in infer_call_result
    for res in node.infer_call_result(caller, context):
astroid/bases.py:334: in infer_call_result
    for res in node.infer_call_result(caller, context):
astroid/bases.py:334: in infer_call_result
    for res in node.infer_call_result(caller, context):
astroid/bases.py:334: in infer_call_result
    for res in node.infer_call_result(caller, context):
astroid/bases.py:334: in infer_call_result
    for res in node.infer_call_result(caller, context):
astroid/bases.py:334: in infer_call_result
    for res in node.infer_call_result(caller, context):
astroid/bases.py:334: in infer_call_result
    for res in node.infer_call_result(caller, context):
astroid/bases.py:334: in infer_call_result
    for res in node.infer_call_result(caller, context):
astroid/bases.py:334: in infer_call_result
    for res in node.infer_call_result(caller, context):
astroid/bases.py:334: in infer_call_result
    for res in node.infer_call_result(caller, context):
astroid/bases.py:334: in infer_call_result
    for res in node.infer_call_result(caller, context):
astroid/bases.py:334: in infer_call_result
    for res in node.infer_call_result(caller, context):
astroid/bases.py:334: in infer_call_result
    for res in node.infer_call_result(caller, context):
astroid/bases.py:334: in infer_call_result
    for res in node.infer_call_result(caller, context):
astroid/bases.py:334: in infer_call_result
    for res in node.infer_call_result(caller, context):
astroid/bases.py:334: in infer_call_result
    for res in node.infer_call_result(caller, context):
astroid/bases.py:334: in infer_call_result
    for res in node.infer_call_result(caller, context):
astroid/bases.py:334: in infer_call_result
    for res in node.infer_call_result(caller, context):
astroid/bases.py:334: in infer_call_result
    for res in node.infer_call_result(caller, context):

The point of the patch is to notice that the two things being checked are the same class and bail out.

@jacobtylerwalls
Copy link
Member Author

(The self and node are both Instances of A, and the ._proxied on each is the ClassDef for A they have in common.)

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.

Thank you for explaining, make sense !

@jacobtylerwalls jacobtylerwalls added the pylint-tested PRs that don't cause major regressions with pylint label May 13, 2024
@Pierre-Sassoulas
Copy link
Member

(Sorry forgot to approve)

@jacobtylerwalls jacobtylerwalls merged commit d1c37a9 into main May 14, 2024
21 checks passed
@jacobtylerwalls jacobtylerwalls deleted the fix-recursion-error-call-result branch May 14, 2024 13:39
github-actions bot pushed a commit that referenced this pull request May 14, 2024
jacobtylerwalls added a commit that referenced this pull request May 14, 2024
(cherry picked from commit d1c37a9)

Co-authored-by: Jacob Walls <jacobtylerwalls@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport maintenance/3.2.x pylint-tested PRs that don't cause major regressions with pylint
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RecursionError escapes during inference
2 participants