Skip to content

Commit

Permalink
Flag errors in quoted annotations
Browse files Browse the repository at this point in the history
  • Loading branch information
charliermarsh committed Mar 20, 2023
1 parent a45753f commit e9ef26d
Show file tree
Hide file tree
Showing 17 changed files with 173 additions and 98 deletions.
23 changes: 9 additions & 14 deletions crates/ruff/src/checkers/ast/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2170,9 +2170,8 @@ where
ExprKind::Subscript { value, slice, .. } => {
// Ex) Optional[...], Union[...]
if self.ctx.in_type_definition
&& !self.ctx.in_deferred_string_type_definition
&& !self.settings.pyupgrade.keep_runtime_typing
&& self.settings.rules.enabled(Rule::TypingUnion)
&& self.settings.rules.enabled(Rule::NonPEP604Annotation)
&& (self.settings.target_version >= PythonVersion::Py310
|| (self.settings.target_version >= PythonVersion::Py37
&& self.ctx.annotations_future_enabled
Expand Down Expand Up @@ -2225,16 +2224,13 @@ where
}

// Ex) List[...]
if !self.ctx.in_deferred_string_type_definition
&& !self.settings.pyupgrade.keep_runtime_typing
&& self.settings.rules.enabled(Rule::DeprecatedCollectionType)
if !self.settings.pyupgrade.keep_runtime_typing
&& self.settings.rules.enabled(Rule::NonPEP585Annotation)
&& (self.settings.target_version >= PythonVersion::Py39
|| (self.settings.target_version >= PythonVersion::Py37
&& self.ctx.annotations_future_enabled
&& self.ctx.in_annotation))
&& typing::is_pep585_builtin(expr, |expr| {
self.ctx.resolve_call_path(expr)
})
&& typing::is_pep585_builtin(expr, &self.ctx)
{
pyupgrade::rules::use_pep585_annotation(self, expr);
}
Expand Down Expand Up @@ -2271,14 +2267,13 @@ where
}
ExprKind::Attribute { attr, value, .. } => {
// Ex) typing.List[...]
if !self.ctx.in_deferred_string_type_definition
&& !self.settings.pyupgrade.keep_runtime_typing
&& self.settings.rules.enabled(Rule::DeprecatedCollectionType)
if !self.settings.pyupgrade.keep_runtime_typing
&& self.settings.rules.enabled(Rule::NonPEP585Annotation)
&& (self.settings.target_version >= PythonVersion::Py39
|| (self.settings.target_version >= PythonVersion::Py37
&& self.ctx.annotations_future_enabled
&& self.ctx.in_annotation))
&& typing::is_pep585_builtin(expr, |expr| self.ctx.resolve_call_path(expr))
&& typing::is_pep585_builtin(expr, &self.ctx)
{
pyupgrade::rules::use_pep585_annotation(self, expr);
}
Expand Down Expand Up @@ -2434,7 +2429,7 @@ where
if self.settings.rules.enabled(Rule::OSErrorAlias) {
pyupgrade::rules::os_error_alias_call(self, func);
}
if self.settings.rules.enabled(Rule::IsinstanceWithTuple)
if self.settings.rules.enabled(Rule::NonPEP604Isinstance)
&& self.settings.target_version >= PythonVersion::Py310
{
pyupgrade::rules::use_pep604_isinstance(self, expr, func, args);
Expand Down Expand Up @@ -3575,7 +3570,7 @@ where
} else {
match match_annotated_subscript(
value,
|expr| self.ctx.resolve_call_path(expr),
&self.ctx,
self.settings.typing_modules.iter().map(String::as_str),
) {
Some(subscript) => {
Expand Down
6 changes: 3 additions & 3 deletions crates/ruff/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -355,8 +355,8 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<Rule> {
(Pyupgrade, "003") => Rule::TypeOfPrimitive,
(Pyupgrade, "004") => Rule::UselessObjectInheritance,
(Pyupgrade, "005") => Rule::DeprecatedUnittestAlias,
(Pyupgrade, "006") => Rule::DeprecatedCollectionType,
(Pyupgrade, "007") => Rule::TypingUnion,
(Pyupgrade, "006") => Rule::NonPEP585Annotation,
(Pyupgrade, "007") => Rule::NonPEP604Annotation,
(Pyupgrade, "008") => Rule::SuperCallWithParameters,
(Pyupgrade, "009") => Rule::UTF8EncodingDeclaration,
(Pyupgrade, "010") => Rule::UnnecessaryFutureImport,
Expand Down Expand Up @@ -386,7 +386,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<Rule> {
(Pyupgrade, "035") => Rule::DeprecatedImport,
(Pyupgrade, "036") => Rule::OutdatedVersionBlock,
(Pyupgrade, "037") => Rule::QuotedAnnotation,
(Pyupgrade, "038") => Rule::IsinstanceWithTuple,
(Pyupgrade, "038") => Rule::NonPEP604Isinstance,

// pydocstyle
(Pydocstyle, "100") => Rule::UndocumentedPublicModule,
Expand Down
6 changes: 3 additions & 3 deletions crates/ruff/src/registry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -325,8 +325,8 @@ ruff_macros::register_rules!(
rules::pyupgrade::rules::TypeOfPrimitive,
rules::pyupgrade::rules::UselessObjectInheritance,
rules::pyupgrade::rules::DeprecatedUnittestAlias,
rules::pyupgrade::rules::DeprecatedCollectionType,
rules::pyupgrade::rules::TypingUnion,
rules::pyupgrade::rules::NonPEP585Annotation,
rules::pyupgrade::rules::NonPEP604Annotation,
rules::pyupgrade::rules::SuperCallWithParameters,
rules::pyupgrade::rules::UTF8EncodingDeclaration,
rules::pyupgrade::rules::UnnecessaryFutureImport,
Expand Down Expand Up @@ -356,7 +356,7 @@ ruff_macros::register_rules!(
rules::pyupgrade::rules::DeprecatedImport,
rules::pyupgrade::rules::OutdatedVersionBlock,
rules::pyupgrade::rules::QuotedAnnotation,
rules::pyupgrade::rules::IsinstanceWithTuple,
rules::pyupgrade::rules::NonPEP604Isinstance,
// pydocstyle
rules::pydocstyle::rules::UndocumentedPublicModule,
rules::pydocstyle::rules::UndocumentedPublicClass,
Expand Down
14 changes: 7 additions & 7 deletions crates/ruff/src/rules/pyupgrade/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,8 @@ mod tests {
#[test_case(Rule::TypeOfPrimitive, Path::new("UP003.py"); "UP003")]
#[test_case(Rule::UselessObjectInheritance, Path::new("UP004.py"); "UP004")]
#[test_case(Rule::DeprecatedUnittestAlias, Path::new("UP005.py"); "UP005")]
#[test_case(Rule::DeprecatedCollectionType, Path::new("UP006.py"); "UP006")]
#[test_case(Rule::TypingUnion, Path::new("UP007.py"); "UP007")]
#[test_case(Rule::NonPEP585Annotation, Path::new("UP006.py"); "UP006")]
#[test_case(Rule::NonPEP604Annotation, Path::new("UP007.py"); "UP007")]
#[test_case(Rule::SuperCallWithParameters, Path::new("UP008.py"); "UP008")]
#[test_case(Rule::UTF8EncodingDeclaration, Path::new("UP009_0.py"); "UP009_0")]
#[test_case(Rule::UTF8EncodingDeclaration, Path::new("UP009_1.py"); "UP009_1")]
Expand Down Expand Up @@ -67,7 +67,7 @@ mod tests {
#[test_case(Rule::OutdatedVersionBlock, Path::new("UP036_3.py"); "UP036_3")]
#[test_case(Rule::OutdatedVersionBlock, Path::new("UP036_4.py"); "UP036_4")]
#[test_case(Rule::QuotedAnnotation, Path::new("UP037.py"); "UP037")]
#[test_case(Rule::IsinstanceWithTuple, Path::new("UP038.py"); "UP038")]
#[test_case(Rule::NonPEP604Isinstance, Path::new("UP038.py"); "UP038")]
fn rules(rule_code: Rule, path: &Path) -> Result<()> {
let snapshot = path.to_string_lossy().to_string();
let diagnostics = test_path(
Expand All @@ -84,7 +84,7 @@ mod tests {
Path::new("pyupgrade/future_annotations.py"),
&settings::Settings {
target_version: PythonVersion::Py37,
..settings::Settings::for_rule(Rule::DeprecatedCollectionType)
..settings::Settings::for_rule(Rule::NonPEP585Annotation)
},
)?;
assert_yaml_snapshot!(diagnostics);
Expand All @@ -97,7 +97,7 @@ mod tests {
Path::new("pyupgrade/future_annotations.py"),
&settings::Settings {
target_version: PythonVersion::Py310,
..settings::Settings::for_rule(Rule::DeprecatedCollectionType)
..settings::Settings::for_rule(Rule::NonPEP585Annotation)
},
)?;
assert_yaml_snapshot!(diagnostics);
Expand All @@ -110,7 +110,7 @@ mod tests {
Path::new("pyupgrade/future_annotations.py"),
&settings::Settings {
target_version: PythonVersion::Py37,
..settings::Settings::for_rule(Rule::TypingUnion)
..settings::Settings::for_rule(Rule::NonPEP604Annotation)
},
)?;
assert_yaml_snapshot!(diagnostics);
Expand All @@ -123,7 +123,7 @@ mod tests {
Path::new("pyupgrade/future_annotations.py"),
&settings::Settings {
target_version: PythonVersion::Py310,
..settings::Settings::for_rule(Rule::TypingUnion)
..settings::Settings::for_rule(Rule::NonPEP604Annotation)
},
)?;
assert_yaml_snapshot!(diagnostics);
Expand Down
6 changes: 3 additions & 3 deletions crates/ruff/src/rules/pyupgrade/rules/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,9 @@ pub(crate) use unnecessary_future_import::{unnecessary_future_import, Unnecessar
pub(crate) use unpacked_list_comprehension::{
unpacked_list_comprehension, UnpackedListComprehension,
};
pub(crate) use use_pep585_annotation::{use_pep585_annotation, DeprecatedCollectionType};
pub(crate) use use_pep604_annotation::{use_pep604_annotation, TypingUnion};
pub(crate) use use_pep604_isinstance::{use_pep604_isinstance, IsinstanceWithTuple};
pub(crate) use use_pep585_annotation::{use_pep585_annotation, NonPEP585Annotation};
pub(crate) use use_pep604_annotation::{use_pep604_annotation, NonPEP604Annotation};
pub(crate) use use_pep604_isinstance::{use_pep604_isinstance, NonPEP604Isinstance};
pub(crate) use useless_metaclass_type::{useless_metaclass_type, UselessMetaclassType};
pub(crate) use useless_object_inheritance::{useless_object_inheritance, UselessObjectInheritance};
pub(crate) use yield_in_for_loop::{yield_in_for_loop, YieldInForLoop};
Expand Down
25 changes: 15 additions & 10 deletions crates/ruff/src/rules/pyupgrade/rules/use_pep585_annotation.rs
Original file line number Diff line number Diff line change
@@ -1,32 +1,35 @@
use rustpython_parser::ast::Expr;

use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic, Fix};
use ruff_diagnostics::{AutofixKind, Availability, Diagnostic, Fix, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::types::Range;

use crate::checkers::ast::Checker;
use crate::registry::AsRule;

// TODO: document referencing [PEP 585]: https://peps.python.org/pep-0585/
#[violation]
pub struct DeprecatedCollectionType {
pub struct NonPEP585Annotation {
pub name: String,
pub fixable: bool,
}

impl AlwaysAutofixableViolation for DeprecatedCollectionType {
impl Violation for NonPEP585Annotation {
const AUTOFIX: Option<AutofixKind> = Some(AutofixKind::new(Availability::Sometimes));

#[derive_message_formats]
fn message(&self) -> String {
let DeprecatedCollectionType { name } = self;
let NonPEP585Annotation { name, .. } = self;
format!(
"Use `{}` instead of `{}` for type annotations",
name.to_lowercase(),
name,
)
}

fn autofix_title(&self) -> String {
let DeprecatedCollectionType { name } = self;
format!("Replace `{name}` with `{}`", name.to_lowercase())
fn autofix_title_formatter(&self) -> Option<fn(&Self) -> String> {
self.fixable.then_some(|NonPEP585Annotation { name, .. }| {
format!("Replace `{name}` with `{}`", name.to_lowercase())
})
}
}

Expand All @@ -37,13 +40,15 @@ pub fn use_pep585_annotation(checker: &mut Checker, expr: &Expr) {
.resolve_call_path(expr)
.and_then(|call_path| call_path.last().copied())
{
let fixable = !checker.ctx.in_deferred_string_type_definition;
let mut diagnostic = Diagnostic::new(
DeprecatedCollectionType {
NonPEP585Annotation {
name: binding.to_string(),
fixable,
},
Range::from(expr),
);
if checker.patch(diagnostic.kind.rule()) {
if fixable && checker.patch(diagnostic.kind.rule()) {
let binding = binding.to_lowercase();
if checker.ctx.is_builtin(&binding) {
diagnostic.amend(Fix::replacement(
Expand Down
33 changes: 18 additions & 15 deletions crates/ruff/src/rules/pyupgrade/rules/use_pep604_annotation.rs
Original file line number Diff line number Diff line change
@@ -1,25 +1,28 @@
use rustpython_parser::ast::{Constant, Expr, ExprKind, Location, Operator};

use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic, Fix};
use ruff_diagnostics::{AutofixKind, Availability, Diagnostic, Fix, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::helpers::unparse_expr;
use ruff_python_ast::types::Range;

use crate::checkers::ast::Checker;
use crate::registry::AsRule;

// TODO: document referencing [PEP 604]: https://peps.python.org/pep-0604/
#[violation]
pub struct TypingUnion;
pub struct NonPEP604Annotation {
pub fixable: bool,
}

impl Violation for NonPEP604Annotation {
const AUTOFIX: Option<AutofixKind> = Some(AutofixKind::new(Availability::Sometimes));

impl AlwaysAutofixableViolation for TypingUnion {
#[derive_message_formats]
fn message(&self) -> String {
format!("Use `X | Y` for type annotations")
}

fn autofix_title(&self) -> String {
"Convert to `X | Y`".to_string()
fn autofix_title_formatter(&self) -> Option<fn(&Self) -> String> {
self.fixable.then_some(|_| format!("Convert to `X | Y`"))
}
}

Expand Down Expand Up @@ -77,11 +80,6 @@ enum TypingMember {

/// UP007
pub fn use_pep604_annotation(checker: &mut Checker, expr: &Expr, value: &Expr, slice: &Expr) {
// Avoid rewriting forward annotations.
if any_arg_is_str(slice) {
return;
}

let Some(typing_member) = checker.ctx.resolve_call_path(value).as_ref().and_then(|call_path| {
if checker.ctx.match_typing_call_path(call_path, "Optional") {
Some(TypingMember::Optional)
Expand All @@ -94,10 +92,14 @@ pub fn use_pep604_annotation(checker: &mut Checker, expr: &Expr, value: &Expr, s
return;
};

// Avoid fixing forward annotations.
let fixable = !checker.ctx.in_deferred_string_type_definition && !any_arg_is_str(slice);

match typing_member {
TypingMember::Optional => {
let mut diagnostic = Diagnostic::new(TypingUnion, Range::from(expr));
if checker.patch(diagnostic.kind.rule()) {
let mut diagnostic =
Diagnostic::new(NonPEP604Annotation { fixable }, Range::from(expr));
if fixable && checker.patch(diagnostic.kind.rule()) {
diagnostic.amend(Fix::replacement(
unparse_expr(&optional(slice), checker.stylist),
expr.location,
Expand All @@ -107,8 +109,9 @@ pub fn use_pep604_annotation(checker: &mut Checker, expr: &Expr, value: &Expr, s
checker.diagnostics.push(diagnostic);
}
TypingMember::Union => {
let mut diagnostic = Diagnostic::new(TypingUnion, Range::from(expr));
if checker.patch(diagnostic.kind.rule()) {
let mut diagnostic =
Diagnostic::new(NonPEP604Annotation { fixable }, Range::from(expr));
if fixable && checker.patch(diagnostic.kind.rule()) {
match &slice.node {
ExprKind::Slice { .. } => {
// Invalid type annotation.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,13 +35,12 @@ impl CallKind {
}
}

// TODO: document referencing [PEP 604]: https://peps.python.org/pep-0604/
#[violation]
pub struct IsinstanceWithTuple {
pub struct NonPEP604Isinstance {
pub kind: CallKind,
}

impl AlwaysAutofixableViolation for IsinstanceWithTuple {
impl AlwaysAutofixableViolation for NonPEP604Isinstance {
#[derive_message_formats]
fn message(&self) -> String {
format!("Use `X | Y` in `{}` call instead of `(X, Y)`", self.kind)
Expand Down Expand Up @@ -93,7 +92,7 @@ pub fn use_pep604_isinstance(checker: &mut Checker, expr: &Expr, func: &Expr, ar
}

let mut diagnostic =
Diagnostic::new(IsinstanceWithTuple { kind }, Range::from(expr));
Diagnostic::new(NonPEP604Isinstance { kind }, Range::from(expr));
if checker.patch(diagnostic.kind.rule()) {
diagnostic.amend(Fix::replacement(
unparse_expr(&union(elts), checker.stylist),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ source: crates/ruff/src/rules/pyupgrade/mod.rs
expression: diagnostics
---
- kind:
name: DeprecatedCollectionType
name: NonPEP585Annotation
body: "Use `list` instead of `List` for type annotations"
suggestion: "Replace `List` with `list`"
fixable: true
Expand All @@ -23,7 +23,7 @@ expression: diagnostics
column: 20
parent: ~
- kind:
name: DeprecatedCollectionType
name: NonPEP585Annotation
body: "Use `list` instead of `List` for type annotations"
suggestion: "Replace `List` with `list`"
fixable: true
Expand All @@ -43,7 +43,7 @@ expression: diagnostics
column: 13
parent: ~
- kind:
name: DeprecatedCollectionType
name: NonPEP585Annotation
body: "Use `list` instead of `List` for type annotations"
suggestion: "Replace `List` with `list`"
fixable: true
Expand All @@ -63,7 +63,7 @@ expression: diagnostics
column: 15
parent: ~
- kind:
name: DeprecatedCollectionType
name: NonPEP585Annotation
body: "Use `list` instead of `List` for type annotations"
suggestion: "Replace `List` with `list`"
fixable: true
Expand All @@ -83,7 +83,20 @@ expression: diagnostics
column: 14
parent: ~
- kind:
name: DeprecatedCollectionType
name: NonPEP585Annotation
body: "Use `list` instead of `List` for type annotations"
suggestion: ~
fixable: false
location:
row: 29
column: 9
end_location:
row: 29
column: 20
fix: ~
parent: ~
- kind:
name: NonPEP585Annotation
body: "Use `list` instead of `List` for type annotations"
suggestion: "Replace `List` with `list`"
fixable: true
Expand Down

0 comments on commit e9ef26d

Please sign in to comment.