From b1247589d48f578bb363676ab9ed5c9d87d0a14c Mon Sep 17 00:00:00 2001 From: InSyncWithFoo Date: Fri, 8 Nov 2024 00:04:00 +0000 Subject: [PATCH 1/4] [`ruff`] Do not report when `Optional` has no type arguments (`RUF013`) --- .../resources/test/fixtures/ruff/RUF013_4.py | 21 +++++++++++++++++++ crates/ruff_linter/src/rules/ruff/mod.rs | 1 + .../src/rules/ruff/rules/implicit_optional.rs | 19 +++++++++++++++++ ...ules__ruff__tests__RUF013_RUF013_4.py.snap | 20 ++++++++++++++++++ 4 files changed, 61 insertions(+) create mode 100644 crates/ruff_linter/resources/test/fixtures/ruff/RUF013_4.py create mode 100644 crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF013_RUF013_4.py.snap 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/rules/implicit_optional.rs b/crates/ruff_linter/src/rules/ruff/rules/implicit_optional.rs index a192a0b4269ff..29ff30c8b1c13 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/implicit_optional.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/implicit_optional.rs @@ -120,6 +120,17 @@ impl From for ConversionType { } } +/// Whether `annotation` is `typing.Optional` with no type arguments. +/// +/// See [#13833](https://github.com/astral-sh/ruff/issues/13833). +fn annotation_is_bare_optional(checker: &Checker, annotation: &Expr) -> bool { + let Some(qualified_name) = checker.semantic().resolve_qualified_name(annotation) else { + return false; + }; + + matches!(qualified_name.segments(), ["typing", "Optional"]) +} + /// Generate a [`Fix`] for the given [`Expr`] as per the [`ConversionType`]. fn generate_fix(checker: &Checker, conversion_type: ConversionType, expr: &Expr) -> Result { match conversion_type { @@ -189,6 +200,10 @@ pub(crate) fn implicit_optional(checker: &mut Checker, parameters: &Parameters) }; let conversion_type = checker.settings.target_version.into(); + if annotation_is_bare_optional(checker, parsed_annotation.expression()) { + continue; + } + let mut diagnostic = Diagnostic::new(ImplicitOptional { conversion_type }, expr.range()); if parsed_annotation.kind().is_simple() { @@ -207,6 +222,10 @@ pub(crate) fn implicit_optional(checker: &mut Checker, parameters: &Parameters) }; let conversion_type = checker.settings.target_version.into(); + if annotation_is_bare_optional(checker, annotation) { + continue; + } + let mut diagnostic = Diagnostic::new(ImplicitOptional { conversion_type }, expr.range()); diagnostic.try_set_fix(|| generate_fix(checker, conversion_type, expr)); 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: ... From 13e77a14e66525af83aec47cb3202d253356cc98 Mon Sep 17 00:00:00 2001 From: InSyncWithFoo Date: Fri, 8 Nov 2024 13:22:47 +0000 Subject: [PATCH 2/4] Move new logic to `type_hint_explicitly_allows_none` --- .../src/rules/ruff/rules/implicit_optional.rs | 19 ----------------- crates/ruff_linter/src/rules/ruff/typing.rs | 21 ++++++++++++++++--- 2 files changed, 18 insertions(+), 22 deletions(-) diff --git a/crates/ruff_linter/src/rules/ruff/rules/implicit_optional.rs b/crates/ruff_linter/src/rules/ruff/rules/implicit_optional.rs index 29ff30c8b1c13..a192a0b4269ff 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/implicit_optional.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/implicit_optional.rs @@ -120,17 +120,6 @@ impl From for ConversionType { } } -/// Whether `annotation` is `typing.Optional` with no type arguments. -/// -/// See [#13833](https://github.com/astral-sh/ruff/issues/13833). -fn annotation_is_bare_optional(checker: &Checker, annotation: &Expr) -> bool { - let Some(qualified_name) = checker.semantic().resolve_qualified_name(annotation) else { - return false; - }; - - matches!(qualified_name.segments(), ["typing", "Optional"]) -} - /// Generate a [`Fix`] for the given [`Expr`] as per the [`ConversionType`]. fn generate_fix(checker: &Checker, conversion_type: ConversionType, expr: &Expr) -> Result { match conversion_type { @@ -200,10 +189,6 @@ pub(crate) fn implicit_optional(checker: &mut Checker, parameters: &Parameters) }; let conversion_type = checker.settings.target_version.into(); - if annotation_is_bare_optional(checker, parsed_annotation.expression()) { - continue; - } - let mut diagnostic = Diagnostic::new(ImplicitOptional { conversion_type }, expr.range()); if parsed_annotation.kind().is_simple() { @@ -222,10 +207,6 @@ pub(crate) fn implicit_optional(checker: &mut Checker, parameters: &Parameters) }; let conversion_type = checker.settings.target_version.into(); - if annotation_is_bare_optional(checker, annotation) { - continue; - } - let mut diagnostic = Diagnostic::new(ImplicitOptional { conversion_type }, expr.range()); diagnostic.try_set_fix(|| generate_fix(checker, conversion_type, expr)); diff --git a/crates/ruff_linter/src/rules/ruff/typing.rs b/crates/ruff_linter/src/rules/ruff/typing.rs index 08fde21b31a70..b030c9e8fea4f 100644 --- a/crates/ruff_linter/src/rules/ruff/typing.rs +++ b/crates/ruff_linter/src/rules/ruff/typing.rs @@ -227,6 +227,17 @@ impl<'a> TypingTarget<'a> { } } +/// Whether `annotation` is `typing.Optional` with no type arguments. +/// +/// See [#13833](https://github.com/astral-sh/ruff/issues/13833). +fn annotation_is_bare_optional(checker: &Checker, annotation: &Expr) -> bool { + let Some(qualified_name) = checker.semantic().resolve_qualified_name(annotation) else { + return false; + }; + + matches!(qualified_name.segments(), ["typing", "Optional"]) +} + /// Check if the given annotation [`Expr`] explicitly allows `None`. /// /// This function will return `None` if the annotation explicitly allows `None` @@ -252,10 +263,14 @@ pub(crate) fn type_hint_explicitly_allows_none<'a>( } Some(target) => { if target.contains_none(checker, minor_version) { - None - } else { - Some(annotation) + return None; + } + + if annotation_is_bare_optional(checker, annotation) { + return None; } + + Some(annotation) } } } From b952f8e72cb38c3cf2563726a996b0b14181946d Mon Sep 17 00:00:00 2001 From: InSyncWithFoo Date: Fri, 8 Nov 2024 13:52:39 +0000 Subject: [PATCH 3/4] `rustfmt` --- crates/ruff_linter/src/rules/ruff/typing.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/ruff_linter/src/rules/ruff/typing.rs b/crates/ruff_linter/src/rules/ruff/typing.rs index b030c9e8fea4f..5713656ea5f2a 100644 --- a/crates/ruff_linter/src/rules/ruff/typing.rs +++ b/crates/ruff_linter/src/rules/ruff/typing.rs @@ -234,7 +234,7 @@ fn annotation_is_bare_optional(checker: &Checker, annotation: &Expr) -> bool { let Some(qualified_name) = checker.semantic().resolve_qualified_name(annotation) else { return false; }; - + matches!(qualified_name.segments(), ["typing", "Optional"]) } @@ -265,11 +265,11 @@ pub(crate) fn type_hint_explicitly_allows_none<'a>( if target.contains_none(checker, minor_version) { return None; } - + if annotation_is_bare_optional(checker, annotation) { return None; } - + Some(annotation) } } From 049f26ec41de1f3359556c6e4ac6a47f4754e246 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Fri, 8 Nov 2024 21:47:01 -0500 Subject: [PATCH 4/4] Allow optional --- .../fixtures/pylint/await_outside_async.py | 4 + crates/ruff_linter/src/rules/ruff/typing.rs | 113 +++++++++--------- 2 files changed, 60 insertions(+), 57 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/await_outside_async.py b/crates/ruff_linter/resources/test/fixtures/pylint/await_outside_async.py index 2bc1267615a4e..ea91f1bf91f06 100644 --- a/crates/ruff_linter/resources/test/fixtures/pylint/await_outside_async.py +++ b/crates/ruff_linter/resources/test/fixtures/pylint/await_outside_async.py @@ -1,13 +1,16 @@ # pylint: disable=missing-docstring,unused-variable import asyncio + async def nested(): return 42 + async def main(): nested() print(await nested()) # This is okay + def not_async(): print(await nested()) # [await-outside-async] @@ -15,6 +18,7 @@ def not_async(): async def func(i): return i**2 + async def okay_function(): var = [await func(i) for i in range(5)] # This should be okay diff --git a/crates/ruff_linter/src/rules/ruff/typing.rs b/crates/ruff_linter/src/rules/ruff/typing.rs index 5713656ea5f2a..e7cab12ed8ea8 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) @@ -227,17 +243,6 @@ impl<'a> TypingTarget<'a> { } } -/// Whether `annotation` is `typing.Optional` with no type arguments. -/// -/// See [#13833](https://github.com/astral-sh/ruff/issues/13833). -fn annotation_is_bare_optional(checker: &Checker, annotation: &Expr) -> bool { - let Some(qualified_name) = checker.semantic().resolve_qualified_name(annotation) else { - return false; - }; - - matches!(qualified_name.segments(), ["typing", "Optional"]) -} - /// Check if the given annotation [`Expr`] explicitly allows `None`. /// /// This function will return `None` if the annotation explicitly allows `None` @@ -259,17 +264,12 @@ 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) { return None; } - - if annotation_is_bare_optional(checker, annotation) { - return None; - } - Some(annotation) } } @@ -284,13 +284,12 @@ 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, // 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), }