Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add support for nested quoted annotations in RUF013 #5254

Merged
merged 3 commits into from Jun 21, 2023
Merged

Add support for nested quoted annotations in RUF013 #5254

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1698e31
Add support for nested quoted annotations in RUF013
dhruvmanila Jun 21, 2023
8449f46
Avoid unnecessary `match` expr (recursion checks it)
dhruvmanila Jun 21, 2023
475a69a
Avoid dependency on `Checker`, use `Any` for parse error
dhruvmanila Jun 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 10 additions & 1 deletion crates/ruff/resources/test/fixtures/ruff/RUF013_0.py
View file
Open in desktop
Expand Up @@ -206,9 +206,18 @@ def f(arg: "Optional[int]" = None):
pass


def f(arg: Union["int", "str"] = None): # False negative
def f(arg: Union["int", "str"] = None): # RUF013
pass


def f(arg: Union["int", "None"] = None):
pass


def f(arg: Union["No" "ne", "int"] = None):
pass


# Avoid flagging when there's a parse error in the forward reference
def f(arg: Union["<>", "int"] = None):
pass
57 changes: 31 additions & 26 deletions crates/ruff/src/rules/ruff/rules/implicit_optional.rs
View file
Open in desktop
Expand Up @@ -7,6 +7,7 @@ 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::helpers::is_const_none;
use ruff_python_ast::source_code::Locator;
use ruff_python_ast::typing::parse_type_annotation;
use ruff_python_semantic::SemanticModel;

Expand Down Expand Up @@ -145,15 +146,15 @@ enum TypingTarget<'a> {
None,
Any,
Object,
ForwardReference,
Optional,
ForwardReference(Expr),
Union(Vec<&'a Expr>),
Literal(Vec<&'a Expr>),
Annotated(&'a Expr),
}

impl<'a> TypingTarget<'a> {
fn try_from_expr(expr: &'a Expr, semantic: &SemanticModel) -> Option<Self> {
fn try_from_expr(expr: &'a Expr, semantic: &SemanticModel, locator: &Locator) -> Option<Self> {
match expr {
Expr::Subscript(ast::ExprSubscript { value, slice, .. }) => {
if semantic.match_typing_expr(value, "Optional") {
Expand All @@ -180,9 +181,14 @@ impl<'a> TypingTarget<'a> {
..
}) => Some(TypingTarget::None),
Expr::Constant(ast::ExprConstant {
value: Constant::Str(_),
value: Constant::Str(string),
range,
..
}) => Some(TypingTarget::ForwardReference),
}) => parse_type_annotation(string, *range, locator)
// In case of a parse error, we return `Any` to avoid false positives.
.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)
Expand All @@ -196,44 +202,42 @@ impl<'a> TypingTarget<'a> {
}

/// Check if the [`TypingTarget`] explicitly allows `None`.
fn contains_none(&self, semantic: &SemanticModel) -> bool {
fn contains_none(&self, semantic: &SemanticModel, locator: &Locator) -> 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) else {
let Some(new_target) = TypingTarget::try_from_expr(element, semantic, locator) 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),
TypingTarget::Literal(_) => new_target.contains_none(semantic, locator),
_ => false,
}
}),
TypingTarget::Union(elements) => elements.iter().any(|element| {
let Some(new_target) = TypingTarget::try_from_expr(element, semantic) else {
let Some(new_target) = TypingTarget::try_from_expr(element, semantic, locator) else {
return false;
};
match new_target {
TypingTarget::None => true,
_ => new_target.contains_none(semantic),
}
new_target.contains_none(semantic, locator)
}),
TypingTarget::Annotated(element) => {
let Some(new_target) = TypingTarget::try_from_expr(element, semantic) else {
let Some(new_target) = TypingTarget::try_from_expr(element, semantic, locator) else {
return false;
};
match new_target {
TypingTarget::None => true,
_ => new_target.contains_none(semantic),
}
new_target.contains_none(semantic, locator)
}
TypingTarget::ForwardReference(expr) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to also take &Expr in ForwardReference and unify the branches or doesn't that work because the expression is created outside the AST an needs to be owned?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was unable to unify them due to the owned value. For ForwardReference, the enum needs to own it while for others we receive it via the AST checker.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I found one small refactor :)

let Some(new_target) = TypingTarget::try_from_expr(expr, semantic, locator) else {
return false;
};
new_target.contains_none(semantic, locator)
}
// TODO(charlie): Add support for nested forward references (e.g., `Union["A", "B"]`).
TypingTarget::ForwardReference => true,
}
}
}
Expand All @@ -248,8 +252,9 @@ impl<'a> TypingTarget<'a> {
fn type_hint_explicitly_allows_none<'a>(
annotation: &'a Expr,
semantic: &SemanticModel,
locator: &Locator,
) -> Option<&'a Expr> {
let Some(target) = TypingTarget::try_from_expr(annotation, semantic) else {
let Some(target) = TypingTarget::try_from_expr(annotation, semantic, locator) else {
return Some(annotation);
};
match target {
Expand All @@ -259,9 +264,9 @@ 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),
TypingTarget::Annotated(expr) => type_hint_explicitly_allows_none(expr, semantic, locator),
_ => {
if target.contains_none(semantic) {
if target.contains_none(semantic, locator) {
None
} else {
Some(annotation)
Expand Down Expand Up @@ -339,13 +344,13 @@ pub(crate) fn implicit_optional(checker: &mut Checker, arguments: &Arguments) {

if let Expr::Constant(ast::ExprConstant {
range,
value: Constant::Str(value),
value: Constant::Str(string),
..
}) = annotation.as_ref()
{
// Quoted annotation.
if let Ok((annotation, kind)) = parse_type_annotation(value, *range, checker.locator) {
let Some(expr) = type_hint_explicitly_allows_none(&annotation, checker.semantic()) else {
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 {
continue;
};
let conversion_type = checker.settings.target_version.into();
Expand All @@ -361,7 +366,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()) else {
let Some(expr) = type_hint_explicitly_allows_none(annotation, checker.semantic(), checker.locator) else {
continue;
};
let conversion_type = checker.settings.target_version.into();
Expand Down
18 changes: 18 additions & 0 deletions crates/ruff/src/rules/ruff/snapshots/ruff__rules__ruff__tests__PY39_RUF013_RUF013_0.py.snap
View file
Open in desktop
Expand Up @@ -359,4 +359,22 @@ RUF013_0.py:201:12: RUF013 PEP 484 prohibits implicit `Optional`
|
= help: Convert to `Optional[T]`

RUF013_0.py:209:12: RUF013 [*] PEP 484 prohibits implicit `Optional`
|
209 | def f(arg: Union["int", "str"] = None): # RUF013
| ^^^^^^^^^^^^^^^^^^^ RUF013
210 | pass
|
= help: Convert to `Optional[T]`

ℹ Suggested fix
206 206 | pass
207 207 |
208 208 |
209 |-def f(arg: Union["int", "str"] = None): # RUF013
209 |+def f(arg: Optional[Union["int", "str"]] = None): # RUF013
210 210 | pass
211 211 |
212 212 |


18 changes: 18 additions & 0 deletions crates/ruff/src/rules/ruff/snapshots/ruff__rules__ruff__tests__RUF013_RUF013_0.py.snap
View file
Open in desktop
Expand Up @@ -359,4 +359,22 @@ RUF013_0.py:201:12: RUF013 PEP 484 prohibits implicit `Optional`
|
= help: Convert to `T | None`

RUF013_0.py:209:12: RUF013 [*] PEP 484 prohibits implicit `Optional`
|
209 | def f(arg: Union["int", "str"] = None): # RUF013
| ^^^^^^^^^^^^^^^^^^^ RUF013
210 | pass
|
= help: Convert to `T | None`

ℹ Suggested fix
206 206 | pass
207 207 |
208 208 |
209 |-def f(arg: Union["int", "str"] = None): # RUF013
209 |+def f(arg: Union["int", "str"] | None = None): # RUF013
210 210 | pass
211 211 |
212 212 |