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

Redirect PHG001 to S307 and PGH002 to G010 #9756

Merged
merged 4 commits into from
Feb 1, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view

This file was deleted.

This file was deleted.

This file was deleted.

This file was deleted.

9 changes: 1 addition & 8 deletions crates/ruff_linter/src/checkers/ast/analyze/expression.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,7 @@ use crate::rules::{
flake8_future_annotations, flake8_gettext, flake8_implicit_str_concat, flake8_logging,
flake8_logging_format, flake8_pie, flake8_print, flake8_pyi, flake8_pytest_style, flake8_self,
flake8_simplify, flake8_tidy_imports, flake8_trio, flake8_type_checking, flake8_use_pathlib,
flynt, numpy, pandas_vet, pep8_naming, pycodestyle, pyflakes, pygrep_hooks, pylint, pyupgrade,
refurb, ruff,
flynt, numpy, pandas_vet, pep8_naming, pycodestyle, pyflakes, pylint, pyupgrade, refurb, ruff,
};
use crate::settings::types::PythonVersion;

Expand Down Expand Up @@ -773,12 +772,6 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
if checker.enabled(Rule::CallDateFromtimestamp) {
flake8_datetimez::rules::call_date_fromtimestamp(checker, func, expr.range());
}
if checker.enabled(Rule::Eval) {
pygrep_hooks::rules::no_eval(checker, func);
}
if checker.enabled(Rule::DeprecatedLogWarn) {
pygrep_hooks::rules::deprecated_log_warn(checker, call);
}
if checker.enabled(Rule::UnnecessaryDirectLambdaCall) {
pylint::rules::unnecessary_direct_lambda_call(checker, expr, func);
}
Expand Down
4 changes: 2 additions & 2 deletions crates/ruff_linter/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -705,8 +705,8 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
(Flake8Datetimez, "012") => (RuleGroup::Stable, rules::flake8_datetimez::rules::CallDateFromtimestamp),

// pygrep-hooks
(PygrepHooks, "001") => (RuleGroup::Stable, rules::pygrep_hooks::rules::Eval),
(PygrepHooks, "002") => (RuleGroup::Stable, rules::pygrep_hooks::rules::DeprecatedLogWarn),
(PygrepHooks, "001") => (RuleGroup::Removed, rules::pygrep_hooks::rules::Eval),
(PygrepHooks, "002") => (RuleGroup::Removed, rules::pygrep_hooks::rules::DeprecatedLogWarn),
(PygrepHooks, "003") => (RuleGroup::Stable, rules::pygrep_hooks::rules::BlanketTypeIgnore),
(PygrepHooks, "004") => (RuleGroup::Stable, rules::pygrep_hooks::rules::BlanketNOQA),
(PygrepHooks, "005") => (RuleGroup::Stable, rules::pygrep_hooks::rules::InvalidMockAccess),
Expand Down
2 changes: 2 additions & 0 deletions crates/ruff_linter/src/rule_redirects.rs
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,8 @@ static REDIRECTS: Lazy<HashMap<&'static str, &'static str>> = Lazy::new(|| {
("RUF011", "B035"),
("TCH006", "TCH010"),
("TRY200", "B904"),
("PGH001", "S307"),
("PHG002", "G010"),
// Test redirect by exact code
#[cfg(feature = "test-rules")]
("RUF940", "RUF950"),
Expand Down
41 changes: 37 additions & 4 deletions crates/ruff_linter/src/rule_selector.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ impl FromStr for RuleSelector {
type Err = ParseError;

fn from_str(s: &str) -> Result<Self, Self::Err> {
// **Changes should be reflected in `parse_no_redirect` as well**
match s {
"ALL" => Ok(Self::All),
#[allow(deprecated)]
Expand All @@ -67,7 +68,6 @@ impl FromStr for RuleSelector {
return Ok(Self::Linter(linter));
}

// Does the selector select a single rule?
let prefix = RuleCodePrefix::parse(&linter, code)
.map_err(|_| ParseError::Unknown(s.to_string()))?;

Expand Down Expand Up @@ -254,8 +254,6 @@ pub struct PreviewOptions {

#[cfg(feature = "schemars")]
mod schema {
use std::str::FromStr;

use itertools::Itertools;
use schemars::JsonSchema;
use schemars::_serde_json::Value;
Expand Down Expand Up @@ -302,7 +300,7 @@ mod schema {
.filter(|p| {
// Exclude any prefixes where all of the rules are removed
if let Ok(Self::Rule { prefix, .. } | Self::Prefix { prefix, .. }) =
RuleSelector::from_str(p)
RuleSelector::parse_no_redirect(p)
{
!prefix.rules().all(|rule| rule.is_removed())
} else {
Expand Down Expand Up @@ -341,6 +339,41 @@ impl RuleSelector {
}
}
}

/// Parse [`RuleSelector`] from a string; but do not follow redirects.
pub fn parse_no_redirect(s: &str) -> Result<Self, ParseError> {
// **Changes should be reflected in `from_str` as well**
match s {
"ALL" => Ok(Self::All),
#[allow(deprecated)]
"NURSERY" => Ok(Self::Nursery),
"C" => Ok(Self::C),
"T" => Ok(Self::T),
_ => {
let (linter, code) =
Linter::parse_code(s).ok_or_else(|| ParseError::Unknown(s.to_string()))?;

if code.is_empty() {
return Ok(Self::Linter(linter));
}

let prefix = RuleCodePrefix::parse(&linter, code)
.map_err(|_| ParseError::Unknown(s.to_string()))?;

if is_single_rule_selector(&prefix) {
Ok(Self::Rule {
prefix,
redirected_from: None,
})
} else {
Ok(Self::Prefix {
prefix,
redirected_from: None,
})
}
}
}
}
}

#[derive(EnumIter, PartialEq, Eq, PartialOrd, Ord, Copy, Clone, Debug)]
Expand Down
4 changes: 0 additions & 4 deletions crates/ruff_linter/src/rules/pygrep_hooks/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,6 @@ mod tests {
use crate::test::test_path;
use crate::{assert_messages, settings};

#[test_case(Rule::Eval, Path::new("PGH001_0.py"))]
#[test_case(Rule::Eval, Path::new("PGH001_1.py"))]
#[test_case(Rule::DeprecatedLogWarn, Path::new("PGH002_0.py"))]
#[test_case(Rule::DeprecatedLogWarn, Path::new("PGH002_1.py"))]
#[test_case(Rule::BlanketTypeIgnore, Path::new("PGH003_0.py"))]
#[test_case(Rule::BlanketTypeIgnore, Path::new("PGH003_1.py"))]
#[test_case(Rule::BlanketNOQA, Path::new("PGH004_0.py"))]
Expand Down
Original file line number Diff line number Diff line change
@@ -1,13 +1,9 @@
use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation};
use ruff_diagnostics::{FixAvailability, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::{self as ast, Expr};
use ruff_python_semantic::analyze::logging;
use ruff_python_stdlib::logging::LoggingLevel;
use ruff_text_size::Ranged;

use crate::checkers::ast::Checker;
use crate::importer::ImportRequest;

/// ## Removed
/// This rule is identical to [G010] which should be used instead.
///
/// ## What it does
/// Check for usages of the deprecated `warn` method from the `logging` module.
///
Expand All @@ -34,9 +30,12 @@ use crate::importer::ImportRequest;
///
/// ## References
/// - [Python documentation: `logger.Logger.warning`](https://docs.python.org/3/library/logging.html#logging.Logger.warning)
///
/// [G010]: https://docs.astral.sh/ruff/rules/logging-warn/
#[violation]
pub struct DeprecatedLogWarn;

/// PGH002
impl Violation for DeprecatedLogWarn {
const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes;

Expand All @@ -49,59 +48,3 @@ impl Violation for DeprecatedLogWarn {
Some(format!("Replace with `warning`"))
}
}

/// PGH002
pub(crate) fn deprecated_log_warn(checker: &mut Checker, call: &ast::ExprCall) {
match call.func.as_ref() {
Expr::Attribute(ast::ExprAttribute { attr, .. }) => {
if !logging::is_logger_candidate(
&call.func,
checker.semantic(),
&checker.settings.logger_objects,
) {
return;
}
if !matches!(
LoggingLevel::from_attribute(attr.as_str()),
Some(LoggingLevel::Warn)
) {
return;
}
}
Expr::Name(_) => {
if !checker
.semantic()
.resolve_call_path(call.func.as_ref())
.is_some_and(|call_path| matches!(call_path.as_slice(), ["logging", "warn"]))
{
return;
}
}
_ => return,
}

let mut diagnostic = Diagnostic::new(DeprecatedLogWarn, call.func.range());

match call.func.as_ref() {
Expr::Attribute(ast::ExprAttribute { attr, .. }) => {
diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement(
"warning".to_string(),
attr.range(),
)));
}
Expr::Name(_) => {
diagnostic.try_set_fix(|| {
let (import_edit, binding) = checker.importer().get_or_import_symbol(
&ImportRequest::import("logging", "warning"),
call.start(),
checker.semantic(),
)?;
let name_edit = Edit::range_replacement(binding, call.func.range());
Ok(Fix::safe_edits(import_edit, [name_edit]))
});
}
_ => {}
}

checker.diagnostics.push(diagnostic);
}
29 changes: 7 additions & 22 deletions crates/ruff_linter/src/rules/pygrep_hooks/rules/no_eval.rs
Original file line number Diff line number Diff line change
@@ -1,11 +1,9 @@
use ruff_python_ast::{self as ast, Expr};

use ruff_diagnostics::{Diagnostic, Violation};
use ruff_diagnostics::Violation;
use ruff_macros::{derive_message_formats, violation};
use ruff_text_size::Ranged;

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

/// ## Removed
/// This rule is identical to [S307] which should be used instead.
///
/// ## What it does
/// Checks for uses of the builtin `eval()` function.
///
Expand All @@ -29,28 +27,15 @@ use crate::checkers::ast::Checker;
/// ## References
/// - [Python documentation: `eval`](https://docs.python.org/3/library/functions.html#eval)
/// - [_Eval really is dangerous_ by Ned Batchelder](https://nedbatchelder.com/blog/201206/eval_really_is_dangerous.html)
///
/// [S307]: https://docs.astral.sh/ruff/rules/suspicious-eval-usage/
#[violation]
pub struct Eval;

/// PGH001
impl Violation for Eval {
#[derive_message_formats]
fn message(&self) -> String {
format!("No builtin `eval()` allowed")
}
}

/// PGH001
pub(crate) fn no_eval(checker: &mut Checker, func: &Expr) {
let Expr::Name(ast::ExprName { id, .. }) = func else {
return;
};
if id != "eval" {
return;
}
if !checker.semantic().is_builtin("eval") {
return;
}
checker
.diagnostics
.push(Diagnostic::new(Eval, func.range()));
}

This file was deleted.

This file was deleted.

This file was deleted.