Skip to content

Commit

Permalink
[ruff] Do not report when Optional has no type arguments (`RUF013…
Browse files Browse the repository at this point in the history 
…`) (#14181)

## Summary

Resolves #13833.

## Test Plan

`cargo nextest run` and `cargo insta test`.

---------

Co-authored-by: Charlie Marsh <charlie.r.marsh@gmail.com>
  • Loading branch information
@InSyncWithFoo @charliermarsh
InSyncWithFoo and charliermarsh authored Nov 9, 2024
1 parent d3f1c8e commit c9b84e2
Show file tree
Hide file tree
Showing 4 changed files with 102 additions and 44 deletions.
21 changes: 21 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/ruff/RUF013_4.py
View file
Original file line number Diff line number Diff line change
@@ -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): ...
1 change: 1 addition & 0 deletions crates/ruff_linter/src/rules/ruff/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"))]
Expand Down
20 changes: 20 additions & 0 deletions ..._linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF013_RUF013_4.py.snap
View file
Original file line number Diff line number Diff line change
@@ -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: ...
104 changes: 60 additions & 44 deletions crates/ruff_linter/src/rules/ruff/typing.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -150,35 +158,39 @@ 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)
.map_or(true, |new_target| {
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| {
Expand All @@ -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)
Expand All @@ -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)
Expand Down Expand Up @@ -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)
}
}
}
Expand All @@ -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),
}
Expand Down

0 comments on commit c9b84e2

Please sign in to comment.