Skip to content

Commit

Permalink
Remove Identifier usages for isolating exception names (#5797)
Browse files Browse the repository at this point in the history
## Summary

The motivating change here is to remove `let range =
except_handler.try_identifier().unwrap();` and instead just do
`name.range()`, since exception names now have ranges attached to them
by the parse. This also required some refactors (which are improvements)
to the built-in attribute shadowing rules, since at least one invocation
relied on passing in the exception handler and calling
`.try_identifier()`. Now that we have easy access to identifiers, we can
remove the whole `AnyShadowing` abstraction.
  • Loading branch information
charliermarsh authored Jul 16, 2023
1 parent 59dfd0e commit 01b05fe
Show file tree
Hide file tree
Showing 9 changed files with 103 additions and 170 deletions.
144 changes: 70 additions & 74 deletions crates/ruff/src/checkers/ast/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ use crate::fs::relativize_path;
use crate::importer::Importer;
use crate::noqa::NoqaMapping;
use crate::registry::Rule;
use crate::rules::flake8_builtins::helpers::AnyShadowing;

use crate::rules::{
airflow, flake8_2020, flake8_annotations, flake8_async, flake8_bandit, flake8_blind_except,
Expand Down Expand Up @@ -614,15 +613,15 @@ where
self,
class_def,
name,
AnyShadowing::from(stmt),
name.range(),
);
}
} else {
if self.enabled(Rule::BuiltinVariableShadowing) {
flake8_builtins::rules::builtin_variable_shadowing(
self,
name,
AnyShadowing::from(stmt),
name.range(),
);
}
}
Expand Down Expand Up @@ -753,11 +752,7 @@ where
flake8_bugbear::rules::f_string_docstring(self, body);
}
if self.enabled(Rule::BuiltinVariableShadowing) {
flake8_builtins::rules::builtin_variable_shadowing(
self,
name,
AnyShadowing::from(stmt),
);
flake8_builtins::rules::builtin_variable_shadowing(self, name, name.range());
}
if self.enabled(Rule::DuplicateBases) {
pylint::rules::duplicate_bases(self, name, bases);
Expand Down Expand Up @@ -837,7 +832,7 @@ where
flake8_builtins::rules::builtin_variable_shadowing(
self,
asname,
AnyShadowing::from(stmt),
asname.range(),
);
}
}
Expand Down Expand Up @@ -1091,7 +1086,7 @@ where
flake8_builtins::rules::builtin_variable_shadowing(
self,
asname,
AnyShadowing::from(stmt),
asname.range(),
);
}
}
Expand Down Expand Up @@ -2265,7 +2260,7 @@ where
}
}
}
Expr::Name(ast::ExprName { id, ctx, range: _ }) => {
Expr::Name(ast::ExprName { id, ctx, range }) => {
match ctx {
ExprContext::Load => {
self.handle_node_load(expr);
Expand Down Expand Up @@ -2338,18 +2333,13 @@ where
if let ScopeKind::Class(class_def) = self.semantic.scope().kind {
if self.enabled(Rule::BuiltinAttributeShadowing) {
flake8_builtins::rules::builtin_attribute_shadowing(
self,
class_def,
id,
AnyShadowing::from(expr),
self, class_def, id, *range,
);
}
} else {
if self.enabled(Rule::BuiltinVariableShadowing) {
flake8_builtins::rules::builtin_variable_shadowing(
self,
id,
AnyShadowing::from(expr),
self, id, *range,
);
}
}
Expand Down Expand Up @@ -3880,7 +3870,6 @@ where
body,
range: _,
}) => {
let name = name.as_deref();
if self.enabled(Rule::BareExcept) {
if let Some(diagnostic) = pycodestyle::rules::bare_except(
type_.as_deref(),
Expand All @@ -3892,17 +3881,25 @@ where
}
}
if self.enabled(Rule::RaiseWithoutFromInsideExcept) {
flake8_bugbear::rules::raise_without_from_inside_except(self, name, body);
flake8_bugbear::rules::raise_without_from_inside_except(
self,
name.as_deref(),
body,
);
}
if self.enabled(Rule::BlindExcept) {
flake8_blind_except::rules::blind_except(self, type_.as_deref(), name, body);
flake8_blind_except::rules::blind_except(
self,
type_.as_deref(),
name.as_deref(),
body,
);
}
if self.enabled(Rule::TryExceptPass) {
flake8_bandit::rules::try_except_pass(
self,
except_handler,
type_.as_deref(),
name,
body,
self.settings.flake8_bandit.check_typed_exception,
);
Expand All @@ -3912,7 +3909,6 @@ where
self,
except_handler,
type_.as_deref(),
name,
body,
self.settings.flake8_bandit.check_typed_exception,
);
Expand All @@ -3929,66 +3925,66 @@ where
if self.enabled(Rule::BinaryOpException) {
pylint::rules::binary_op_exception(self, except_handler);
}
match name {
Some(name) => {
let range = except_handler.try_identifier().unwrap();

if self.enabled(Rule::AmbiguousVariableName) {
if let Some(diagnostic) =
pycodestyle::rules::ambiguous_variable_name(name, range)
{
self.diagnostics.push(diagnostic);
}
}
if self.enabled(Rule::BuiltinVariableShadowing) {
flake8_builtins::rules::builtin_variable_shadowing(
self,
name,
AnyShadowing::from(except_handler),
);
if let Some(name) = name {
if self.enabled(Rule::AmbiguousVariableName) {
if let Some(diagnostic) =
pycodestyle::rules::ambiguous_variable_name(name.as_str(), name.range())
{
self.diagnostics.push(diagnostic);
}

// Store the existing binding, if any.
let existing_id = self.semantic.lookup_symbol(name);

// Add the bound exception name to the scope.
let binding_id = self.add_binding(
}
if self.enabled(Rule::BuiltinVariableShadowing) {
flake8_builtins::rules::builtin_variable_shadowing(
self,
name,
range,
BindingKind::Assignment,
BindingFlags::empty(),
name.range(),
);
}

walk_except_handler(self, except_handler);
// Store the existing binding, if any.
let existing_id = self.semantic.lookup_symbol(name.as_str());

// If the exception name wasn't used in the scope, emit a diagnostic.
if !self.semantic.is_used(binding_id) {
if self.enabled(Rule::UnusedVariable) {
let mut diagnostic = Diagnostic::new(
pyflakes::rules::UnusedVariable { name: name.into() },
range,
);
if self.patch(Rule::UnusedVariable) {
diagnostic.try_set_fix(|| {
pyflakes::fixes::remove_exception_handler_assignment(
except_handler,
self.locator,
)
.map(Fix::automatic)
});
}
self.diagnostics.push(diagnostic);
// Add the bound exception name to the scope.
let binding_id = self.add_binding(
name.as_str(),
name.range(),
BindingKind::Assignment,
BindingFlags::empty(),
);

walk_except_handler(self, except_handler);

// If the exception name wasn't used in the scope, emit a diagnostic.
if !self.semantic.is_used(binding_id) {
if self.enabled(Rule::UnusedVariable) {
let mut diagnostic = Diagnostic::new(
pyflakes::rules::UnusedVariable {
name: name.to_string(),
},
name.range(),
);
if self.patch(Rule::UnusedVariable) {
diagnostic.try_set_fix(|| {
pyflakes::fixes::remove_exception_handler_assignment(
except_handler,
self.locator,
)
.map(Fix::automatic)
});
}
self.diagnostics.push(diagnostic);
}

self.add_binding(
name,
range,
BindingKind::UnboundException(existing_id),
BindingFlags::empty(),
);
}
None => walk_except_handler(self, except_handler),

self.add_binding(
name.as_str(),
name.range(),
BindingKind::UnboundException(existing_id),
BindingFlags::empty(),
);
} else {
walk_except_handler(self, except_handler);
}
}
}
Expand Down
14 changes: 6 additions & 8 deletions crates/ruff/src/rules/flake8_bandit/rules/try_except_continue.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,16 +55,14 @@ pub(crate) fn try_except_continue(
checker: &mut Checker,
except_handler: &ExceptHandler,
type_: Option<&Expr>,
_name: Option<&str>,
body: &[Stmt],
check_typed_exception: bool,
) {
if body.len() == 1
&& body[0].is_continue_stmt()
&& (check_typed_exception || is_untyped_exception(type_, checker.semantic()))
{
checker
.diagnostics
.push(Diagnostic::new(TryExceptContinue, except_handler.range()));
if matches!(body, [Stmt::Continue(_)]) {
if check_typed_exception || is_untyped_exception(type_, checker.semantic()) {
checker
.diagnostics
.push(Diagnostic::new(TryExceptContinue, except_handler.range()));
}
}
}
14 changes: 6 additions & 8 deletions crates/ruff/src/rules/flake8_bandit/rules/try_except_pass.rs
Original file line number Diff line number Diff line change
Expand Up @@ -51,16 +51,14 @@ pub(crate) fn try_except_pass(
checker: &mut Checker,
except_handler: &ExceptHandler,
type_: Option<&Expr>,
_name: Option<&str>,
body: &[Stmt],
check_typed_exception: bool,
) {
if body.len() == 1
&& body[0].is_pass_stmt()
&& (check_typed_exception || is_untyped_exception(type_, checker.semantic()))
{
checker
.diagnostics
.push(Diagnostic::new(TryExceptPass, except_handler.range()));
if matches!(body, [Stmt::Pass(_)]) {
if check_typed_exception || is_untyped_exception(type_, checker.semantic()) {
checker
.diagnostics
.push(Diagnostic::new(TryExceptPass, except_handler.range()));
}
}
}
39 changes: 0 additions & 39 deletions crates/ruff/src/rules/flake8_builtins/helpers.rs
Original file line number Diff line number Diff line change
@@ -1,44 +1,5 @@
use ruff_text_size::TextRange;
use rustpython_parser::ast::{ExceptHandler, Expr, Ranged, Stmt};

use ruff_python_ast::identifier::{Identifier, TryIdentifier};
use ruff_python_stdlib::builtins::BUILTINS;

pub(super) fn shadows_builtin(name: &str, ignorelist: &[String]) -> bool {
BUILTINS.contains(&name) && ignorelist.iter().all(|ignore| ignore != name)
}

#[derive(Debug, Copy, Clone, PartialEq)]
pub(crate) enum AnyShadowing<'a> {
Expression(&'a Expr),
Statement(&'a Stmt),
ExceptHandler(&'a ExceptHandler),
}

impl Identifier for AnyShadowing<'_> {
fn identifier(&self) -> TextRange {
match self {
AnyShadowing::Expression(expr) => expr.range(),
AnyShadowing::Statement(stmt) => stmt.identifier(),
AnyShadowing::ExceptHandler(handler) => handler.try_identifier().unwrap(),
}
}
}

impl<'a> From<&'a Stmt> for AnyShadowing<'a> {
fn from(value: &'a Stmt) -> Self {
AnyShadowing::Statement(value)
}
}

impl<'a> From<&'a Expr> for AnyShadowing<'a> {
fn from(value: &'a Expr) -> Self {
AnyShadowing::Expression(value)
}
}

impl<'a> From<&'a ExceptHandler> for AnyShadowing<'a> {
fn from(value: &'a ExceptHandler) -> Self {
AnyShadowing::ExceptHandler(value)
}
}
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
use ruff_text_size::TextRange;
use rustpython_parser::ast;

use ruff_diagnostics::Diagnostic;
use ruff_diagnostics::Violation;
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::identifier::Identifier;
use rustpython_parser::ast;

use crate::checkers::ast::Checker;

use super::super::helpers::{shadows_builtin, AnyShadowing};
use crate::rules::flake8_builtins::helpers::shadows_builtin;

/// ## What it does
/// Checks for any class attributes that use the same name as a builtin.
Expand Down Expand Up @@ -67,7 +67,7 @@ pub(crate) fn builtin_attribute_shadowing(
checker: &mut Checker,
class_def: &ast::StmtClassDef,
name: &str,
shadowing: AnyShadowing,
range: TextRange,
) {
if shadows_builtin(name, &checker.settings.flake8_builtins.builtins_ignorelist) {
// Ignore shadowing within `TypedDict` definitions, since these are only accessible through
Expand All @@ -84,7 +84,7 @@ pub(crate) fn builtin_attribute_shadowing(
BuiltinAttributeShadowing {
name: name.to_string(),
},
shadowing.identifier(),
range,
));
}
}
Loading

0 comments on commit 01b05fe

Please sign in to comment.