From 928ffd66503759679823c5f346941ef58486766e Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Wed, 24 Jul 2024 21:03:23 +0100 Subject: [PATCH] Ignore `NPY201` inside `except` blocks for compatibility with older numpy versions (#12490) --- .../resources/test/fixtures/numpy/NPY201.py | 17 ++ .../resources/test/fixtures/numpy/NPY201_2.py | 18 ++ crates/ruff_linter/src/checkers/ast/mod.rs | 44 ++-- .../numpy/rules/numpy_2_0_deprecation.rs | 241 +++++++++++++++++- ...__tests__numpy2-deprecation_NPY201.py.snap | 5 + ...tests__numpy2-deprecation_NPY201_2.py.snap | 47 ++++ crates/ruff_python_semantic/src/binding.rs | 47 +++- crates/ruff_python_semantic/src/model.rs | 24 +- 8 files changed, 413 insertions(+), 30 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/resources/test/fixtures/numpy/NPY201_2.py b/crates/ruff_linter/resources/test/fixtures/numpy/NPY201_2.py index 0f9262b7eb558..e43a6611c446a 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,21 @@ def func(): np.ComplexWarning 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 # `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..fa2a4f2cfcae2 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(); @@ -1837,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 @@ -1853,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 eeded38dde932..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 @@ -1,7 +1,10 @@ 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::statement_visitor::StatementVisitor; +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 +668,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 +708,233 @@ 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, + semantic: &SemanticModel, +) -> bool { + match expr { + Expr::Attribute(_) => { + 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 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| { + semantic + .statements(import_id) + .find_map(|stmt| stmt.as_try_stmt()) + }) 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, + } +} + +/// 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, + semantic: &SemanticModel, +) -> bool { + let Details::AutoImport { + path, + name, + compatibility: _, + } = replacement_details + else { + return false; + }; + 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); + attribute_searcher.visit_body(&try_node.body); + attribute_searcher.found_attribute +} + +/// 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>, + 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); + } + + fn visit_stmt(&mut self, stmt: &ruff_python_ast::Stmt) { + 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; + } + } + } +} + +/// 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, +) -> bool { + let Details::AutoImport { + path, + name, + compatibility: _, + } = replacement_details + else { + return false; + }; + let mut import_searcher = ImportSearcher::new(path, name); + import_searcher.visit_body(&try_node.body); + import_searcher.found_import +} + +/// 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, + found_import: bool, +} + +impl<'a> ImportSearcher<'a> { + fn new(module: &'a str, name: &'a str) -> Self { + Self { + module, + name, + found_import: false, + } + } +} + +impl StatementVisitor<'_> 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::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_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_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..d4cc059088ca8 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}; @@ -114,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 { @@ -333,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; } } @@ -579,6 +604,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 } } diff --git a/crates/ruff_python_semantic/src/model.rs b/crates/ruff_python_semantic/src/model.rs index 3fe72f4658322..4a632279ae12f 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 @@ -1193,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> {