diff --git a/README.md b/README.md index 39fde993a1b6e..0f185e191e0ac 100644 --- a/README.md +++ b/README.md @@ -1194,6 +1194,7 @@ For more, see [tryceratops](https://pypi.org/project/tryceratops/1.1.0/) on PyPI | Code | Name | Message | Fix | | ---- | ---- | ------- | --- | +| TRY004 | prefer-type-error | Prefer `TypeError` exception for invalid type | | | TRY300 | try-consider-else | Consider `else` block | | ### Ruff-specific rules (RUF) diff --git a/resources/test/fixtures/tryceratops/TRY004.py b/resources/test/fixtures/tryceratops/TRY004.py new file mode 100644 index 0000000000000..e5e28a1316465 --- /dev/null +++ b/resources/test/fixtures/tryceratops/TRY004.py @@ -0,0 +1,296 @@ +""" +Violation: + +Prefer TypeError when relevant. +""" + + +def incorrect_basic(some_arg): + if isinstance(some_arg, int): + pass + else: + raise Exception("...") # should be typeerror + + +def incorrect_multiple_type_check(some_arg): + if isinstance(some_arg, (int, str)): + pass + else: + raise Exception("...") # should be typeerror + + +class MyClass: + pass + + +def incorrect_with_issubclass(some_arg): + if issubclass(some_arg, MyClass): + pass + else: + raise Exception("...") # should be typeerror + + +def incorrect_with_callable(some_arg): + if callable(some_arg): + pass + else: + raise Exception("...") # should be typeerror + + +def incorrect_ArithmeticError(some_arg): + if isinstance(some_arg, int): + pass + else: + raise ArithmeticError("...") # should be typeerror + + +def incorrect_AssertionError(some_arg): + if isinstance(some_arg, int): + pass + else: + raise AssertionError("...") # should be typeerror + + +def incorrect_AttributeError(some_arg): + if isinstance(some_arg, int): + pass + else: + raise AttributeError("...") # should be typeerror + + +def incorrect_BufferError(some_arg): + if isinstance(some_arg, int): + pass + else: + raise BufferError # should be typeerror + + +def incorrect_EOFError(some_arg): + if isinstance(some_arg, int): + pass + else: + raise EOFError("...") # should be typeerror + + +def incorrect_ImportError(some_arg): + if isinstance(some_arg, int): + pass + else: + raise ImportError("...") # should be typeerror + + +def incorrect_LookupError(some_arg): + if isinstance(some_arg, int): + pass + else: + raise LookupError("...") # should be typeerror + + +def incorrect_MemoryError(some_arg): + if isinstance(some_arg, int): + pass + else: + # should be typeerror + # not multiline is on purpose for fix + raise MemoryError( + "..." + ) + + +def incorrect_NameError(some_arg): + if isinstance(some_arg, int): + pass + else: + raise NameError("...") # should be typeerror + + +def incorrect_ReferenceError(some_arg): + if isinstance(some_arg, int): + pass + else: + raise ReferenceError("...") # should be typeerror + + +def incorrect_RuntimeError(some_arg): + if isinstance(some_arg, int): + pass + else: + raise RuntimeError("...") # should be typeerror + + +def incorrect_SyntaxError(some_arg): + if isinstance(some_arg, int): + pass + else: + raise SyntaxError("...") # should be typeerror + + +def incorrect_SystemError(some_arg): + if isinstance(some_arg, int): + pass + else: + raise SystemError("...") # should be typeerror + + +def incorrect_ValueError(some_arg): + if isinstance(some_arg, int): + pass + else: + raise ValueError("...") # should be typeerror + + +def incorrect_not_operator_isinstance(some_arg): + if not isinstance(some_arg): + pass + else: + raise Exception("...") # should be typeerror + + +def incorrect_and_operator_isinstance(arg1, arg2): + if isinstance(some_arg) and isinstance(arg2): + pass + else: + raise Exception("...") # should be typeerror + + +def incorrect_or_operator_isinstance(arg1, arg2): + if isinstance(some_arg) or isinstance(arg2): + pass + else: + raise Exception("...") # should be typeerror + + +def incorrect_multiple_operators_isinstance(arg1, arg2, arg3): + if not isinstance(arg1) and isinstance(arg2) or isinstance(arg3): + pass + else: + raise Exception("...") # should be typeerror + + +def incorrect_not_operator_callable(some_arg): + if not callable(some_arg): + pass + else: + raise Exception("...") # should be typeerror + + +def incorrect_and_operator_callable(arg1, arg2): + if callable(some_arg) and callable(arg2): + pass + else: + raise Exception("...") # should be typeerror + + +def incorrect_or_operator_callable(arg1, arg2): + if callable(some_arg) or callable(arg2): + pass + else: + raise Exception("...") # should be typeerror + + +def incorrect_multiple_operators_callable(arg1, arg2, arg3): + if not callable(arg1) and callable(arg2) or callable(arg3): + pass + else: + raise Exception("...") # should be typeerror + + +def incorrect_not_operator_issubclass(some_arg): + if not issubclass(some_arg): + pass + else: + raise Exception("...") # should be typeerror + + +def incorrect_and_operator_issubclass(arg1, arg2): + if issubclass(some_arg) and issubclass(arg2): + pass + else: + raise Exception("...") # should be typeerror + + +def incorrect_or_operator_issubclass(arg1, arg2): + if issubclass(some_arg) or issubclass(arg2): + pass + else: + raise Exception("...") # should be typeerror + + +def incorrect_multiple_operators_issubclass(arg1, arg2, arg3): + if not issubclass(arg1) and issubclass(arg2) or issubclass(arg3): + pass + else: + raise Exception("...") # should be typeerror + + +def incorrect_multi_conditional(arg1, arg2): + if isinstance(arg1, int): + pass + elif isinstance(arg2, int): + raise Exception("...") # should be typeerror + + +class MyCustomTypeValidation(Exception): + pass + + +def correct_custom_exception(some_arg): + if isinstance(some_arg, int): + pass + else: + raise MyCustomTypeValidation("...") # that's correct, because it's not vanilla + + +def correct_complex_conditional(val): + if val is not None and (not isinstance(val, int) or val < 0): + raise ValueError(...) # fine if this is not a TypeError + + +def correct_multi_conditional(some_arg): + if some_arg == 3: + pass + elif isinstance(some_arg, int): + pass + else: + raise Exception("...") # fine if this is not a TypeError + + +def correct_should_ignore(some_arg): + if isinstance(some_arg, int): + pass + else: + raise TypeError("...") + + +def check_body(some_args): + if isinstance(some_args, int): + raise ValueError("...") # should be typeerror + + +def check_body_correct(some_args): + if isinstance(some_args, int): + raise TypeError("...") # correct + + +def multiple_elifs(some_args): + if not isinstance(some_args, int): + raise ValueError("...") # should be typerror + elif some_args < 3: + raise ValueError("...") # this is ok + elif some_args > 10: + raise ValueError("...") # this is ok if we don't simplify + else: + pass + + +def multiple_ifs(some_args): + if not isinstance(some_args, int): + raise ValueError("...") # should be typerror + else: + if some_args < 3: + raise ValueError("...") # this is ok + else: + if some_args > 10: + raise ValueError("...") # this is ok if we don't simplify + else: + pass diff --git a/ruff.schema.json b/ruff.schema.json index 9e7210998dd88..d32b6a3138224 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -1748,6 +1748,9 @@ "TID251", "TID252", "TRY", + "TRY0", + "TRY00", + "TRY004", "TRY3", "TRY30", "TRY300", diff --git a/src/checkers/ast.rs b/src/checkers/ast.rs index 8cf8e9ec0c3e5..66b6bc7519467 100644 --- a/src/checkers/ast.rs +++ b/src/checkers/ast.rs @@ -1391,6 +1391,15 @@ where self.current_stmt_parent().map(std::convert::Into::into), ); } + if self.settings.rules.enabled(&Rule::PreferTypeError) { + tryceratops::rules::prefer_type_error( + self, + body, + test, + orelse, + self.current_stmt_parent().map(Into::into), + ); + } } StmtKind::Assert { test, msg } => { if self.settings.rules.enabled(&Rule::AssertTuple) { diff --git a/src/registry.rs b/src/registry.rs index 3f1f9160418a6..673e79c56551c 100644 --- a/src/registry.rs +++ b/src/registry.rs @@ -428,6 +428,7 @@ ruff_macros::define_rule_mapping!( // flake8-type-checking TYP005 => rules::flake8_type_checking::rules::EmptyTypeCheckingBlock, // tryceratops + TRY004 => rules::tryceratops::rules::PreferTypeError, TRY300 => rules::tryceratops::rules::TryConsiderElse, // Ruff RUF001 => violations::AmbiguousUnicodeCharacterString, diff --git a/src/rules/tryceratops/mod.rs b/src/rules/tryceratops/mod.rs index 4d9ccd8e3b344..9d2738ff0a958 100644 --- a/src/rules/tryceratops/mod.rs +++ b/src/rules/tryceratops/mod.rs @@ -13,6 +13,7 @@ mod tests { use crate::registry::Rule; use crate::settings; + #[test_case(Rule::PreferTypeError, Path::new("TRY004.py"); "TRY004")] #[test_case(Rule::TryConsiderElse, Path::new("TRY300.py"); "TRY300")] fn rules(rule_code: Rule, path: &Path) -> Result<()> { let snapshot = format!("{}_{}", rule_code.as_ref(), path.to_string_lossy()); diff --git a/src/rules/tryceratops/rules/mod.rs b/src/rules/tryceratops/rules/mod.rs index 9d8f57c56a991..5fd6a64cdfc32 100644 --- a/src/rules/tryceratops/rules/mod.rs +++ b/src/rules/tryceratops/rules/mod.rs @@ -1,3 +1,5 @@ +pub use prefer_type_error::{prefer_type_error, PreferTypeError}; pub use try_consider_else::{try_consider_else, TryConsiderElse}; +mod prefer_type_error; mod try_consider_else; diff --git a/src/rules/tryceratops/rules/prefer_type_error.rs b/src/rules/tryceratops/rules/prefer_type_error.rs new file mode 100644 index 0000000000000..5d406aa00979b --- /dev/null +++ b/src/rules/tryceratops/rules/prefer_type_error.rs @@ -0,0 +1,162 @@ +use ruff_macros::derive_message_formats; +use rustpython_ast::{Expr, ExprKind, Stmt, StmtKind}; + +use crate::ast::types::Range; +use crate::checkers::ast::Checker; +use crate::define_violation; +use crate::fix::Fix; +use crate::registry::Diagnostic; +use crate::violation::Violation; + +define_violation!( + pub struct PreferTypeError; +); +impl Violation for PreferTypeError { + #[derive_message_formats] + fn message(&self) -> String { + format!("Prefer `TypeError` exception for invalid type") + } +} + +/// Returns `true` if an [`Expr`] is a call to check types. +fn check_type_check_call(checker: &mut Checker, call: &Expr) -> bool { + checker.resolve_call_path(call).map_or(false, |call_path| { + call_path.as_slice() == ["", "isinstance"] + || call_path.as_slice() == ["", "issubclass"] + || call_path.as_slice() == ["", "callable"] + }) +} + +/// Returns `true` if an [`Expr`] is a test to check types (e.g. via isinstance) +fn check_type_check_test(checker: &mut Checker, test: &Expr) -> bool { + match &test.node { + ExprKind::BoolOp { values, .. } => values + .iter() + .all(|expr| check_type_check_test(checker, expr)), + ExprKind::UnaryOp { operand, .. } => check_type_check_test(checker, operand), + ExprKind::Call { .. } => check_type_check_call(checker, test), + _ => false, + } +} + +/// Returns the [`Expr`] representing the name of the exception. +fn match_name(exc: &Expr) -> Option<&Expr> { + match &exc.node { + ExprKind::Name { .. } => Some(exc), + ExprKind::Call { func, .. } => { + if let ExprKind::Name { .. } = &func.node { + Some(func) + } else { + None + } + } + _ => None, + } +} + +/// Returns `true` if `exc` is a reference to a builtin exception. +fn is_builtin_exception(checker: &mut Checker, exc: &Expr) -> bool { + return checker.resolve_call_path(exc).map_or(false, |call_path| { + [ + "ArithmeticError", + "AssertionError", + "AttributeError", + "BufferError", + "EOFError", + "Exception", + "ImportError", + "LookupError", + "MemoryError", + "NameError", + "ReferenceError", + "RuntimeError", + "SyntaxError", + "SystemError", + "ValueError", + ] + .iter() + .any(|target| call_path.as_slice() == ["", target]) + }); +} + +/// Returns `true` if an [`Expr`] is a reference to a builtin exception. +fn check_raise_type(checker: &mut Checker, exc: &Expr) -> bool { + match &exc.node { + ExprKind::Name { .. } => is_builtin_exception(checker, exc), + ExprKind::Call { func, .. } => { + if let ExprKind::Name { .. } = &func.node { + is_builtin_exception(checker, func) + } else { + false + } + } + _ => false, + } +} + +fn check_raise(checker: &mut Checker, exc: &Expr, item: &Stmt) { + if check_raise_type(checker, exc) { + let mut diagnostic = Diagnostic::new(PreferTypeError, Range::from_located(item)); + + if checker.patch(diagnostic.kind.rule()) { + if let Some(name) = match_name(exc) { + diagnostic.amend(Fix::replacement( + "TypeError".to_string(), + name.location, + name.end_location.unwrap(), + )); + } + } + + checker.diagnostics.push(diagnostic); + } +} + +/// Search the body of an if-condition for raises. +fn check_body(checker: &mut Checker, func: &[Stmt]) { + for item in func { + if let StmtKind::Raise { exc: Some(exc), .. } = &item.node { + check_raise(checker, exc, item); + } + } +} + +/// Search the orelse of an if-condition for raises. +fn check_orelse(checker: &mut Checker, func: &[Stmt]) { + for item in func { + match &item.node { + StmtKind::If { test, .. } => { + if !check_type_check_test(checker, test) { + return; + } + } + StmtKind::Raise { exc: Some(exc), .. } => { + check_raise(checker, exc, item); + } + _ => {} + } + } +} + +/// TRY004 +pub fn prefer_type_error( + checker: &mut Checker, + body: &[Stmt], + test: &Expr, + orelse: &[Stmt], + parent: Option<&Stmt>, +) { + if let Some(parent) = parent { + if let StmtKind::If { test, .. } = &parent.node { + if !check_type_check_test(checker, test) { + return; + } + } + } + + // Only consider the body when the `if` condition is all type-related + if check_type_check_test(checker, test) { + check_body(checker, body); + check_orelse(checker, orelse); + } +} diff --git a/src/rules/tryceratops/snapshots/ruff__rules__tryceratops__tests__prefer-type-error_TRY004.py.snap b/src/rules/tryceratops/snapshots/ruff__rules__tryceratops__tests__prefer-type-error_TRY004.py.snap new file mode 100644 index 0000000000000..8c1921bab9b80 --- /dev/null +++ b/src/rules/tryceratops/snapshots/ruff__rules__tryceratops__tests__prefer-type-error_TRY004.py.snap @@ -0,0 +1,583 @@ +--- +source: src/rules/tryceratops/mod.rs +expression: diagnostics +--- +- kind: + PreferTypeError: ~ + location: + row: 12 + column: 8 + end_location: + row: 12 + column: 30 + fix: + content: TypeError + location: + row: 12 + column: 14 + end_location: + row: 12 + column: 23 + parent: ~ +- kind: + PreferTypeError: ~ + location: + row: 19 + column: 8 + end_location: + row: 19 + column: 30 + fix: + content: TypeError + location: + row: 19 + column: 14 + end_location: + row: 19 + column: 23 + parent: ~ +- kind: + PreferTypeError: ~ + location: + row: 30 + column: 8 + end_location: + row: 30 + column: 30 + fix: + content: TypeError + location: + row: 30 + column: 14 + end_location: + row: 30 + column: 23 + parent: ~ +- kind: + PreferTypeError: ~ + location: + row: 37 + column: 8 + end_location: + row: 37 + column: 30 + fix: + content: TypeError + location: + row: 37 + column: 14 + end_location: + row: 37 + column: 23 + parent: ~ +- kind: + PreferTypeError: ~ + location: + row: 44 + column: 8 + end_location: + row: 44 + column: 36 + fix: + content: TypeError + location: + row: 44 + column: 14 + end_location: + row: 44 + column: 29 + parent: ~ +- kind: + PreferTypeError: ~ + location: + row: 51 + column: 8 + end_location: + row: 51 + column: 35 + fix: + content: TypeError + location: + row: 51 + column: 14 + end_location: + row: 51 + column: 28 + parent: ~ +- kind: + PreferTypeError: ~ + location: + row: 58 + column: 8 + end_location: + row: 58 + column: 35 + fix: + content: TypeError + location: + row: 58 + column: 14 + end_location: + row: 58 + column: 28 + parent: ~ +- kind: + PreferTypeError: ~ + location: + row: 65 + column: 8 + end_location: + row: 65 + column: 25 + fix: + content: TypeError + location: + row: 65 + column: 14 + end_location: + row: 65 + column: 25 + parent: ~ +- kind: + PreferTypeError: ~ + location: + row: 72 + column: 8 + end_location: + row: 72 + column: 29 + fix: + content: TypeError + location: + row: 72 + column: 14 + end_location: + row: 72 + column: 22 + parent: ~ +- kind: + PreferTypeError: ~ + location: + row: 79 + column: 8 + end_location: + row: 79 + column: 32 + fix: + content: TypeError + location: + row: 79 + column: 14 + end_location: + row: 79 + column: 25 + parent: ~ +- kind: + PreferTypeError: ~ + location: + row: 86 + column: 8 + end_location: + row: 86 + column: 32 + fix: + content: TypeError + location: + row: 86 + column: 14 + end_location: + row: 86 + column: 25 + parent: ~ +- kind: + PreferTypeError: ~ + location: + row: 95 + column: 8 + end_location: + row: 97 + column: 9 + fix: + content: TypeError + location: + row: 95 + column: 14 + end_location: + row: 95 + column: 25 + parent: ~ +- kind: + PreferTypeError: ~ + location: + row: 104 + column: 8 + end_location: + row: 104 + column: 30 + fix: + content: TypeError + location: + row: 104 + column: 14 + end_location: + row: 104 + column: 23 + parent: ~ +- kind: + PreferTypeError: ~ + location: + row: 111 + column: 8 + end_location: + row: 111 + column: 35 + fix: + content: TypeError + location: + row: 111 + column: 14 + end_location: + row: 111 + column: 28 + parent: ~ +- kind: + PreferTypeError: ~ + location: + row: 118 + column: 8 + end_location: + row: 118 + column: 33 + fix: + content: TypeError + location: + row: 118 + column: 14 + end_location: + row: 118 + column: 26 + parent: ~ +- kind: + PreferTypeError: ~ + location: + row: 125 + column: 8 + end_location: + row: 125 + column: 32 + fix: + content: TypeError + location: + row: 125 + column: 14 + end_location: + row: 125 + column: 25 + parent: ~ +- kind: + PreferTypeError: ~ + location: + row: 132 + column: 8 + end_location: + row: 132 + column: 32 + fix: + content: TypeError + location: + row: 132 + column: 14 + end_location: + row: 132 + column: 25 + parent: ~ +- kind: + PreferTypeError: ~ + location: + row: 139 + column: 8 + end_location: + row: 139 + column: 31 + fix: + content: TypeError + location: + row: 139 + column: 14 + end_location: + row: 139 + column: 24 + parent: ~ +- kind: + PreferTypeError: ~ + location: + row: 146 + column: 8 + end_location: + row: 146 + column: 30 + fix: + content: TypeError + location: + row: 146 + column: 14 + end_location: + row: 146 + column: 23 + parent: ~ +- kind: + PreferTypeError: ~ + location: + row: 153 + column: 8 + end_location: + row: 153 + column: 30 + fix: + content: TypeError + location: + row: 153 + column: 14 + end_location: + row: 153 + column: 23 + parent: ~ +- kind: + PreferTypeError: ~ + location: + row: 160 + column: 8 + end_location: + row: 160 + column: 30 + fix: + content: TypeError + location: + row: 160 + column: 14 + end_location: + row: 160 + column: 23 + parent: ~ +- kind: + PreferTypeError: ~ + location: + row: 167 + column: 8 + end_location: + row: 167 + column: 30 + fix: + content: TypeError + location: + row: 167 + column: 14 + end_location: + row: 167 + column: 23 + parent: ~ +- kind: + PreferTypeError: ~ + location: + row: 174 + column: 8 + end_location: + row: 174 + column: 30 + fix: + content: TypeError + location: + row: 174 + column: 14 + end_location: + row: 174 + column: 23 + parent: ~ +- kind: + PreferTypeError: ~ + location: + row: 181 + column: 8 + end_location: + row: 181 + column: 30 + fix: + content: TypeError + location: + row: 181 + column: 14 + end_location: + row: 181 + column: 23 + parent: ~ +- kind: + PreferTypeError: ~ + location: + row: 188 + column: 8 + end_location: + row: 188 + column: 30 + fix: + content: TypeError + location: + row: 188 + column: 14 + end_location: + row: 188 + column: 23 + parent: ~ +- kind: + PreferTypeError: ~ + location: + row: 195 + column: 8 + end_location: + row: 195 + column: 30 + fix: + content: TypeError + location: + row: 195 + column: 14 + end_location: + row: 195 + column: 23 + parent: ~ +- kind: + PreferTypeError: ~ + location: + row: 202 + column: 8 + end_location: + row: 202 + column: 30 + fix: + content: TypeError + location: + row: 202 + column: 14 + end_location: + row: 202 + column: 23 + parent: ~ +- kind: + PreferTypeError: ~ + location: + row: 209 + column: 8 + end_location: + row: 209 + column: 30 + fix: + content: TypeError + location: + row: 209 + column: 14 + end_location: + row: 209 + column: 23 + parent: ~ +- kind: + PreferTypeError: ~ + location: + row: 216 + column: 8 + end_location: + row: 216 + column: 30 + fix: + content: TypeError + location: + row: 216 + column: 14 + end_location: + row: 216 + column: 23 + parent: ~ +- kind: + PreferTypeError: ~ + location: + row: 223 + column: 8 + end_location: + row: 223 + column: 30 + fix: + content: TypeError + location: + row: 223 + column: 14 + end_location: + row: 223 + column: 23 + parent: ~ +- kind: + PreferTypeError: ~ + location: + row: 230 + column: 8 + end_location: + row: 230 + column: 30 + fix: + content: TypeError + location: + row: 230 + column: 14 + end_location: + row: 230 + column: 23 + parent: ~ +- kind: + PreferTypeError: ~ + location: + row: 267 + column: 8 + end_location: + row: 267 + column: 31 + fix: + content: TypeError + location: + row: 267 + column: 14 + end_location: + row: 267 + column: 24 + parent: ~ +- kind: + PreferTypeError: ~ + location: + row: 277 + column: 8 + end_location: + row: 277 + column: 31 + fix: + content: TypeError + location: + row: 277 + column: 14 + end_location: + row: 277 + column: 24 + parent: ~ +- kind: + PreferTypeError: ~ + location: + row: 288 + column: 8 + end_location: + row: 288 + column: 31 + fix: + content: TypeError + location: + row: 288 + column: 14 + end_location: + row: 288 + column: 24 + parent: ~ +