diff --git a/crates/ruff_linter/resources/test/fixtures/ruff/RUF013_4.py b/crates/ruff_linter/resources/test/fixtures/ruff/RUF013_4.py new file mode 100644 index 0000000000000..c6038a9531d2d --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/ruff/RUF013_4.py @@ -0,0 +1,21 @@ +# https://github.com/astral-sh/ruff/issues/13833 + +from typing import Optional + + +def no_default(arg: Optional): ... + + +def has_default(arg: Optional = None): ... + + +def multiple_1(arg1: Optional, arg2: Optional = None): ... + + +def multiple_2(arg1: Optional, arg2: Optional = None, arg3: int = None): ... + + +def return_type(arg: Optional = None) -> Optional: ... + + +def has_type_argument(arg: Optional[int] = None): ... diff --git a/crates/ruff_linter/src/rules/ruff/mod.rs b/crates/ruff_linter/src/rules/ruff/mod.rs index 84b7fd0d8ff7d..8b02d708e7efd 100644 --- a/crates/ruff_linter/src/rules/ruff/mod.rs +++ b/crates/ruff_linter/src/rules/ruff/mod.rs @@ -32,6 +32,7 @@ mod tests { #[test_case(Rule::ImplicitOptional, Path::new("RUF013_1.py"))] #[test_case(Rule::ImplicitOptional, Path::new("RUF013_2.py"))] #[test_case(Rule::ImplicitOptional, Path::new("RUF013_3.py"))] + #[test_case(Rule::ImplicitOptional, Path::new("RUF013_4.py"))] #[test_case(Rule::MutableClassDefault, Path::new("RUF012.py"))] #[test_case(Rule::MutableDataclassDefault, Path::new("RUF008.py"))] #[test_case(Rule::ZipInsteadOfPairwise, Path::new("RUF007.py"))] diff --git a/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF013_RUF013_4.py.snap b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF013_RUF013_4.py.snap new file mode 100644 index 0000000000000..1e4ea1f4eeff1 --- /dev/null +++ b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF013_RUF013_4.py.snap @@ -0,0 +1,20 @@ +--- +source: crates/ruff_linter/src/rules/ruff/mod.rs +snapshot_kind: text +--- +RUF013_4.py:15:61: RUF013 [*] PEP 484 prohibits implicit `Optional` + | +15 | def multiple_2(arg1: Optional, arg2: Optional = None, arg3: int = None): ... + | ^^^ RUF013 + | + = help: Convert to `T | None` + +ℹ Unsafe fix +12 12 | def multiple_1(arg1: Optional, arg2: Optional = None): ... +13 13 | +14 14 | +15 |-def multiple_2(arg1: Optional, arg2: Optional = None, arg3: int = None): ... + 15 |+def multiple_2(arg1: Optional, arg2: Optional = None, arg3: int | None = None): ... +16 16 | +17 17 | +18 18 | def return_type(arg: Optional = None) -> Optional: ... diff --git a/crates/ruff_linter/src/rules/ruff/typing.rs b/crates/ruff_linter/src/rules/ruff/typing.rs index 08fde21b31a70..f7ed900719e0e 100644 --- a/crates/ruff_linter/src/rules/ruff/typing.rs +++ b/crates/ruff_linter/src/rules/ruff/typing.rs @@ -42,19 +42,19 @@ enum TypingTarget<'a> { ForwardReference(&'a Expr), /// A `typing.Union` type e.g., `Union[int, str]`. - Union(&'a Expr), + Union(Option<&'a Expr>), /// A PEP 604 union type e.g., `int | str`. PEP604Union(&'a Expr, &'a Expr), /// A `typing.Literal` type e.g., `Literal[1, 2, 3]`. - Literal(&'a Expr), + Literal(Option<&'a Expr>), /// A `typing.Optional` type e.g., `Optional[int]`. - Optional(&'a Expr), + Optional(Option<&'a Expr>), /// A `typing.Annotated` type e.g., `Annotated[int, ...]`. - Annotated(&'a Expr), + Annotated(Option<&'a Expr>), /// The `typing.Hashable` type. Hashable, @@ -80,16 +80,16 @@ impl<'a> TypingTarget<'a> { Some(TypingTarget::Unknown), |qualified_name| { if semantic.match_typing_qualified_name(&qualified_name, "Optional") { - Some(TypingTarget::Optional(slice.as_ref())) + Some(TypingTarget::Optional(Some(slice.as_ref()))) } else if semantic.match_typing_qualified_name(&qualified_name, "Literal") { - Some(TypingTarget::Literal(slice.as_ref())) + Some(TypingTarget::Literal(Some(slice.as_ref()))) } else if semantic.match_typing_qualified_name(&qualified_name, "Union") { - Some(TypingTarget::Union(slice.as_ref())) + Some(TypingTarget::Union(Some(slice.as_ref()))) } else if semantic.match_typing_qualified_name(&qualified_name, "Annotated") { - resolve_slice_value(slice.as_ref()) - .next() - .map(TypingTarget::Annotated) + Some(TypingTarget::Annotated( + resolve_slice_value(slice.as_ref()).next(), + )) } else { if is_known_type(&qualified_name, minor_version) { Some(TypingTarget::Known) @@ -118,7 +118,15 @@ impl<'a> TypingTarget<'a> { // same file, so we assume it's `Any` as it could be a type alias. Some(TypingTarget::Unknown), |qualified_name| { - if semantic.match_typing_qualified_name(&qualified_name, "Any") { + if semantic.match_typing_qualified_name(&qualified_name, "Optional") { + Some(TypingTarget::Optional(None)) + } else if semantic.match_typing_qualified_name(&qualified_name, "Literal") { + Some(TypingTarget::Literal(None)) + } else if semantic.match_typing_qualified_name(&qualified_name, "Union") { + Some(TypingTarget::Union(None)) + } else if semantic.match_typing_qualified_name(&qualified_name, "Annotated") { + Some(TypingTarget::Annotated(None)) + } else if semantic.match_typing_qualified_name(&qualified_name, "Any") { Some(TypingTarget::Any) } else if matches!(qualified_name.segments(), ["" | "builtins", "object"]) { Some(TypingTarget::Object) @@ -150,22 +158,26 @@ impl<'a> TypingTarget<'a> { | TypingTarget::Object | TypingTarget::Unknown => true, TypingTarget::Known => false, - TypingTarget::Literal(slice) => resolve_slice_value(slice).any(|element| { - // Literal can only contain `None`, a literal value, other `Literal` - // or an enum value. - match TypingTarget::try_from_expr(element, checker, minor_version) { - None | Some(TypingTarget::None) => true, - Some(new_target @ TypingTarget::Literal(_)) => { - new_target.contains_none(checker, minor_version) + TypingTarget::Literal(slice) => slice.is_some_and(|slice| { + resolve_slice_value(slice).any(|element| { + // Literal can only contain `None`, a literal value, other `Literal` + // or an enum value. + match TypingTarget::try_from_expr(element, checker, minor_version) { + None | Some(TypingTarget::None) => true, + Some(new_target @ TypingTarget::Literal(_)) => { + new_target.contains_none(checker, minor_version) + } + _ => false, } - _ => false, - } + }) }), - TypingTarget::Union(slice) => resolve_slice_value(slice).any(|element| { - TypingTarget::try_from_expr(element, checker, minor_version) - .map_or(true, |new_target| { - new_target.contains_none(checker, minor_version) - }) + TypingTarget::Union(slice) => slice.is_some_and(|slice| { + resolve_slice_value(slice).any(|element| { + TypingTarget::try_from_expr(element, checker, minor_version) + .map_or(true, |new_target| { + new_target.contains_none(checker, minor_version) + }) + }) }), TypingTarget::PEP604Union(left, right) => [left, right].iter().any(|element| { TypingTarget::try_from_expr(element, checker, minor_version) @@ -173,12 +185,12 @@ impl<'a> TypingTarget<'a> { new_target.contains_none(checker, minor_version) }) }), - TypingTarget::Annotated(expr) => { + TypingTarget::Annotated(expr) => expr.is_some_and(|expr| { TypingTarget::try_from_expr(expr, checker, minor_version) .map_or(true, |new_target| { new_target.contains_none(checker, minor_version) }) - } + }), TypingTarget::ForwardReference(expr) => { TypingTarget::try_from_expr(expr, checker, minor_version) .map_or(true, |new_target| { @@ -199,11 +211,13 @@ impl<'a> TypingTarget<'a> { | TypingTarget::Object | TypingTarget::Known | TypingTarget::Unknown => false, - TypingTarget::Union(slice) => resolve_slice_value(slice).any(|element| { - TypingTarget::try_from_expr(element, checker, minor_version) - .map_or(true, |new_target| { - new_target.contains_any(checker, minor_version) - }) + TypingTarget::Union(slice) => slice.is_some_and(|slice| { + resolve_slice_value(slice).any(|element| { + TypingTarget::try_from_expr(element, checker, minor_version) + .map_or(true, |new_target| { + new_target.contains_any(checker, minor_version) + }) + }) }), TypingTarget::PEP604Union(left, right) => [left, right].iter().any(|element| { TypingTarget::try_from_expr(element, checker, minor_version) @@ -212,10 +226,12 @@ impl<'a> TypingTarget<'a> { }) }), TypingTarget::Annotated(expr) | TypingTarget::Optional(expr) => { - TypingTarget::try_from_expr(expr, checker, minor_version) - .map_or(true, |new_target| { - new_target.contains_any(checker, minor_version) - }) + expr.is_some_and(|expr| { + TypingTarget::try_from_expr(expr, checker, minor_version) + .map_or(true, |new_target| { + new_target.contains_any(checker, minor_version) + }) + }) } TypingTarget::ForwardReference(expr) => { TypingTarget::try_from_expr(expr, checker, minor_version) @@ -248,14 +264,13 @@ pub(crate) fn type_hint_explicitly_allows_none<'a>( // is found nested inside another type, then the outer type should // be returned. Some(TypingTarget::Annotated(expr)) => { - type_hint_explicitly_allows_none(expr, checker, minor_version) + expr.and_then(|expr| type_hint_explicitly_allows_none(expr, checker, minor_version)) } Some(target) => { if target.contains_none(checker, minor_version) { - None - } else { - Some(annotation) + return None; } + Some(annotation) } } } @@ -269,13 +284,14 @@ pub(crate) fn type_hint_resolves_to_any( minor_version: u8, ) -> bool { match TypingTarget::try_from_expr(annotation, checker, minor_version) { - None | - // Short circuit on top level `Any` - Some(TypingTarget::Any) => true, + // Short circuit on top level `Any` + None | Some(TypingTarget::Any) => true, + // `Optional` is `Optional[Any]` which is `Any | None`. + Some(TypingTarget::Optional(None)) => true, // Top-level `Annotated` node should check if the inner type resolves // to `Any`. Some(TypingTarget::Annotated(expr)) => { - type_hint_resolves_to_any(expr, checker, minor_version) + expr.is_some_and(|expr| type_hint_resolves_to_any(expr, checker, minor_version)) } Some(target) => target.contains_any(checker, minor_version), }