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

Log RecursionError out as warning during inference #9614

Closed
wants to merge 2 commits into from

Conversation

jacobtylerwalls
Copy link
Member

Partner to pylint-dev/astroid#2385

Type of Changes

Type
βœ“ πŸ› Bug fix

Description

Closes #9139

Copy link

codecov bot commented May 12, 2024

Codecov Report

Attention: Patch coverage is 19.58042% with 115 lines in your changes are missing coverage. Please review.

Project coverage is 95.25%. Comparing base (fd6790b) to head (ed4fe10).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #9614      +/-   ##
==========================================
- Coverage   95.83%   95.25%   -0.59%     
==========================================
  Files         174      174              
  Lines       18888    19014     +126     
==========================================
+ Hits        18102    18111       +9     
- Misses        786      903     +117     
Files Coverage Ξ”
pylint/checkers/base/comparison_checker.py 100.00% <ΓΈ> (ΓΈ)
pylint/checkers/refactoring/refactoring_checker.py 98.06% <100.00%> (-0.21%) ⬇️
pylint/checkers/newstyle.py 90.90% <0.00%> (-5.25%) ⬇️
...heckers/refactoring/implicit_booleaness_checker.py 97.60% <0.00%> (-2.41%) ⬇️
pylint/extensions/typing.py 96.02% <62.50%> (-1.66%) ⬇️
pylint/checkers/classes/special_methods_checker.py 92.34% <0.00%> (-3.14%) ⬇️
pylint/checkers/stdlib.py 95.03% <14.28%> (-1.92%) ⬇️
pylint/checkers/strings.py 93.00% <0.00%> (-1.28%) ⬇️
pylint/pyreverse/utils.py 90.34% <14.28%> (-3.86%) ⬇️
pylint/checkers/base/basic_checker.py 96.01% <0.00%> (-2.07%) ⬇️
... and 4 more

This comment has been minimized.

@jacobtylerwalls jacobtylerwalls marked this pull request as ready for review May 12, 2024 17:38
Comment on lines +237 to +239
except RecursionError:
warn_on_recursion_error()
return {ann} if ann else set()
Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe we should do this in astroid after all?

Copy link
Contributor

πŸ€– According to the primer, this change has no effect on the checked open source code. πŸ€–πŸŽ‰

This comment was generated for commit ed4fe10

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.

Great ! Clearly a lot of work went into this. I'm not sure New contributor or plugin maintainer will be able to keep up in new code. The exception catching around infer seem almost mandatory and not very DRY now. Maybe we could reraise an inference error with warning from astroid directly ? Or rework safe_infer / infer ?

Other than that I think it's pretty hard to know how much this will be shown in case of a problematic code base. I assume it can happens from more than once, to too much. Maybe we need an antispam limit or some ux refinement (one warning per full lint, with the number of total error ? One warning per second max ?)

@jacobtylerwalls
Copy link
Member Author

Clearly a lot of work went into this.

No, don't worry, this went pretty fast, happy to discuss alternatives :-)

Maybe we could reraise an inference error with warning from astroid directly ?

Yeah, I'm thinking we should do that after all.

@jacobtylerwalls jacobtylerwalls deleted the warn-recursion branch May 12, 2024 18:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RecursionError escapes during inference
2 participants