diff --git a/crates/ruff/resources/test/fixtures/ruff/RUF013_0.py b/crates/ruff/resources/test/fixtures/ruff/RUF013_0.py index 371c57da11..21c5908979 100644 --- a/crates/ruff/resources/test/fixtures/ruff/RUF013_0.py +++ b/crates/ruff/resources/test/fixtures/ruff/RUF013_0.py @@ -221,3 +221,23 @@ def f(arg: Union["No" "ne", "int"] = None): # Avoid flagging when there's a parse error in the forward reference def f(arg: Union["<>", "int"] = None): pass + + +# Type aliases + +Text = str | bytes + + +def f(arg: Text = None): + pass + + +def f(arg: "Text" = None): + pass + + +from custom_typing import MaybeInt + + +def f(arg: MaybeInt = None): + pass diff --git a/crates/ruff/src/rules/ruff/rules/implicit_optional.rs b/crates/ruff/src/rules/ruff/rules/implicit_optional.rs index 459d7eb451..2ceb04365d 100644 --- a/crates/ruff/src/rules/ruff/rules/implicit_optional.rs +++ b/crates/ruff/src/rules/ruff/rules/implicit_optional.rs @@ -6,10 +6,12 @@ use rustpython_parser::ast::{self, ArgWithDefault, Arguments, Constant, Expr, Op use ruff_diagnostics::{AutofixKind, Diagnostic, Edit, Fix, Violation}; use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast::call_path::CallPath; use ruff_python_ast::helpers::is_const_none; use ruff_python_ast::source_code::Locator; use ruff_python_ast::typing::parse_type_annotation; use ruff_python_semantic::SemanticModel; +use ruff_python_stdlib::sys::is_known_standard_library; use crate::checkers::ast::Checker; use crate::importer::ImportRequest; @@ -58,6 +60,18 @@ use crate::settings::types::PythonVersion; /// pass /// ``` /// +/// ## Limitations +/// +/// Type aliases are not supported and could result in false negatives. +/// For example, the following code will not be flagged: +/// ```python +/// Text = str | bytes +/// +/// +/// def foo(arg: Text = None): +/// pass +/// ``` +/// /// ## Options /// - `target-version` /// @@ -141,6 +155,18 @@ impl<'a> Iterator for PEP604UnionIterator<'a> { } } +/// Returns `true` if the given call path is a known type. +/// +/// A known type is either a builtin type, any object from the standard library, +/// or a type from the `typing_extensions` module. +fn is_known_type(call_path: &CallPath, target_version: PythonVersion) -> bool { + match call_path.as_slice() { + ["" | "typing_extensions", ..] => true, + [module, ..] => is_known_standard_library(target_version.minor(), module), + _ => false, + } +} + #[derive(Debug)] enum TypingTarget<'a> { None, @@ -154,7 +180,12 @@ enum TypingTarget<'a> { } impl<'a> TypingTarget<'a> { - fn try_from_expr(expr: &'a Expr, semantic: &SemanticModel, locator: &Locator) -> Option { + fn try_from_expr( + expr: &'a Expr, + semantic: &SemanticModel, + locator: &Locator, + target_version: PythonVersion, + ) -> Option { match expr { Expr::Subscript(ast::ExprSubscript { value, slice, .. }) => { if semantic.match_typing_expr(value, "Optional") { @@ -170,7 +201,20 @@ impl<'a> TypingTarget<'a> { } else if semantic.match_typing_expr(value, "Annotated") { elements.first().map(TypingTarget::Annotated) } else { - None + semantic.resolve_call_path(value).map_or( + // If we can't resolve the call path, it must be defined + // in the same file, so we assume it's `Any` as it could + // be a type alias. + Some(TypingTarget::Any), + |call_path| { + if is_known_type(&call_path, target_version) { + None + } else { + // If it's not a known type, we assume it's `Any`. + Some(TypingTarget::Any) + } + }, + ) } } Expr::BinOp(..) => Some(TypingTarget::Union( @@ -189,54 +233,67 @@ impl<'a> TypingTarget<'a> { .map_or(Some(TypingTarget::Any), |(expr, _)| { Some(TypingTarget::ForwardReference(expr)) }), - _ => semantic.resolve_call_path(expr).and_then(|call_path| { - if semantic.match_typing_call_path(&call_path, "Any") { - Some(TypingTarget::Any) - } else if matches!(call_path.as_slice(), ["" | "builtins", "object"]) { - Some(TypingTarget::Object) - } else { - None - } - }), + _ => semantic.resolve_call_path(expr).map_or( + // If we can't resolve the call path, it must be defined in the + // same file, so we assume it's `Any` as it could be a type alias. + Some(TypingTarget::Any), + |call_path| { + if semantic.match_typing_call_path(&call_path, "Any") { + Some(TypingTarget::Any) + } else if matches!(call_path.as_slice(), ["" | "builtins", "object"]) { + Some(TypingTarget::Object) + } else if !is_known_type(&call_path, target_version) { + // If it's not a known type, we assume it's `Any`. + Some(TypingTarget::Any) + } else { + None + } + }, + ), } } /// Check if the [`TypingTarget`] explicitly allows `None`. - fn contains_none(&self, semantic: &SemanticModel, locator: &Locator) -> bool { + fn contains_none( + &self, + semantic: &SemanticModel, + locator: &Locator, + target_version: PythonVersion, + ) -> bool { match self { TypingTarget::None | TypingTarget::Optional | TypingTarget::Any | TypingTarget::Object => true, TypingTarget::Literal(elements) => elements.iter().any(|element| { - let Some(new_target) = TypingTarget::try_from_expr(element, semantic, locator) else { + let Some(new_target) = TypingTarget::try_from_expr(element, semantic, locator, target_version) else { return false; }; // Literal can only contain `None`, a literal value, other `Literal` // or an enum value. match new_target { TypingTarget::None => true, - TypingTarget::Literal(_) => new_target.contains_none(semantic, locator), + TypingTarget::Literal(_) => new_target.contains_none(semantic, locator, target_version), _ => false, } }), TypingTarget::Union(elements) => elements.iter().any(|element| { - let Some(new_target) = TypingTarget::try_from_expr(element, semantic, locator) else { + let Some(new_target) = TypingTarget::try_from_expr(element, semantic, locator, target_version) else { return false; }; - new_target.contains_none(semantic, locator) + new_target.contains_none(semantic, locator, target_version) }), TypingTarget::Annotated(element) => { - let Some(new_target) = TypingTarget::try_from_expr(element, semantic, locator) else { + let Some(new_target) = TypingTarget::try_from_expr(element, semantic, locator, target_version) else { return false; }; - new_target.contains_none(semantic, locator) + new_target.contains_none(semantic, locator, target_version) } TypingTarget::ForwardReference(expr) => { - let Some(new_target) = TypingTarget::try_from_expr(expr, semantic, locator) else { + let Some(new_target) = TypingTarget::try_from_expr(expr, semantic, locator, target_version) else { return false; }; - new_target.contains_none(semantic, locator) + new_target.contains_none(semantic, locator, target_version) } } } @@ -253,8 +310,9 @@ fn type_hint_explicitly_allows_none<'a>( annotation: &'a Expr, semantic: &SemanticModel, locator: &Locator, + target_version: PythonVersion, ) -> Option<&'a Expr> { - let Some(target) = TypingTarget::try_from_expr(annotation, semantic, locator) else { + let Some(target) = TypingTarget::try_from_expr(annotation, semantic, locator, target_version) else { return Some(annotation); }; match target { @@ -264,9 +322,11 @@ fn type_hint_explicitly_allows_none<'a>( // return the inner type if it doesn't allow `None`. If `Annotated` // is found nested inside another type, then the outer type should // be returned. - TypingTarget::Annotated(expr) => type_hint_explicitly_allows_none(expr, semantic, locator), + TypingTarget::Annotated(expr) => { + type_hint_explicitly_allows_none(expr, semantic, locator, target_version) + } _ => { - if target.contains_none(semantic, locator) { + if target.contains_none(semantic, locator, target_version) { None } else { Some(annotation) @@ -350,7 +410,7 @@ pub(crate) fn implicit_optional(checker: &mut Checker, arguments: &Arguments) { { // Quoted annotation. if let Ok((annotation, kind)) = parse_type_annotation(string, *range, checker.locator) { - let Some(expr) = type_hint_explicitly_allows_none(&annotation, checker.semantic(), checker.locator) else { + let Some(expr) = type_hint_explicitly_allows_none(&annotation, checker.semantic(), checker.locator, checker.settings.target_version) else { continue; }; let conversion_type = checker.settings.target_version.into(); @@ -366,7 +426,7 @@ pub(crate) fn implicit_optional(checker: &mut Checker, arguments: &Arguments) { } } else { // Unquoted annotation. - let Some(expr) = type_hint_explicitly_allows_none(annotation, checker.semantic(), checker.locator) else { + let Some(expr) = type_hint_explicitly_allows_none(annotation, checker.semantic(), checker.locator, checker.settings.target_version) else { continue; }; let conversion_type = checker.settings.target_version.into(); @@ -380,3 +440,40 @@ pub(crate) fn implicit_optional(checker: &mut Checker, arguments: &Arguments) { } } } + +#[cfg(test)] +mod tests { + use ruff_python_ast::call_path::CallPath; + + use crate::settings::types::PythonVersion; + + use super::is_known_type; + + #[test] + fn test_is_known_type() { + assert!(is_known_type( + &CallPath::from_slice(&["", "int"]), + PythonVersion::Py311 + )); + assert!(is_known_type( + &CallPath::from_slice(&["builtins", "int"]), + PythonVersion::Py311 + )); + assert!(is_known_type( + &CallPath::from_slice(&["typing", "Optional"]), + PythonVersion::Py311 + )); + assert!(is_known_type( + &CallPath::from_slice(&["typing_extensions", "Literal"]), + PythonVersion::Py311 + )); + assert!(is_known_type( + &CallPath::from_slice(&["zoneinfo", "ZoneInfo"]), + PythonVersion::Py311 + )); + assert!(!is_known_type( + &CallPath::from_slice(&["zoneinfo", "ZoneInfo"]), + PythonVersion::Py38 + )); + } +}