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

Ignore type aliases for RUF013 #5344

Merged
merged 5 commits into from
Jun 23, 2023
Merged

Ignore type aliases for RUF013 #5344

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8ff652c
Ignore type aliases for RUF013 to avoid false positives
dhruvmanila Jun 23, 2023
94fbe35
Fix docs formatting
dhruvmanila Jun 23, 2023
bfb9a63
Detect known type as builtin, stdlib or typing_extensions
dhruvmanila Jun 23, 2023
d3ec468
Use updated API to check for stdlib module
dhruvmanila Jun 23, 2023
23a9077
Tweak
charliermarsh Jun 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions crates/ruff/resources/test/fixtures/ruff/RUF013_0.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
147 changes: 122 additions & 25 deletions crates/ruff/src/rules/ruff/rules/implicit_optional.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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`
///
Expand Down Expand Up @@ -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,
Expand All @@ -154,7 +180,12 @@ enum TypingTarget<'a> {
}

impl<'a> TypingTarget<'a> {
fn try_from_expr(expr: &'a Expr, semantic: &SemanticModel, locator: &Locator) -> Option<Self> {
fn try_from_expr(
expr: &'a Expr,
semantic: &SemanticModel,
locator: &Locator,
target_version: PythonVersion,
) -> Option<Self> {
match expr {
Expr::Subscript(ast::ExprSubscript { value, slice, .. }) => {
if semantic.match_typing_expr(value, "Optional") {
Expand All @@ -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(
Expand All @@ -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)
}
}
}
Expand All @@ -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 {
Expand All @@ -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)
Expand Down Expand Up @@ -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();
Expand All @@ -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();
Expand All @@ -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
));
}
}
Loading