From 67f304cdc6a04622f416146f81327f72feb1fc51 Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Tue, 23 Jul 2024 18:04:20 +0100 Subject: [PATCH 1/6] Add failing test --- .../ruff_linter/resources/test/fixtures/numpy/NPY201_2.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/crates/ruff_linter/resources/test/fixtures/numpy/NPY201_2.py b/crates/ruff_linter/resources/test/fixtures/numpy/NPY201_2.py index 0f9262b7eb558..838cfb4547d9d 100644 --- a/crates/ruff_linter/resources/test/fixtures/numpy/NPY201_2.py +++ b/crates/ruff_linter/resources/test/fixtures/numpy/NPY201_2.py @@ -56,3 +56,10 @@ def func(): np.ComplexWarning np.compare_chararrays + + try: + exc = np.exceptions.ComplexWarning + except AttributeError: + exc = np.ComplexWarning + + raise exc From 1b0137e801c75be3e1939dddb7b1441f82efd219 Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Tue, 23 Jul 2024 18:53:37 +0100 Subject: [PATCH 2/6] Ignore deprecated attribute accesses in `except AttributeError` blocks --- .../resources/test/fixtures/numpy/NPY201_2.py | 13 ++- crates/ruff_linter/src/checkers/ast/mod.rs | 38 +++---- .../numpy/rules/numpy_2_0_deprecation.rs | 107 +++++++++++++++++- ...tests__numpy2-deprecation_NPY201_2.py.snap | 47 ++++++++ crates/ruff_python_semantic/src/binding.rs | 23 +++- 5 files changed, 199 insertions(+), 29 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/numpy/NPY201_2.py b/crates/ruff_linter/resources/test/fixtures/numpy/NPY201_2.py index 838cfb4547d9d..e43a6611c446a 100644 --- a/crates/ruff_linter/resources/test/fixtures/numpy/NPY201_2.py +++ b/crates/ruff_linter/resources/test/fixtures/numpy/NPY201_2.py @@ -57,9 +57,20 @@ def func(): np.compare_chararrays + try: + np.all([True, True]) + except TypeError: + np.alltrue([True, True]) # Should emit a warning here (`except TypeError`, not `except AttributeError`) + + try: + np.anyyyy([True, True]) + except AttributeError: + np.sometrue([True, True]) # Should emit a warning here + # (must have an attribute access of the undeprecated name in the `try` body for it to be ignored) + try: exc = np.exceptions.ComplexWarning except AttributeError: - exc = np.ComplexWarning + exc = np.ComplexWarning # `except AttributeError` means that this is okay raise exc diff --git a/crates/ruff_linter/src/checkers/ast/mod.rs b/crates/ruff_linter/src/checkers/ast/mod.rs index e92a41aff6a82..70c22a952be7e 100644 --- a/crates/ruff_linter/src/checkers/ast/mod.rs +++ b/crates/ruff_linter/src/checkers/ast/mod.rs @@ -33,9 +33,7 @@ use log::debug; use ruff_diagnostics::{Diagnostic, IsolationLevel}; use ruff_notebook::{CellOffsets, NotebookIndex}; -use ruff_python_ast::helpers::{ - collect_import_from_member, extract_handled_exceptions, is_docstring_stmt, to_module_path, -}; +use ruff_python_ast::helpers::{collect_import_from_member, is_docstring_stmt, to_module_path}; use ruff_python_ast::identifier::Identifier; use ruff_python_ast::name::QualifiedName; use ruff_python_ast::str::Quote; @@ -834,32 +832,22 @@ impl<'a> Visitor<'a> for Checker<'a> { self.semantic.pop_scope(); self.visit_expr(name); } - Stmt::Try(ast::StmtTry { - body, - handlers, - orelse, - finalbody, - .. - }) => { - let mut handled_exceptions = Exceptions::empty(); - for type_ in extract_handled_exceptions(handlers) { - if let Some(builtins_name) = self.semantic.resolve_builtin_symbol(type_) { - match builtins_name { - "NameError" => handled_exceptions |= Exceptions::NAME_ERROR, - "ModuleNotFoundError" => { - handled_exceptions |= Exceptions::MODULE_NOT_FOUND_ERROR; - } - "ImportError" => handled_exceptions |= Exceptions::IMPORT_ERROR, - _ => {} - } - } - } - + Stmt::Try( + try_node @ ast::StmtTry { + body, + handlers, + orelse, + finalbody, + .. + }, + ) => { // Iterate over the `body`, then the `handlers`, then the `orelse`, then the // `finalbody`, but treat the body and the `orelse` as a single branch for // flow analysis purposes. let branch = self.semantic.push_branch(); - self.semantic.handled_exceptions.push(handled_exceptions); + self.semantic + .handled_exceptions + .push(Exceptions::from_try_stmt(try_node, &self.semantic)); self.visit_body(body); self.semantic.handled_exceptions.pop(); self.semantic.pop_branch(); diff --git a/crates/ruff_linter/src/rules/numpy/rules/numpy_2_0_deprecation.rs b/crates/ruff_linter/src/rules/numpy/rules/numpy_2_0_deprecation.rs index eeded38dde932..35ceca201102a 100644 --- a/crates/ruff_linter/src/rules/numpy/rules/numpy_2_0_deprecation.rs +++ b/crates/ruff_linter/src/rules/numpy/rules/numpy_2_0_deprecation.rs @@ -1,7 +1,9 @@ use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation}; use ruff_macros::{derive_message_formats, violation}; -use ruff_python_ast::Expr; -use ruff_python_semantic::Modules; +use ruff_python_ast::name::{QualifiedName, QualifiedNameBuilder}; +use ruff_python_ast::visitor::Visitor; +use ruff_python_ast::{self as ast, Expr}; +use ruff_python_semantic::{Exceptions, Modules, SemanticModel}; use ruff_text_size::Ranged; use crate::checkers::ast::Checker; @@ -665,6 +667,10 @@ pub(crate) fn numpy_2_0_deprecation(checker: &mut Checker, expr: &Expr) { _ => return, }; + if is_guarded_by_try_except(expr, &replacement, semantic) { + return; + } + let mut diagnostic = Diagnostic::new( Numpy2Deprecation { existing: replacement.existing.to_string(), @@ -701,3 +707,100 @@ pub(crate) fn numpy_2_0_deprecation(checker: &mut Checker, expr: &Expr) { }; checker.diagnostics.push(diagnostic); } + +fn is_guarded_by_try_except( + expr: &Expr, + replacement: &Replacement, + semantic: &SemanticModel, +) -> bool { + if !semantic.in_exception_handler() { + return false; + } + + match expr { + Expr::Attribute(_) => { + let Some(try_node) = find_try_node(semantic) else { + return false; + }; + let handled_exceptions = Exceptions::from_try_stmt(try_node, semantic); + if !handled_exceptions.contains(Exceptions::ATTRIBUTE_ERROR) { + return false; + } + try_block_contains_undeprecated_attribute(try_node, &replacement.details, semantic) + } + _ => false, + } +} + +fn find_try_node<'a>(semantic: &'a SemanticModel) -> Option<&'a ast::StmtTry> { + semantic + .current_statements() + .find_map(|stmt| stmt.as_try_stmt()) +} + +fn undeprecated_qualified_name<'a>(path: &'a str, name: &'a str) -> QualifiedName<'a> { + let mut builder = QualifiedNameBuilder::default(); + for part in path.split('.') { + builder.push(part); + } + builder.push(name); + builder.build() +} + +fn try_block_contains_undeprecated_attribute( + try_node: &ast::StmtTry, + replacement_details: &Details, + semantic: &SemanticModel, +) -> bool { + let Details::AutoImport { + path, + name, + compatibility: _, + } = replacement_details + else { + return false; + }; + let undeprecated_qualified_name = undeprecated_qualified_name(path, name); + let mut attribute_searcher = AttributeSearcher::new(undeprecated_qualified_name, semantic); + for stmt in &try_node.body { + attribute_searcher.visit_stmt(stmt); + if attribute_searcher.found_attribute { + return true; + } + } + false +} + +struct AttributeSearcher<'a> { + attribute_to_find: QualifiedName<'a>, + semantic: &'a SemanticModel<'a>, + found_attribute: bool, +} + +impl<'a> AttributeSearcher<'a> { + fn new(attribute_to_find: QualifiedName<'a>, semantic: &'a SemanticModel<'a>) -> Self { + Self { + attribute_to_find, + semantic, + found_attribute: false, + } + } +} + +impl Visitor<'_> for AttributeSearcher<'_> { + fn visit_expr(&mut self, expr: &'_ Expr) { + if self.found_attribute { + return; + } + if expr.is_attribute_expr() + && self + .semantic + .resolve_qualified_name(expr) + .is_some_and(|qualified_name| qualified_name == self.attribute_to_find) + { + self.found_attribute = true; + return; + } + ast::visitor::walk_expr(self, expr); + } +} diff --git a/crates/ruff_linter/src/rules/numpy/snapshots/ruff_linter__rules__numpy__tests__numpy2-deprecation_NPY201_2.py.snap b/crates/ruff_linter/src/rules/numpy/snapshots/ruff_linter__rules__numpy__tests__numpy2-deprecation_NPY201_2.py.snap index ce41036c18cff..56be229615aa6 100644 --- a/crates/ruff_linter/src/rules/numpy/snapshots/ruff_linter__rules__numpy__tests__numpy2-deprecation_NPY201_2.py.snap +++ b/crates/ruff_linter/src/rules/numpy/snapshots/ruff_linter__rules__numpy__tests__numpy2-deprecation_NPY201_2.py.snap @@ -482,6 +482,7 @@ NPY201_2.py:56:5: NPY201 [*] `np.ComplexWarning` will be removed in NumPy 2.0. U 57 |+ ComplexWarning 57 58 | 58 59 | np.compare_chararrays +59 60 | NPY201_2.py:58:5: NPY201 [*] `np.compare_chararrays` will be removed in NumPy 2.0. Use `numpy.char.compare_chararrays` instead. | @@ -489,6 +490,8 @@ NPY201_2.py:58:5: NPY201 [*] `np.compare_chararrays` will be removed in NumPy 2. 57 | 58 | np.compare_chararrays | ^^^^^^^^^^^^^^^^^^^^^ NPY201 +59 | +60 | try: | = help: Replace with `numpy.char.compare_chararrays` @@ -503,3 +506,47 @@ NPY201_2.py:58:5: NPY201 [*] `np.compare_chararrays` will be removed in NumPy 2. 57 58 | 58 |- np.compare_chararrays 59 |+ compare_chararrays +59 60 | +60 61 | try: +61 62 | np.all([True, True]) + +NPY201_2.py:63:9: NPY201 [*] `np.alltrue` will be removed in NumPy 2.0. Use `numpy.all` instead. + | +61 | np.all([True, True]) +62 | except TypeError: +63 | np.alltrue([True, True]) # Should emit a warning here (`except TypeError`, not `except AttributeError`) + | ^^^^^^^^^^ NPY201 +64 | +65 | try: + | + = help: Replace with `numpy.all` + +ℹ Safe fix +60 60 | try: +61 61 | np.all([True, True]) +62 62 | except TypeError: +63 |- np.alltrue([True, True]) # Should emit a warning here (`except TypeError`, not `except AttributeError`) + 63 |+ np.all([True, True]) # Should emit a warning here (`except TypeError`, not `except AttributeError`) +64 64 | +65 65 | try: +66 66 | np.anyyyy([True, True]) + +NPY201_2.py:68:9: NPY201 [*] `np.sometrue` will be removed in NumPy 2.0. Use `numpy.any` instead. + | +66 | np.anyyyy([True, True]) +67 | except AttributeError: +68 | np.sometrue([True, True]) # Should emit a warning here + | ^^^^^^^^^^^ NPY201 +69 | # (must have an attribute access of the undeprecated name in the `try` body for it to be ignored) + | + = help: Replace with `numpy.any` + +ℹ Safe fix +65 65 | try: +66 66 | np.anyyyy([True, True]) +67 67 | except AttributeError: +68 |- np.sometrue([True, True]) # Should emit a warning here + 68 |+ np.any([True, True]) # Should emit a warning here +69 69 | # (must have an attribute access of the undeprecated name in the `try` body for it to be ignored) +70 70 | +71 71 | try: diff --git a/crates/ruff_python_semantic/src/binding.rs b/crates/ruff_python_semantic/src/binding.rs index 3ff36bd06ca41..30bb330f4cf54 100644 --- a/crates/ruff_python_semantic/src/binding.rs +++ b/crates/ruff_python_semantic/src/binding.rs @@ -5,8 +5,9 @@ use bitflags::bitflags; use crate::all::DunderAllName; use ruff_index::{newtype_index, IndexSlice, IndexVec}; +use ruff_python_ast::helpers::extract_handled_exceptions; use ruff_python_ast::name::QualifiedName; -use ruff_python_ast::Stmt; +use ruff_python_ast::{self as ast, Stmt}; use ruff_source_file::Locator; use ruff_text_size::{Ranged, TextRange}; @@ -579,6 +580,26 @@ bitflags! { const NAME_ERROR = 0b0000_0001; const MODULE_NOT_FOUND_ERROR = 0b0000_0010; const IMPORT_ERROR = 0b0000_0100; + const ATTRIBUTE_ERROR = 0b000_100; + } +} + +impl Exceptions { + pub fn from_try_stmt( + ast::StmtTry { handlers, .. }: &ast::StmtTry, + semantic: &SemanticModel, + ) -> Self { + let mut handled_exceptions = Self::empty(); + for type_ in extract_handled_exceptions(handlers) { + handled_exceptions |= match semantic.resolve_builtin_symbol(type_) { + Some("NameError") => Self::NAME_ERROR, + Some("ModuleNotFoundError") => Self::MODULE_NOT_FOUND_ERROR, + Some("ImportError") => Self::IMPORT_ERROR, + Some("AttributeError") => Self::ATTRIBUTE_ERROR, + _ => continue, + } + } + handled_exceptions } } From bc79d9c4d30e6bb1d89dcb026a071fd010c1078d Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Wed, 24 Jul 2024 12:53:04 +0100 Subject: [PATCH 3/6] Ignore deprecated imports in `except ImportError` blocks --- .../resources/test/fixtures/numpy/NPY201.py | 17 +++ crates/ruff_linter/src/checkers/ast/mod.rs | 6 +- .../numpy/rules/numpy_2_0_deprecation.rs | 115 ++++++++++++++++-- ...__tests__numpy2-deprecation_NPY201.py.snap | 5 + crates/ruff_python_semantic/src/binding.rs | 24 ++++ crates/ruff_python_semantic/src/model.rs | 16 ++- 6 files changed, 169 insertions(+), 14 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/numpy/NPY201.py b/crates/ruff_linter/resources/test/fixtures/numpy/NPY201.py index 01846b92b6dd0..5fad89e5294de 100644 --- a/crates/ruff_linter/resources/test/fixtures/numpy/NPY201.py +++ b/crates/ruff_linter/resources/test/fixtures/numpy/NPY201.py @@ -70,3 +70,20 @@ def func(): np.lookfor np.NAN + + try: + from numpy.lib.npyio import DataSource + except ImportError: + from numpy import DataSource + + DataSource("foo").abspath() # fine (`except ImportError` branch) + + try: + from numpy.rec import format_parser + from numpy import clongdouble + except ModuleNotFoundError: + from numpy import format_parser + from numpy import longcomplex as clongdouble + + format_parser("foo") # fine (`except ModuleNotFoundError` branch) + clongdouble(42) # fine (`except ModuleNotFoundError` branch) diff --git a/crates/ruff_linter/src/checkers/ast/mod.rs b/crates/ruff_linter/src/checkers/ast/mod.rs index 70c22a952be7e..fa2a4f2cfcae2 100644 --- a/crates/ruff_linter/src/checkers/ast/mod.rs +++ b/crates/ruff_linter/src/checkers/ast/mod.rs @@ -1825,7 +1825,7 @@ impl<'a> Checker<'a> { name: &'a str, range: TextRange, kind: BindingKind<'a>, - flags: BindingFlags, + mut flags: BindingFlags, ) -> BindingId { // Determine the scope to which the binding belongs. // Per [PEP 572](https://peps.python.org/pep-0572/#scope-of-the-target), named @@ -1841,6 +1841,10 @@ impl<'a> Checker<'a> { self.semantic.scope_id }; + if self.semantic.in_exception_handler() { + flags |= BindingFlags::IN_EXCEPT_HANDLER; + } + // Create the `Binding`. let binding_id = self.semantic.push_binding(range, kind, flags); diff --git a/crates/ruff_linter/src/rules/numpy/rules/numpy_2_0_deprecation.rs b/crates/ruff_linter/src/rules/numpy/rules/numpy_2_0_deprecation.rs index 35ceca201102a..4277d4da9e572 100644 --- a/crates/ruff_linter/src/rules/numpy/rules/numpy_2_0_deprecation.rs +++ b/crates/ruff_linter/src/rules/numpy/rules/numpy_2_0_deprecation.rs @@ -3,7 +3,7 @@ use ruff_macros::{derive_message_formats, violation}; use ruff_python_ast::name::{QualifiedName, QualifiedNameBuilder}; use ruff_python_ast::visitor::Visitor; use ruff_python_ast::{self as ast, Expr}; -use ruff_python_semantic::{Exceptions, Modules, SemanticModel}; +use ruff_python_semantic::{Exceptions, Modules, NodeId, SemanticModel}; use ruff_text_size::Ranged; use crate::checkers::ast::Checker; @@ -713,29 +713,63 @@ fn is_guarded_by_try_except( replacement: &Replacement, semantic: &SemanticModel, ) -> bool { - if !semantic.in_exception_handler() { - return false; - } - match expr { Expr::Attribute(_) => { - let Some(try_node) = find_try_node(semantic) else { + if !semantic.in_exception_handler() { + return false; + } + let Some(try_node) = semantic + .current_statements() + .find_map(|stmt| stmt.as_try_stmt()) + else { return false; }; - let handled_exceptions = Exceptions::from_try_stmt(try_node, semantic); - if !handled_exceptions.contains(Exceptions::ATTRIBUTE_ERROR) { + let suspended_exceptions = Exceptions::from_try_stmt(try_node, semantic); + if !suspended_exceptions.contains(Exceptions::ATTRIBUTE_ERROR) { return false; } try_block_contains_undeprecated_attribute(try_node, &replacement.details, semantic) } + Expr::Name(ast::ExprName { id, .. }) => { + let Some(binding_id) = semantic.lookup_symbol(id.as_str()) else { + return false; + }; + let binding = semantic.binding(binding_id); + if !binding.is_external() { + return false; + } + if !binding.in_exception_handler() { + return false; + } + let Some(try_node) = binding + .source + .and_then(|import_id| try_node_parent_of_import_stmt(import_id, semantic)) + else { + return false; + }; + let suspended_exceptions = Exceptions::from_try_stmt(try_node, semantic); + if !suspended_exceptions + .intersects(Exceptions::IMPORT_ERROR | Exceptions::MODULE_NOT_FOUND_ERROR) + { + return false; + } + try_block_contains_undeprecated_import(try_node, &replacement.details) + } _ => false, } } -fn find_try_node<'a>(semantic: &'a SemanticModel) -> Option<&'a ast::StmtTry> { - semantic - .current_statements() - .find_map(|stmt| stmt.as_try_stmt()) +fn try_node_parent_of_import_stmt<'a>( + import_id: NodeId, + semantic: &'a SemanticModel, +) -> Option<&'a ast::StmtTry> { + let mut node_id = import_id; + loop { + node_id = semantic.parent_statement_id(node_id)?; + if let ast::Stmt::Try(try_node) = semantic.statement(node_id) { + return Some(try_node); + } + } } fn undeprecated_qualified_name<'a>(path: &'a str, name: &'a str) -> QualifiedName<'a> { @@ -804,3 +838,60 @@ impl Visitor<'_> for AttributeSearcher<'_> { ast::visitor::walk_expr(self, expr); } } + +fn try_block_contains_undeprecated_import( + try_node: &ast::StmtTry, + replacement_details: &Details, +) -> bool { + let Details::AutoImport { + path, + name, + compatibility: _, + } = replacement_details + else { + return false; + }; + let mut import_searcher = ImportSearcher::new(path, name); + for stmt in &try_node.body { + import_searcher.visit_stmt(stmt); + if import_searcher.found_import { + return true; + } + } + false +} + +struct ImportSearcher<'a> { + module: &'a str, + name: &'a str, + found_import: bool, +} + +impl<'a> ImportSearcher<'a> { + fn new(module: &'a str, name: &'a str) -> Self { + Self { + module, + name, + found_import: false, + } + } +} + +impl Visitor<'_> for ImportSearcher<'_> { + fn visit_stmt(&mut self, stmt: &ast::Stmt) { + if self.found_import { + return; + } + if let ast::Stmt::ImportFrom(ast::StmtImportFrom { module, names, .. }) = stmt { + if module.as_ref().is_some_and(|module| module == self.module) + && names + .iter() + .any(|ast::Alias { name, .. }| name == self.name) + { + self.found_import = true; + return; + } + } + ast::visitor::walk_stmt(self, stmt); + } +} diff --git a/crates/ruff_linter/src/rules/numpy/snapshots/ruff_linter__rules__numpy__tests__numpy2-deprecation_NPY201.py.snap b/crates/ruff_linter/src/rules/numpy/snapshots/ruff_linter__rules__numpy__tests__numpy2-deprecation_NPY201.py.snap index 1799f3ef12f0a..6941c9efc9aff 100644 --- a/crates/ruff_linter/src/rules/numpy/snapshots/ruff_linter__rules__numpy__tests__numpy2-deprecation_NPY201.py.snap +++ b/crates/ruff_linter/src/rules/numpy/snapshots/ruff_linter__rules__numpy__tests__numpy2-deprecation_NPY201.py.snap @@ -570,6 +570,8 @@ NPY201.py:72:5: NPY201 [*] `np.NAN` will be removed in NumPy 2.0. Use `numpy.nan 71 | 72 | np.NAN | ^^^^^^ NPY201 +73 | +74 | try: | = help: Replace with `numpy.nan` @@ -579,3 +581,6 @@ NPY201.py:72:5: NPY201 [*] `np.NAN` will be removed in NumPy 2.0. Use `numpy.nan 71 71 | 72 |- np.NAN 72 |+ np.nan +73 73 | +74 74 | try: +75 75 | from numpy.lib.npyio import DataSource diff --git a/crates/ruff_python_semantic/src/binding.rs b/crates/ruff_python_semantic/src/binding.rs index 30bb330f4cf54..d4cc059088ca8 100644 --- a/crates/ruff_python_semantic/src/binding.rs +++ b/crates/ruff_python_semantic/src/binding.rs @@ -115,6 +115,18 @@ impl<'a> Binding<'a> { self.flags.contains(BindingFlags::PRIVATE_DECLARATION) } + /// Return `true` if this [`Binding`] took place inside an exception handler, + /// e.g. `y` in: + /// ```python + /// try: + /// x = 42 + /// except RuntimeError: + /// y = 42 + /// ``` + pub const fn in_exception_handler(&self) -> bool { + self.flags.contains(BindingFlags::IN_EXCEPT_HANDLER) + } + /// Return `true` if this binding "redefines" the given binding, as per Pyflake's definition of /// redefinition. pub fn redefines(&self, existing: &Binding) -> bool { @@ -334,6 +346,18 @@ bitflags! { /// (x, y) = 1, 2 /// ``` const UNPACKED_ASSIGNMENT = 1 << 9; + + /// The binding took place inside an exception handling. + /// + /// For example, the `x` binding in the following example + /// would *not* have this flag set, but the `y` binding *would*: + /// ```python + /// try: + /// x = 42 + /// except RuntimeError: + /// y = 42 + /// ``` + const IN_EXCEPT_HANDLER = 1 << 10; } } diff --git a/crates/ruff_python_semantic/src/model.rs b/crates/ruff_python_semantic/src/model.rs index 3fe72f4658322..ff1761b7b9479 100644 --- a/crates/ruff_python_semantic/src/model.rs +++ b/crates/ruff_python_semantic/src/model.rs @@ -124,7 +124,21 @@ pub struct SemanticModel<'a> { /// Modules that have been seen by the semantic model. pub seen: Modules, - /// Exceptions that have been handled by the current scope. + /// Exceptions that are handled by the current `try` block. + /// + /// For example, if we're visiting the `x = 1` assignment below, + /// `AttributeError` is considered to be a "handled exception", + /// but `TypeError` is not: + /// + /// ```py + /// try: + /// try: + /// foo() + /// except TypeError: + /// pass + /// except AttributeError: + /// pass + /// ``` pub handled_exceptions: Vec, /// Map from [`ast::ExprName`] node (represented as a [`NameId`]) to the [`Binding`] to which From a2a6aaf8e34feba2f4dd555bb70178400d2c1a64 Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Wed, 24 Jul 2024 13:27:30 +0100 Subject: [PATCH 4/6] docs --- .../numpy/rules/numpy_2_0_deprecation.rs | 70 +++++++++++++++---- 1 file changed, 56 insertions(+), 14 deletions(-) diff --git a/crates/ruff_linter/src/rules/numpy/rules/numpy_2_0_deprecation.rs b/crates/ruff_linter/src/rules/numpy/rules/numpy_2_0_deprecation.rs index 4277d4da9e572..587b58e9f87dc 100644 --- a/crates/ruff_linter/src/rules/numpy/rules/numpy_2_0_deprecation.rs +++ b/crates/ruff_linter/src/rules/numpy/rules/numpy_2_0_deprecation.rs @@ -708,6 +708,38 @@ pub(crate) fn numpy_2_0_deprecation(checker: &mut Checker, expr: &Expr) { checker.diagnostics.push(diagnostic); } +/// Ignore attempts to access a `numpy` member via its deprecated name +/// if the access takes place in an `except` block that provides compatibility +/// with older numpy versions. +/// +/// For attribute accesses (e.g. `np.ComplexWarning`), we only ignore the violation +/// if it's inside an `except AttributeError` block, and the member is accessed +/// through its non-deprecated name in the associated `try` block. +/// +/// For uses of the `numpy` member where it's simply an `ExprName` node, +/// we check to see how the `numpy` member was bound. If it was bound via a +/// `from numpy import foo` statement, we check to see if that import statement +/// took place inside an `except ImportError` or `except ModuleNotFoundError` block. +/// If so, and if the `numpy` member was imported through its non-deprecated name +/// in the associated try block, we ignore the violation in the same way. +/// +/// Examples: +/// +/// ```py +/// import numpy as np +/// +/// try: +/// np.all([True, True]) +/// except AttributeError: +/// np.alltrue([True, True]) # Okay +/// +/// try: +/// from numpy.exceptions import ComplexWarning +/// except ImportError: +/// from numpy import ComplexWarning +/// +/// x = ComplexWarning() # Okay +/// ``` fn is_guarded_by_try_except( expr: &Expr, replacement: &Replacement, @@ -743,7 +775,7 @@ fn is_guarded_by_try_except( } let Some(try_node) = binding .source - .and_then(|import_id| try_node_parent_of_import_stmt(import_id, semantic)) + .and_then(|import_id| enclosing_try_node(import_id, semantic)) else { return false; }; @@ -759,11 +791,13 @@ fn is_guarded_by_try_except( } } -fn try_node_parent_of_import_stmt<'a>( - import_id: NodeId, +/// Given the `NodeId` of a node that is known to be inside an exception handler, +/// find the [`ast::StmtTry`] node in which the node resides. +fn enclosing_try_node<'a>( + node_id: NodeId, semantic: &'a SemanticModel, ) -> Option<&'a ast::StmtTry> { - let mut node_id = import_id; + let mut node_id = node_id; loop { node_id = semantic.parent_statement_id(node_id)?; if let ast::Stmt::Try(try_node) = semantic.statement(node_id) { @@ -772,15 +806,9 @@ fn try_node_parent_of_import_stmt<'a>( } } -fn undeprecated_qualified_name<'a>(path: &'a str, name: &'a str) -> QualifiedName<'a> { - let mut builder = QualifiedNameBuilder::default(); - for part in path.split('.') { - builder.push(part); - } - builder.push(name); - builder.build() -} - +/// Given an [`ast::StmtTry`] node, does the `try` branch of that node +/// contain any [`ast::ExprAttribute`] nodes that indicate the numpy +/// member is being accessed from the non-deprecated location? fn try_block_contains_undeprecated_attribute( try_node: &ast::StmtTry, replacement_details: &Details, @@ -794,7 +822,14 @@ fn try_block_contains_undeprecated_attribute( else { return false; }; - let undeprecated_qualified_name = undeprecated_qualified_name(path, name); + let undeprecated_qualified_name = { + let mut builder = QualifiedNameBuilder::default(); + for part in path.split('.') { + builder.push(part); + } + builder.push(name); + builder.build() + }; let mut attribute_searcher = AttributeSearcher::new(undeprecated_qualified_name, semantic); for stmt in &try_node.body { attribute_searcher.visit_stmt(stmt); @@ -805,6 +840,8 @@ fn try_block_contains_undeprecated_attribute( false } +/// AST visitor that searches an AST tree for [`ast::ExprAttribute`] nodes +/// that match a certain [`QualifiedName`]. struct AttributeSearcher<'a> { attribute_to_find: QualifiedName<'a>, semantic: &'a SemanticModel<'a>, @@ -839,6 +876,9 @@ impl Visitor<'_> for AttributeSearcher<'_> { } } +/// Given an [`ast::StmtTry`] node, does the `try` branch of that node +/// contain any [`ast::StmtImportFrom`] nodes that indicate the numpy +/// member is being imported from the non-deprecated location? fn try_block_contains_undeprecated_import( try_node: &ast::StmtTry, replacement_details: &Details, @@ -861,6 +901,8 @@ fn try_block_contains_undeprecated_import( false } +/// AST visitor that searches an AST tree for [`ast::StmtImportFrom`] nodes +/// that match a certain [`QualifiedName`]. struct ImportSearcher<'a> { module: &'a str, name: &'a str, From bb14c21f41927aa570c6543d63c4b2441329eaef Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Wed, 24 Jul 2024 15:01:28 +0100 Subject: [PATCH 5/6] Optimize --- .../src/rules/numpy/rules/numpy_2_0_deprecation.rs | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/crates/ruff_linter/src/rules/numpy/rules/numpy_2_0_deprecation.rs b/crates/ruff_linter/src/rules/numpy/rules/numpy_2_0_deprecation.rs index 587b58e9f87dc..5d156134faa75 100644 --- a/crates/ruff_linter/src/rules/numpy/rules/numpy_2_0_deprecation.rs +++ b/crates/ruff_linter/src/rules/numpy/rules/numpy_2_0_deprecation.rs @@ -1,6 +1,7 @@ use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation}; use ruff_macros::{derive_message_formats, violation}; use ruff_python_ast::name::{QualifiedName, QualifiedNameBuilder}; +use ruff_python_ast::statement_visitor::StatementVisitor; use ruff_python_ast::visitor::Visitor; use ruff_python_ast::{self as ast, Expr}; use ruff_python_semantic::{Exceptions, Modules, NodeId, SemanticModel}; @@ -874,6 +875,13 @@ impl Visitor<'_> for AttributeSearcher<'_> { } ast::visitor::walk_expr(self, expr); } + + fn visit_stmt(&mut self, stmt: &ruff_python_ast::Stmt) { + if self.found_attribute { + return; + } + ast::visitor::walk_stmt(self, stmt); + } } /// Given an [`ast::StmtTry`] node, does the `try` branch of that node @@ -919,7 +927,7 @@ impl<'a> ImportSearcher<'a> { } } -impl Visitor<'_> for ImportSearcher<'_> { +impl StatementVisitor<'_> for ImportSearcher<'_> { fn visit_stmt(&mut self, stmt: &ast::Stmt) { if self.found_import { return; @@ -934,6 +942,6 @@ impl Visitor<'_> for ImportSearcher<'_> { return; } } - ast::visitor::walk_stmt(self, stmt); + ast::statement_visitor::walk_stmt(self, stmt); } } From 9c8c7d345fd1c706ab265eca61716d37219a278b Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Wed, 24 Jul 2024 20:59:19 +0100 Subject: [PATCH 6/6] Charlie review --- .../numpy/rules/numpy_2_0_deprecation.rs | 67 +++++++++---------- crates/ruff_python_semantic/src/model.rs | 8 +++ 2 files changed, 38 insertions(+), 37 deletions(-) diff --git a/crates/ruff_linter/src/rules/numpy/rules/numpy_2_0_deprecation.rs b/crates/ruff_linter/src/rules/numpy/rules/numpy_2_0_deprecation.rs index 5d156134faa75..ea219d08042f8 100644 --- a/crates/ruff_linter/src/rules/numpy/rules/numpy_2_0_deprecation.rs +++ b/crates/ruff_linter/src/rules/numpy/rules/numpy_2_0_deprecation.rs @@ -4,7 +4,7 @@ use ruff_python_ast::name::{QualifiedName, QualifiedNameBuilder}; use ruff_python_ast::statement_visitor::StatementVisitor; use ruff_python_ast::visitor::Visitor; use ruff_python_ast::{self as ast, Expr}; -use ruff_python_semantic::{Exceptions, Modules, NodeId, SemanticModel}; +use ruff_python_semantic::{Exceptions, Modules, SemanticModel}; use ruff_text_size::Ranged; use crate::checkers::ast::Checker; @@ -774,10 +774,11 @@ fn is_guarded_by_try_except( if !binding.in_exception_handler() { return false; } - let Some(try_node) = binding - .source - .and_then(|import_id| enclosing_try_node(import_id, semantic)) - else { + let Some(try_node) = binding.source.and_then(|import_id| { + semantic + .statements(import_id) + .find_map(|stmt| stmt.as_try_stmt()) + }) else { return false; }; let suspended_exceptions = Exceptions::from_try_stmt(try_node, semantic); @@ -792,21 +793,6 @@ fn is_guarded_by_try_except( } } -/// Given the `NodeId` of a node that is known to be inside an exception handler, -/// find the [`ast::StmtTry`] node in which the node resides. -fn enclosing_try_node<'a>( - node_id: NodeId, - semantic: &'a SemanticModel, -) -> Option<&'a ast::StmtTry> { - let mut node_id = node_id; - loop { - node_id = semantic.parent_statement_id(node_id)?; - if let ast::Stmt::Try(try_node) = semantic.statement(node_id) { - return Some(try_node); - } - } -} - /// Given an [`ast::StmtTry`] node, does the `try` branch of that node /// contain any [`ast::ExprAttribute`] nodes that indicate the numpy /// member is being accessed from the non-deprecated location? @@ -832,13 +818,8 @@ fn try_block_contains_undeprecated_attribute( builder.build() }; let mut attribute_searcher = AttributeSearcher::new(undeprecated_qualified_name, semantic); - for stmt in &try_node.body { - attribute_searcher.visit_stmt(stmt); - if attribute_searcher.found_attribute { - return true; - } - } - false + attribute_searcher.visit_body(&try_node.body); + attribute_searcher.found_attribute } /// AST visitor that searches an AST tree for [`ast::ExprAttribute`] nodes @@ -877,10 +858,18 @@ impl Visitor<'_> for AttributeSearcher<'_> { } fn visit_stmt(&mut self, stmt: &ruff_python_ast::Stmt) { - if self.found_attribute { - return; + if !self.found_attribute { + ast::visitor::walk_stmt(self, stmt); + } + } + + fn visit_body(&mut self, body: &[ruff_python_ast::Stmt]) { + for stmt in body { + self.visit_stmt(stmt); + if self.found_attribute { + return; + } } - ast::visitor::walk_stmt(self, stmt); } } @@ -900,13 +889,8 @@ fn try_block_contains_undeprecated_import( return false; }; let mut import_searcher = ImportSearcher::new(path, name); - for stmt in &try_node.body { - import_searcher.visit_stmt(stmt); - if import_searcher.found_import { - return true; - } - } - false + import_searcher.visit_body(&try_node.body); + import_searcher.found_import } /// AST visitor that searches an AST tree for [`ast::StmtImportFrom`] nodes @@ -944,4 +928,13 @@ impl StatementVisitor<'_> for ImportSearcher<'_> { } ast::statement_visitor::walk_stmt(self, stmt); } + + fn visit_body(&mut self, body: &[ruff_python_ast::Stmt]) { + for stmt in body { + self.visit_stmt(stmt); + if self.found_import { + return; + } + } + } } diff --git a/crates/ruff_python_semantic/src/model.rs b/crates/ruff_python_semantic/src/model.rs index ff1761b7b9479..4a632279ae12f 100644 --- a/crates/ruff_python_semantic/src/model.rs +++ b/crates/ruff_python_semantic/src/model.rs @@ -1207,6 +1207,14 @@ impl<'a> SemanticModel<'a> { .expect("No statement found") } + /// Returns an [`Iterator`] over the statements, starting from the given [`NodeId`]. + /// through to any parents. + pub fn statements(&self, node_id: NodeId) -> impl Iterator + '_ { + self.nodes + .ancestor_ids(node_id) + .filter_map(move |id| self.nodes[id].as_statement()) + } + /// Given a [`Stmt`], return its parent, if any. #[inline] pub fn parent_statement(&self, node_id: NodeId) -> Option<&'a Stmt> {