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.

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

Ignore NPY201 inside except blocks for compatibility with older numpy versions #12490

Merged
merged 6 commits into from
Jul 24, 2024
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
17 changes: 17 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/numpy/NPY201.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
18 changes: 18 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/numpy/NPY201_2.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
44 changes: 18 additions & 26 deletions crates/ruff_linter/src/checkers/ast/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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
Expand All @@ -1853,6 +1841,10 @@ impl<'a> Checker<'a> {
self.semantic.scope_id
};

if self.semantic.in_exception_handler() {
flags |= BindingFlags::IN_EXCEPT_HANDLER;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Idly wondering if we should just store the semantic model flags on the binding, in theory it's tiny.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As in, get rid of the separate BindingFlags struct altogether, and store the semantic-model flags on Binding instead? Or store the semantic-model flags in addition to the BindingFlags?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Store them in addition (and then remove this flag from BindingFlags). But, it's ok, no need to change.


// Create the `Binding`.
let binding_id = self.semantic.push_binding(range, kind, flags);

Expand Down
248 changes: 246 additions & 2 deletions crates/ruff_linter/src/rules/numpy/rules/numpy_2_0_deprecation.rs
Original file line number Diff line number Diff line change
@@ -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, NodeId, SemanticModel};
use ruff_text_size::Ranged;

use crate::checkers::ast::Checker;
Expand Down Expand Up @@ -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(),
Expand Down Expand Up @@ -701,3 +708,240 @@ 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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we check semantic.handled_exceptions instead? Does that work? Then we wouldn't need to find the try statement or anything.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, semantic.handled_exceptions records the exceptions if we're visiting the try block of a try/except statement. But here we're visiting the except block of a try/except statement. Hence "suspended_exceptions" rather than "handled_exceptions".

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also we still need to find the node for the try statement so that we can check whether it was accessed via the undeprecated call path in the try block

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| enclosing_try_node(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,
}
}

/// 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);
}
AlexWaygood marked this conversation as resolved.
Show resolved Hide resolved
}
}

/// 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);
for stmt in &try_node.body {
AlexWaygood marked this conversation as resolved.
Show resolved Hide resolved
attribute_searcher.visit_stmt(stmt);
if attribute_searcher.found_attribute {
return true;
}
}
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>,
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;
}
AlexWaygood marked this conversation as resolved.
Show resolved Hide resolved
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 {
return;
}
ast::visitor::walk_stmt(self, stmt);
}
}

/// 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);
for stmt in &try_node.body {
import_searcher.visit_stmt(stmt);
if import_searcher.found_import {
return true;
}
}
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,
found_import: bool,
}
Comment on lines +896 to +902
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it matters much because we only run this code in the error case, but it is kind of expensive. Do we have no way of resolving the statement in which a binding is declared and then traverse upwards (by using the parent API?)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The situation is that we're visiting an except block in a try/except statement. We find a deprecated numpy access in that except block. We need to find out if there's an undeprecated access in the try block of the try/except statement. The way I do this is by traversing upwards from the node we're linting in the except block (using the parent API) to find the StmtTry node, then from the StmtTry node I then traverse downwards into the try block using the ImportSearcher to find if there are any imports using the undeprecated API.


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);
}
}
AlexWaygood marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -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`

Expand All @@ -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
Loading
Loading