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.

Sign up for GitHub

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

Jump to bottom

Detect unreachable code after sys.exit, quit, exit, and os._exit #7520

Merged
merged 21 commits into from
Nov 13, 2022
Merged

Detect unreachable code after sys.exit, quit, exit, and os._exit #7520

merged 21 commits into from
Nov 13, 2022

Conversation

clavedeluna
Copy link
Contributor

@clavedeluna clavedeluna commented Sep 23, 2022 •
edited by jacobtylerwalls
Loading

Type of Changes

Type
✓ ✨ New feature

Description

Pylint can now detect unreachable code for any code that follows a sys.exit call

Refs #519

@coveralls
Copy link

coveralls commented Sep 23, 2022 •
edited
Loading

Pull Request Test Coverage Report for Build 3457101611

  • 16 of 16 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.004%) to 95.408%
Totals Coverage Status
Change from base Build 3456941332: 0.004%
Covered Lines: 17347
Relevant Lines: 18182
💛 - Coveralls

@clavedeluna clavedeluna marked this pull request as ready for review September 23, 2022 15:52
@github-actions

This comment has been minimized.

Sign in to view
@mbyrnepr2
Copy link
Member

mbyrnepr2 commented Sep 23, 2022 •
edited
Loading

Thanks for tackling this @clavedeluna!

I think the Django primer result shows that some control flow is needed since the message shouldn't emit in this case in the else block.

@Pierre-Sassoulas Pierre-Sassoulas added Control flow Requires control flow understanding False Negative 🦋 No message is emitted but something is wrong with the code labels Sep 23, 2022
@github-actions

This comment has been minimized.

Sign in to view
@clavedeluna
Copy link
Contributor Author

I'm still trying to figure out how to use the primer results, it's confusing because the run hasn't finished but there's an output above in the PR

@Pierre-Sassoulas
Copy link
Member

it's confusing because the run hasn't finished but there's an output above in the PR

There's a new comment each time the primer is run, the new one should appear shortly.

@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
Pierre-Sassoulas
Pierre-Sassoulas requested changes Sep 28, 2022
View reviewed changes
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.

It's starting to look pretty good, thank you @clavedeluna :)

pylint/checkers/base/basic_checker.py Outdated
"""
if self._is_sys_exit(node):
self._check_unreachable(node)
Copy link
Member

Choose a reason for hiding this comment

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

The confidence need to be INFERENCE in this case, but it should still be HIGH when we don't need inference.

pylint/checkers/base/basic_checker.py Outdated Show resolved Hide resolved
clavedeluna and others added 21 commits November 13, 2022 22:57
@clavedeluna @Pierre-Sassoulas
detect unreachable sys.exit
78e311e
@clavedeluna @Pierre-Sassoulas
document new func
a363fc9
@clavedeluna @Pierre-Sassoulas
add doc
cab9811
@pre-commit-ci @Pierre-Sassoulas
[pre-commit.ci] auto fixes from pre-commit.com hooks
5d5b975 
for more information, see https://pre-commit.ci
@mbyrnepr2 @Pierre-Sassoulas
Update doc/whatsnew/fragments/519.other
346aae1
@pre-commit-ci @Pierre-Sassoulas
[pre-commit.ci] auto fixes from pre-commit.com hooks
a02ba93 
for more information, see https://pre-commit.ci
@clavedeluna @Pierre-Sassoulas
handle edge case of sys.exit in a lambda func
1a9765e
@pre-commit-ci @Pierre-Sassoulas
[pre-commit.ci] auto fixes from pre-commit.com hooks
812f69c 
for more information, see https://pre-commit.ci
@clavedeluna @Pierre-Sassoulas
Update tests/functional/u/unreachable.py
55615d3 
Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com>
@clavedeluna @Pierre-Sassoulas
should infer sys.exit call
81dffde
@clavedeluna @Pierre-Sassoulas
safe infer
6c5c5bd
@clavedeluna @Pierre-Sassoulas
add inference confidence
059960e
@clavedeluna @mbyrnepr2 @Pierre-Sassoulas
Apply suggestions from code review
fe60975 
Co-authored-by: Mark Byrne <31762852+mbyrnepr2@users.noreply.github.com>
@clavedeluna @Pierre-Sassoulas
detect calls to quit, exit
fb3c753
@pre-commit-ci @Pierre-Sassoulas
[pre-commit.ci] auto fixes from pre-commit.com hooks
43a868e 
for more information, see https://pre-commit.ci
@clavedeluna @Pierre-Sassoulas
detect call to os._exit
89bfd8b
@pre-commit-ci @Pierre-Sassoulas
[pre-commit.ci] auto fixes from pre-commit.com hooks
2e18acd 
for more information, see https://pre-commit.ci
@clavedeluna @Pierre-Sassoulas
support qname for windows
e210ffc
@clavedeluna @Pierre-Sassoulas
use try/except and infer once
ed5dd42
@clavedeluna @Pierre-Sassoulas
handle shadowed func edge case
db4ff7c
@clavedeluna @Pierre-Sassoulas
catch some exc and use qnames
6857803
@Pierre-Sassoulas
Copy link
Member

Rebased on upstream/main so the pipeline can run properly.

@github-actions
Copy link
Contributor

🤖 According to the primer, this change has no effect on the checked open source code. 🤖🎉

This comment was generated for commit 6857803

@Pierre-Sassoulas Pierre-Sassoulas merged commit bed42ba into pylint-dev:main Nov 13, 2022
@Pierre-Sassoulas Pierre-Sassoulas changed the title Detect unreachable code after sys.exit call Detect unreachable code after sys.exit, quit, exit, and os._exit Nov 13, 2022
@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.16.0 milestone Nov 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jacobtylerwalls jacobtylerwalls jacobtylerwalls left review comments

@mbyrnepr2 mbyrnepr2 mbyrnepr2 left review comments

@DanielNoord DanielNoord DanielNoord left review comments

@Pierre-Sassoulas Pierre-Sassoulas Pierre-Sassoulas approved these changes

Assignees
No one assigned
Labels
Control flow Requires control flow understanding False Negative 🦋 No message is emitted but something is wrong with the code Needs decision 🔒 Needs a decision before implemention or rejection
Projects
None yet
Milestone
2.16.0
Development

Successfully merging this pull request may close these issues.
6 participants
@clavedeluna @coveralls @mbyrnepr2 @Pierre-Sassoulas @DanielNoord @jacobtylerwalls