From ab11dd08dfcbe9045fc334b65dee8a5217953196 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Mon, 12 Jun 2023 17:23:39 -0400 Subject: [PATCH] Improve `TypedDict` conversion logic for shadowed builtins and dunder methods (#5038) ## Summary This PR (1) avoids flagging `TypedDict` and `NamedTuple` conversions when attributes are dunder methods, like `__dict__`, and (2) avoids flagging the `A003` shadowed-attribute rule for `TypedDict` classes at all, where it doesn't really apply (since those attributes are only accessed via subscripting anyway). Closes #5027. --- .../test/fixtures/flake8_builtins/A003.py | 9 ++- crates/ruff/src/checkers/ast/mod.rs | 6 +- .../ruff/src/rules/flake8_builtins/helpers.rs | 2 +- .../rules/builtin_attribute_shadowing.rs | 12 ++++ ..._flake8_builtins__tests__A003_A003.py.snap | 6 +- ...sts__A003_A003.py_builtins_ignorelist.snap | 2 +- ...convert_named_tuple_functional_to_class.rs | 33 ++++++----- .../convert_typed_dict_functional_to_class.rs | 55 ++++++++++--------- crates/ruff_python_ast/src/helpers.rs | 2 +- 9 files changed, 78 insertions(+), 49 deletions(-) diff --git a/crates/ruff/resources/test/fixtures/flake8_builtins/A003.py b/crates/ruff/resources/test/fixtures/flake8_builtins/A003.py index 0337a34e95..b971974ef2 100644 --- a/crates/ruff/resources/test/fixtures/flake8_builtins/A003.py +++ b/crates/ruff/resources/test/fixtures/flake8_builtins/A003.py @@ -1,6 +1,6 @@ class MyClass: ImportError = 4 - id = 5 + id: int dir = "/" def __init__(self): @@ -10,3 +10,10 @@ class MyClass: def str(self): pass + + +from typing import TypedDict + + +class MyClass(TypedDict): + id: int diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs index e166718bdb..80a5fb86e3 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/crates/ruff/src/checkers/ast/mod.rs @@ -656,10 +656,11 @@ where pyupgrade::rules::yield_in_for_loop(self, stmt); } - if self.semantic_model.scope().kind.is_class() { + if let ScopeKind::Class(class_def) = self.semantic_model.scope().kind { if self.enabled(Rule::BuiltinAttributeShadowing) { flake8_builtins::rules::builtin_attribute_shadowing( self, + class_def, name, AnyShadowing::from(stmt), ); @@ -2369,10 +2370,11 @@ where } } - if self.semantic_model.scope().kind.is_class() { + if let ScopeKind::Class(class_def) = self.semantic_model.scope().kind { if self.enabled(Rule::BuiltinAttributeShadowing) { flake8_builtins::rules::builtin_attribute_shadowing( self, + class_def, id, AnyShadowing::from(expr), ); diff --git a/crates/ruff/src/rules/flake8_builtins/helpers.rs b/crates/ruff/src/rules/flake8_builtins/helpers.rs index 1f1eb0f3ba..d350c11447 100644 --- a/crates/ruff/src/rules/flake8_builtins/helpers.rs +++ b/crates/ruff/src/rules/flake8_builtins/helpers.rs @@ -1,9 +1,9 @@ +use ruff_text_size::TextRange; use rustpython_parser::ast::{Excepthandler, Expr, Ranged, Stmt}; use ruff_python_ast::helpers::identifier_range; use ruff_python_ast::source_code::Locator; use ruff_python_stdlib::builtins::BUILTINS; -use ruff_text_size::TextRange; pub(super) fn shadows_builtin(name: &str, ignorelist: &[String]) -> bool { BUILTINS.contains(&name) && ignorelist.iter().all(|ignore| ignore != name) diff --git a/crates/ruff/src/rules/flake8_builtins/rules/builtin_attribute_shadowing.rs b/crates/ruff/src/rules/flake8_builtins/rules/builtin_attribute_shadowing.rs index 701fa4edd3..ec7e9fb4e6 100644 --- a/crates/ruff/src/rules/flake8_builtins/rules/builtin_attribute_shadowing.rs +++ b/crates/ruff/src/rules/flake8_builtins/rules/builtin_attribute_shadowing.rs @@ -1,6 +1,7 @@ use ruff_diagnostics::Diagnostic; use ruff_diagnostics::Violation; use ruff_macros::{derive_message_formats, violation}; +use rustpython_parser::ast; use crate::checkers::ast::Checker; @@ -64,10 +65,21 @@ impl Violation for BuiltinAttributeShadowing { /// A003 pub(crate) fn builtin_attribute_shadowing( checker: &mut Checker, + class_def: &ast::StmtClassDef, name: &str, shadowing: AnyShadowing, ) { if shadows_builtin(name, &checker.settings.flake8_builtins.builtins_ignorelist) { + // Ignore shadowing within `TypedDict` definitions, since these are only accessible through + // subscripting and not through attribute access. + if class_def.bases.iter().any(|base| { + checker + .semantic_model() + .match_typing_expr(base, "TypedDict") + }) { + return; + } + checker.diagnostics.push(Diagnostic::new( BuiltinAttributeShadowing { name: name.to_string(), diff --git a/crates/ruff/src/rules/flake8_builtins/snapshots/ruff__rules__flake8_builtins__tests__A003_A003.py.snap b/crates/ruff/src/rules/flake8_builtins/snapshots/ruff__rules__flake8_builtins__tests__A003_A003.py.snap index 8c8a70b623..48c9dfa217 100644 --- a/crates/ruff/src/rules/flake8_builtins/snapshots/ruff__rules__flake8_builtins__tests__A003_A003.py.snap +++ b/crates/ruff/src/rules/flake8_builtins/snapshots/ruff__rules__flake8_builtins__tests__A003_A003.py.snap @@ -6,7 +6,7 @@ A003.py:2:5: A003 Class attribute `ImportError` is shadowing a Python builtin 1 | class MyClass: 2 | ImportError = 4 | ^^^^^^^^^^^ A003 -3 | id = 5 +3 | id: int 4 | dir = "/" | @@ -14,7 +14,7 @@ A003.py:3:5: A003 Class attribute `id` is shadowing a Python builtin | 1 | class MyClass: 2 | ImportError = 4 -3 | id = 5 +3 | id: int | ^^ A003 4 | dir = "/" | @@ -22,7 +22,7 @@ A003.py:3:5: A003 Class attribute `id` is shadowing a Python builtin A003.py:4:5: A003 Class attribute `dir` is shadowing a Python builtin | 2 | ImportError = 4 -3 | id = 5 +3 | id: int 4 | dir = "/" | ^^^ A003 5 | diff --git a/crates/ruff/src/rules/flake8_builtins/snapshots/ruff__rules__flake8_builtins__tests__A003_A003.py_builtins_ignorelist.snap b/crates/ruff/src/rules/flake8_builtins/snapshots/ruff__rules__flake8_builtins__tests__A003_A003.py_builtins_ignorelist.snap index 3c509b2fb8..342f14a9a1 100644 --- a/crates/ruff/src/rules/flake8_builtins/snapshots/ruff__rules__flake8_builtins__tests__A003_A003.py_builtins_ignorelist.snap +++ b/crates/ruff/src/rules/flake8_builtins/snapshots/ruff__rules__flake8_builtins__tests__A003_A003.py_builtins_ignorelist.snap @@ -6,7 +6,7 @@ A003.py:2:5: A003 Class attribute `ImportError` is shadowing a Python builtin 1 | class MyClass: 2 | ImportError = 4 | ^^^^^^^^^^^ A003 -3 | id = 5 +3 | id: int 4 | dir = "/" | diff --git a/crates/ruff/src/rules/pyupgrade/rules/convert_named_tuple_functional_to_class.rs b/crates/ruff/src/rules/pyupgrade/rules/convert_named_tuple_functional_to_class.rs index d2de8fb2b8..f9f86618f8 100644 --- a/crates/ruff/src/rules/pyupgrade/rules/convert_named_tuple_functional_to_class.rs +++ b/crates/ruff/src/rules/pyupgrade/rules/convert_named_tuple_functional_to_class.rs @@ -5,6 +5,7 @@ use rustpython_parser::ast::{self, Constant, Expr, ExprContext, Keyword, Ranged, use ruff_diagnostics::{AutofixKind, Diagnostic, Edit, Fix, Violation}; use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast::helpers::is_dunder; use ruff_python_ast::source_code::Generator; use ruff_python_semantic::model::SemanticModel; use ruff_python_stdlib::identifiers::is_identifier; @@ -66,19 +67,21 @@ fn create_property_assignment_stmt( annotation: &Expr, value: Option<&Expr>, ) -> Stmt { - let node = ast::ExprName { - id: property.into(), - ctx: ExprContext::Load, - range: TextRange::default(), - }; - let node1 = ast::StmtAnnAssign { - target: Box::new(node.into()), + ast::StmtAnnAssign { + target: Box::new( + ast::ExprName { + id: property.into(), + ctx: ExprContext::Load, + range: TextRange::default(), + } + .into(), + ), annotation: Box::new(annotation.clone()), value: value.map(|value| Box::new(value.clone())), simple: true, range: TextRange::default(), - }; - node1.into() + } + .into() } /// Match the `defaults` keyword in a `NamedTuple(...)` call. @@ -140,6 +143,9 @@ fn create_properties_from_args(args: &[Expr], defaults: &[Expr]) -> Result Result, base_class: &Expr) -> Stmt { - let node = ast::StmtClassDef { + ast::StmtClassDef { name: typename.into(), bases: vec![base_class.clone()], keywords: vec![], body, decorator_list: vec![], range: TextRange::default(), - }; - node.into() + } + .into() } /// Generate a `Fix` to convert a `NamedTuple` assignment to a class definition. @@ -169,8 +175,7 @@ fn convert_to_class( base_class: &Expr, generator: Generator, ) -> Fix { - #[allow(deprecated)] - Fix::unspecified(Edit::range_replacement( + Fix::suggested(Edit::range_replacement( generator.stmt(&create_class_def_stmt(typename, body, base_class)), stmt.range(), )) diff --git a/crates/ruff/src/rules/pyupgrade/rules/convert_typed_dict_functional_to_class.rs b/crates/ruff/src/rules/pyupgrade/rules/convert_typed_dict_functional_to_class.rs index e1906fa880..50ae7304f4 100644 --- a/crates/ruff/src/rules/pyupgrade/rules/convert_typed_dict_functional_to_class.rs +++ b/crates/ruff/src/rules/pyupgrade/rules/convert_typed_dict_functional_to_class.rs @@ -5,6 +5,7 @@ use rustpython_parser::ast::{self, Constant, Expr, ExprContext, Keyword, Ranged, use ruff_diagnostics::{AutofixKind, Diagnostic, Edit, Fix, Violation}; use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast::helpers::is_dunder; use ruff_python_ast::source_code::Generator; use ruff_python_semantic::model::SemanticModel; use ruff_python_stdlib::identifiers::is_identifier; @@ -62,20 +63,21 @@ fn match_typed_dict_assign<'a>( /// Generate a `Stmt::AnnAssign` representing the provided property /// definition. fn create_property_assignment_stmt(property: &str, annotation: &Expr) -> Stmt { - let node = annotation.clone(); - let node1 = ast::ExprName { - id: property.into(), - ctx: ExprContext::Load, - range: TextRange::default(), - }; - let node2 = ast::StmtAnnAssign { - target: Box::new(node1.into()), - annotation: Box::new(node), + ast::StmtAnnAssign { + target: Box::new( + ast::ExprName { + id: property.into(), + ctx: ExprContext::Load, + range: TextRange::default(), + } + .into(), + ), + annotation: Box::new(annotation.clone()), value: None, simple: true, range: TextRange::default(), - }; - node2.into() + } + .into() } /// Generate a `StmtKind:ClassDef` statement based on the provided body, @@ -90,15 +92,15 @@ fn create_class_def_stmt( Some(keyword) => vec![keyword.clone()], None => vec![], }; - let node = ast::StmtClassDef { + ast::StmtClassDef { name: class_name.into(), bases: vec![base_class.clone()], keywords, body, decorator_list: vec![], range: TextRange::default(), - }; - node.into() + } + .into() } fn properties_from_dict_literal(keys: &[Option], values: &[Expr]) -> Result> { @@ -116,11 +118,13 @@ fn properties_from_dict_literal(keys: &[Option], values: &[Expr]) -> Resul value: Constant::Str(property), .. })) => { - if is_identifier(property) { - Ok(create_property_assignment_stmt(property, value)) - } else { - bail!("Property name is not valid identifier: {}", property) + if !is_identifier(property) { + bail!("Invalid property name: {}", property) } + if is_dunder(property) { + bail!("Cannot use dunder property name: {}", property) + } + Ok(create_property_assignment_stmt(property, value)) } _ => bail!("Expected `key` to be `Constant::Str`"), }) @@ -170,12 +174,12 @@ fn properties_from_keywords(keywords: &[Keyword]) -> Result> { // TypedDict('name', {'a': int}, total=True) // ``` fn match_total_from_only_keyword(keywords: &[Keyword]) -> Option<&Keyword> { - let keyword = keywords.get(0)?; - let arg = &keyword.arg.as_ref()?; - match arg.as_str() { - "total" => Some(keyword), - _ => None, - } + keywords.iter().find(|keyword| { + let Some(arg) = &keyword.arg else { + return false + }; + arg.as_str() == "total" + }) } fn match_properties_and_total<'a>( @@ -219,8 +223,7 @@ fn convert_to_class( base_class: &Expr, generator: Generator, ) -> Fix { - #[allow(deprecated)] - Fix::unspecified(Edit::range_replacement( + Fix::suggested(Edit::range_replacement( generator.stmt(&create_class_def_stmt( class_name, body, diff --git a/crates/ruff_python_ast/src/helpers.rs b/crates/ruff_python_ast/src/helpers.rs index 0f9ee007db..c9dce0160c 100644 --- a/crates/ruff_python_ast/src/helpers.rs +++ b/crates/ruff_python_ast/src/helpers.rs @@ -546,7 +546,7 @@ where body.iter().any(|stmt| any_over_stmt(stmt, func)) } -fn is_dunder(id: &str) -> bool { +pub fn is_dunder(id: &str) -> bool { id.starts_with("__") && id.ends_with("__") }