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

Add convert exit() to sys.exit() rule #816

Merged
merged 6 commits into from
Nov 20, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -465,8 +465,8 @@ For more, see [pyupgrade](https://pypi.org/project/pyupgrade/3.2.0/) on PyPI.
| U010 | UnnecessaryFutureImport | Unnecessary `__future__` import `...` for target Python version | 🛠 |
| U011 | UnnecessaryLRUCacheParams | Unnecessary parameters to `functools.lru_cache` | 🛠 |
| U012 | UnnecessaryEncodeUTF8 | Unnecessary call to `encode` as UTF-8 | 🛠 |
| U013 | ConvertTypedDictFunctionalToClass | Convert `TypedDict` functional syntax to class syntax | 🛠 |
| U014 | ConvertNamedTupleFunctionalToClass | Convert `NamedTuple` functional syntax to class syntax | 🛠 |
| U013 | ConvertTypedDictFunctionalToClass | Convert `...` from `TypedDict` functional to class syntax | 🛠 |
| U014 | ConvertNamedTupleFunctionalToClass | Convert `...` from `NamedTuple` functional to class syntax | 🛠 |

### pep8-naming

Expand Down Expand Up @@ -664,6 +664,7 @@ For more, see [mccabe](https://pypi.org/project/mccabe/0.7.0/) on PyPI.
| RUF001 | AmbiguousUnicodeCharacterString | String contains ambiguous unicode character '𝐁' (did you mean 'B'?) | 🛠 |
| RUF002 | AmbiguousUnicodeCharacterDocstring | Docstring contains ambiguous unicode character '𝐁' (did you mean 'B'?) | 🛠 |
| RUF003 | AmbiguousUnicodeCharacterComment | Comment contains ambiguous unicode character '𝐁' (did you mean 'B'?) | |
| RUF101 | ConvertExitToSysExit | `exit()` is only available in the interpreter, use `sys.exit()` instead | 🛠 |

### Meta rules

Expand Down
5 changes: 5 additions & 0 deletions resources/test/fixtures/RUF101_0.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
exit(0)


def main():
exit(2)
10 changes: 10 additions & 0 deletions resources/test/fixtures/RUF101_1.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import sys

exit(0)


def main():
exit(1)


sys.exit(2)
7 changes: 7 additions & 0 deletions resources/test/fixtures/RUF101_2.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import sys as sys2

exit(0)


def main():
exit(1)
7 changes: 7 additions & 0 deletions resources/test/fixtures/RUF101_3.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
from sys import exit

exit(0)


def main():
exit(1)
7 changes: 7 additions & 0 deletions resources/test/fixtures/RUF101_4.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
from sys import exit as exit2

exit(0)


def main():
exit(1)
7 changes: 7 additions & 0 deletions resources/test/fixtures/RUF101_5.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
from sys import *

exit(0)


def main():
exit(1)
12 changes: 12 additions & 0 deletions resources/test/fixtures/RUF101_6.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
exit(0)


def exit(e):
pass


exit(1)


def main():
exit(2)
22 changes: 15 additions & 7 deletions src/check_ast.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ use crate::visibility::{module_visibility, transition_scope, Modifier, Visibilit
use crate::{
docstrings, flake8_2020, flake8_annotations, flake8_bandit, flake8_blind_except,
flake8_boolean_trap, flake8_bugbear, flake8_builtins, flake8_comprehensions, flake8_print,
flake8_tidy_imports, mccabe, pep8_naming, pycodestyle, pydocstyle, pyflakes, pyupgrade,
flake8_tidy_imports, mccabe, pep8_naming, pycodestyle, pydocstyle, pyflakes, pyupgrade, rules,
};

const GLOBAL_SCOPE_INDEX: usize = 0;
Expand Down Expand Up @@ -471,7 +471,7 @@ where
}
StmtKind::Return { .. } => {
if self.settings.enabled.contains(&CheckCode::F706) {
if let Some(index) = self.scope_stack.last().cloned() {
if let Some(&index) = self.scope_stack.last() {
if matches!(
self.scopes[index].kind,
ScopeKind::Class(_) | ScopeKind::Module
Expand Down Expand Up @@ -1527,6 +1527,11 @@ where
}
}
}

// Ruff
if self.settings.enabled.contains(&CheckCode::RUF101) {
rules::plugins::convert_exit_to_sys_exit(self, func);
}
}
ExprKind::Dict { keys, .. } => {
let check_repeated_literals = self.settings.enabled.contains(&CheckCode::F601);
Expand Down Expand Up @@ -2169,6 +2174,10 @@ impl<'a> Checker<'a> {
&self.scopes[*(self.scope_stack.last().expect("No current scope found."))]
}

pub fn current_scopes(&self) -> impl Iterator<Item = &Scope> {
self.scope_stack.iter().rev().map(|s| &self.scopes[*s])
}

pub fn current_parent(&self) -> &'a Stmt {
self.parents[*(self.parent_stack.last().expect("No parent found."))]
}
Expand All @@ -2188,7 +2197,7 @@ impl<'a> Checker<'a> {
'b: 'a,
{
if self.settings.enabled.contains(&CheckCode::F402) {
let scope = &self.scopes[*(self.scope_stack.last().expect("No current scope found."))];
let scope = self.current_scope();
if let Some(existing) = scope.values.get(&name) {
if matches!(binding.kind, BindingKind::LoopVar)
&& matches!(
Expand All @@ -2213,7 +2222,7 @@ impl<'a> Checker<'a> {

// TODO(charlie): Don't treat annotations as assignments if there is an existing
// value.
let scope = &self.scopes[*(self.scope_stack.last().expect("No current scope found."))];
let scope = self.current_scope();
let binding = match scope.values.get(&name) {
None => binding,
Some(existing) => Binding {
Expand All @@ -2229,8 +2238,7 @@ impl<'a> Checker<'a> {

fn handle_node_load(&mut self, expr: &Expr) {
if let ExprKind::Name { id, .. } = &expr.node {
let scope_id =
self.scopes[*(self.scope_stack.last().expect("No current scope found."))].id;
let scope_id = self.current_scope().id;

let mut first_iter = true;
let mut in_generator = false;
Expand Down Expand Up @@ -2385,7 +2393,7 @@ impl<'a> Checker<'a> {
return;
}

let current = &self.scopes[*(self.scope_stack.last().expect("No current scope found."))];
let current = self.current_scope();
if id == "__all__"
&& matches!(current.kind, ScopeKind::Module)
&& matches!(
Expand Down
9 changes: 9 additions & 0 deletions src/checks.rs
Original file line number Diff line number Diff line change
Expand Up @@ -249,6 +249,7 @@ pub enum CheckCode {
RUF001,
RUF002,
RUF003,
RUF101,
// Meta
M001,
}
Expand Down Expand Up @@ -583,6 +584,7 @@ pub enum CheckKind {
AmbiguousUnicodeCharacterString(char, char),
AmbiguousUnicodeCharacterDocstring(char, char),
AmbiguousUnicodeCharacterComment(char, char),
ConvertExitToSysExit,
// Meta
UnusedNOQA(Option<Vec<String>>),
}
Expand Down Expand Up @@ -872,6 +874,7 @@ impl CheckCode {
CheckCode::RUF001 => CheckKind::AmbiguousUnicodeCharacterString('𝐁', 'B'),
CheckCode::RUF002 => CheckKind::AmbiguousUnicodeCharacterDocstring('𝐁', 'B'),
CheckCode::RUF003 => CheckKind::AmbiguousUnicodeCharacterComment('𝐁', 'B'),
CheckCode::RUF101 => CheckKind::ConvertExitToSysExit,
// Meta
CheckCode::M001 => CheckKind::UnusedNOQA(None),
}
Expand Down Expand Up @@ -1082,6 +1085,7 @@ impl CheckCode {
CheckCode::RUF001 => CheckCategory::Ruff,
CheckCode::RUF002 => CheckCategory::Ruff,
CheckCode::RUF003 => CheckCategory::Ruff,
CheckCode::RUF101 => CheckCategory::Ruff,
CheckCode::M001 => CheckCategory::Meta,
}
}
Expand Down Expand Up @@ -1313,6 +1317,7 @@ impl CheckKind {
CheckKind::AmbiguousUnicodeCharacterString(..) => &CheckCode::RUF001,
CheckKind::AmbiguousUnicodeCharacterDocstring(..) => &CheckCode::RUF002,
CheckKind::AmbiguousUnicodeCharacterComment(..) => &CheckCode::RUF003,
CheckKind::ConvertExitToSysExit => &CheckCode::RUF101,
// Meta
CheckKind::UnusedNOQA(_) => &CheckCode::M001,
}
Expand Down Expand Up @@ -2002,6 +2007,9 @@ impl CheckKind {
'{representant}'?)"
)
}
CheckKind::ConvertExitToSysExit => "`exit()` is only available in the interpreter, \
use `sys.exit()` instead"
.to_string(),
// Meta
CheckKind::UnusedNOQA(codes) => match codes {
None => "Unused `noqa` directive".to_string(),
Expand Down Expand Up @@ -2048,6 +2056,7 @@ impl CheckKind {
self,
CheckKind::AmbiguousUnicodeCharacterString(..)
| CheckKind::AmbiguousUnicodeCharacterDocstring(..)
| CheckKind::ConvertExitToSysExit
| CheckKind::BlankLineAfterLastSection(..)
| CheckKind::BlankLineAfterSection(..)
| CheckKind::BlankLineAfterSummary
Expand Down
16 changes: 15 additions & 1 deletion src/checks_gen.rs
Original file line number Diff line number Diff line change
Expand Up @@ -267,6 +267,9 @@ pub enum CheckCodePrefix {
RUF001,
RUF002,
RUF003,
RUF1,
RUF10,
RUF101,
S,
S1,
S10,
Expand Down Expand Up @@ -1058,12 +1061,20 @@ impl CheckCodePrefix {
CheckCodePrefix::Q001 => vec![CheckCode::Q001],
CheckCodePrefix::Q002 => vec![CheckCode::Q002],
CheckCodePrefix::Q003 => vec![CheckCode::Q003],
CheckCodePrefix::RUF => vec![CheckCode::RUF001, CheckCode::RUF002, CheckCode::RUF003],
CheckCodePrefix::RUF => vec![
CheckCode::RUF001,
CheckCode::RUF002,
CheckCode::RUF003,
CheckCode::RUF101,
],
CheckCodePrefix::RUF0 => vec![CheckCode::RUF001, CheckCode::RUF002, CheckCode::RUF003],
CheckCodePrefix::RUF00 => vec![CheckCode::RUF001, CheckCode::RUF002, CheckCode::RUF003],
CheckCodePrefix::RUF001 => vec![CheckCode::RUF001],
CheckCodePrefix::RUF002 => vec![CheckCode::RUF002],
CheckCodePrefix::RUF003 => vec![CheckCode::RUF003],
CheckCodePrefix::RUF1 => vec![CheckCode::RUF101],
CheckCodePrefix::RUF10 => vec![CheckCode::RUF101],
CheckCodePrefix::RUF101 => vec![CheckCode::RUF101],
CheckCodePrefix::S => vec![
CheckCode::S101,
CheckCode::S102,
Expand Down Expand Up @@ -1471,6 +1482,9 @@ impl CheckCodePrefix {
CheckCodePrefix::RUF001 => PrefixSpecificity::Explicit,
CheckCodePrefix::RUF002 => PrefixSpecificity::Explicit,
CheckCodePrefix::RUF003 => PrefixSpecificity::Explicit,
CheckCodePrefix::RUF1 => PrefixSpecificity::Hundreds,
CheckCodePrefix::RUF10 => PrefixSpecificity::Tens,
CheckCodePrefix::RUF101 => PrefixSpecificity::Explicit,
CheckCodePrefix::S => PrefixSpecificity::Category,
CheckCodePrefix::S1 => PrefixSpecificity::Hundreds,
CheckCodePrefix::S10 => PrefixSpecificity::Tens,
Expand Down
3 changes: 1 addition & 2 deletions src/flake8_print/checks.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,14 @@ use crate::checks::{Check, CheckKind};

/// Check whether a function call is a `print` or `pprint` invocation
pub fn print_call(
expr: &Expr,
func: &Expr,
check_print: bool,
check_pprint: bool,
location: Range,
) -> Option<Check> {
if let ExprKind::Name { id, .. } = &func.node {
if check_print && id == "print" {
return Some(Check::new(CheckKind::PrintFound, Range::from_located(expr)));
return Some(Check::new(CheckKind::PrintFound, location));
} else if check_pprint && id == "pprint" {
return Some(Check::new(CheckKind::PPrintFound, location));
}
Expand Down
1 change: 0 additions & 1 deletion src/flake8_print/plugins/print_call.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ use crate::flake8_print::checks;
/// T201, T203
pub fn print_call(checker: &mut Checker, expr: &Expr, func: &Expr) {
if let Some(mut check) = checks::print_call(
expr,
func,
checker.settings.enabled.contains(&CheckCode::T201),
checker.settings.enabled.contains(&CheckCode::T203),
Expand Down
7 changes: 7 additions & 0 deletions src/linter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -526,6 +526,13 @@ mod tests {
#[test_case(CheckCode::RUF001, Path::new("RUF001.py"); "RUF001")]
#[test_case(CheckCode::RUF002, Path::new("RUF002.py"); "RUF002")]
#[test_case(CheckCode::RUF003, Path::new("RUF003.py"); "RUF003")]
#[test_case(CheckCode::RUF101, Path::new("RUF101_0.py"); "RUF101_0")]
#[test_case(CheckCode::RUF101, Path::new("RUF101_1.py"); "RUF101_1")]
#[test_case(CheckCode::RUF101, Path::new("RUF101_2.py"); "RUF101_2")]
#[test_case(CheckCode::RUF101, Path::new("RUF101_3.py"); "RUF101_3")]
#[test_case(CheckCode::RUF101, Path::new("RUF101_4.py"); "RUF101_4")]
#[test_case(CheckCode::RUF101, Path::new("RUF101_5.py"); "RUF101_5")]
#[test_case(CheckCode::RUF101, Path::new("RUF101_6.py"); "RUF101_6")]
#[test_case(CheckCode::YTT101, Path::new("YTT101.py"); "YTT101")]
#[test_case(CheckCode::YTT102, Path::new("YTT102.py"); "YTT102")]
#[test_case(CheckCode::YTT103, Path::new("YTT103.py"); "YTT103")]
Expand Down
1 change: 1 addition & 0 deletions src/rules/mod.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
//! Module for Ruff-specific rules.
pub mod checks;
pub mod plugins;
92 changes: 92 additions & 0 deletions src/rules/plugins/convert_exit_to_sys_exit.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
use rustpython_ast::{Expr, ExprKind};

use crate::ast::types::{BindingKind, Range};
use crate::autofix::Fix;
use crate::check_ast::Checker;
use crate::checks::{Check, CheckKind};

/// Return `true` if the `module` was imported using a star import (e.g., `from
/// sys import *`).
fn is_module_star_imported(checker: &Checker, module: &str) -> bool {
checker.current_scopes().any(|scope| {
scope.values.values().any(|binding| {
if let BindingKind::StarImportation(_, name) = &binding.kind {
name.as_ref().map(|name| name == module).unwrap_or_default()
} else {
false
}
})
})
}

/// Return `true` if `exit` is (still) bound as a built-in in the current scope.
fn has_builtin_exit_in_scope(checker: &Checker) -> bool {
!is_module_star_imported(checker, "sys")
&& checker
.current_scopes()
.find_map(|scope| scope.values.get("exit"))
.map(|binding| matches!(binding.kind, BindingKind::Builtin))
.unwrap_or_default()
}

/// Return the appropriate `sys.exit` reference based on the current set of
/// imports, or `None` is `sys.exit` hasn't been imported.
fn get_member_import_name_alias(checker: &Checker, module: &str, member: &str) -> Option<String> {
checker.current_scopes().find_map(|scope| {
scope
.values
.values()
.find_map(|binding| match &binding.kind {
// e.g. module=sys object=exit
// `import sys` -> `sys.exit`
// `import sys as sys2` -> `sys2.exit`
BindingKind::Importation(name, full_name, _) if full_name == module => {
Some(format!("{}.{}", name, member))
}
// e.g. module=os.path object=join
// `from os.path import join` -> `join`
// `from os.path import join as join2` -> `join2`
BindingKind::FromImportation(name, full_name, _)
if full_name == &format!("{}.{}", module, member) =>
{
Some(name.to_string())
}
// e.g. module=os.path object=join
// `from os.path import *` -> `join`
BindingKind::StarImportation(_, name)
if name.as_ref().map(|name| name == module).unwrap_or_default() =>
{
Some(member.to_string())
}
// e.g. module=os.path object=join
// `import os.path ` -> `os.path.join`
BindingKind::SubmoduleImportation(_, full_name, _) if full_name == module => {
Some(format!("{}.{}", full_name, member))
}
// Non-imports.
_ => None,
})
})
}

/// RUF101
pub fn convert_exit_to_sys_exit(checker: &mut Checker, func: &Expr) {
if let ExprKind::Name { id, .. } = &func.node {
if id == "exit" {
if has_builtin_exit_in_scope(checker) {
let mut check =
Check::new(CheckKind::ConvertExitToSysExit, Range::from_located(func));
if checker.patch(check.kind.code()) {
if let Some(content) = get_member_import_name_alias(checker, "sys", "exit") {
check.amend(Fix::replacement(
content,
func.location,
func.end_location.unwrap(),
))
}
}
checker.add_check(check);
}
}
}
}
3 changes: 3 additions & 0 deletions src/rules/plugins/mod.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
pub use convert_exit_to_sys_exit::convert_exit_to_sys_exit;

mod convert_exit_to_sys_exit;
Loading