Redirect PHG001 to S307 and PGH002 to G010 (#9756)

Follow-up to #9754 and #9689. Alternative to #9714.
Replaces #7506 and #7507
Same ideas as #9755
Part of #8931

Author: zanieb

crates/ruff_linter/resources/test/fixtures/pygrep_hooks/PGH001_0.py

@@ -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;
 
@@ -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);
             }

crates/ruff_linter/resources/test/fixtures/pygrep_hooks/PGH001_1.py

@@ -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),

crates/ruff_linter/resources/test/fixtures/pygrep_hooks/PGH002_0.py

@@ -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"),

crates/ruff_linter/resources/test/fixtures/pygrep_hooks/PGH002_1.py

@@ -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)]
@@ -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()))?;
 
@@ -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;
@@ -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 {
@@ -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)]

crates/ruff_linter/src/checkers/ast/analyze/expression.rs

@@ -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"))]

crates/ruff_linter/src/codes.rs

@@ -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.
 ///
@@ -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;
 
@@ -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);
-}

crates/ruff_linter/src/rule_redirects.rs

@@ -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.
 ///
@@ -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()));
-}