diff --git a/crates/ruff/resources/test/fixtures/flake8_logging_format/G010.py b/crates/ruff/resources/test/fixtures/flake8_logging_format/G010.py index 8d16e5e050b8d..cd22173b5acac 100644 --- a/crates/ruff/resources/test/fixtures/flake8_logging_format/G010.py +++ b/crates/ruff/resources/test/fixtures/flake8_logging_format/G010.py @@ -1,5 +1,8 @@ import logging from distutils import log +from logging_setup import logger + logging.warn("Hello World!") log.warn("Hello world!") # This shouldn't be considered as a logger candidate +logger.warn("Hello world!") diff --git a/crates/ruff/src/rules/flake8_blind_except/rules/blind_except.rs b/crates/ruff/src/rules/flake8_blind_except/rules/blind_except.rs index f0650afdda045..d80f2216aee67 100644 --- a/crates/ruff/src/rules/flake8_blind_except/rules/blind_except.rs +++ b/crates/ruff/src/rules/flake8_blind_except/rules/blind_except.rs @@ -95,7 +95,11 @@ pub(crate) fn blind_except( if body.iter().any(|stmt| { if let Stmt::Expr(ast::StmtExpr { value, range: _ }) = stmt { if let Expr::Call(ast::ExprCall { func, keywords, .. }) = value.as_ref() { - if logging::is_logger_candidate(func, checker.semantic()) { + if logging::is_logger_candidate( + func, + checker.semantic(), + &checker.settings.logger_objects, + ) { if let Some(attribute) = func.as_attribute_expr() { let attr = attribute.attr.as_str(); if attr == "exception" { diff --git a/crates/ruff/src/rules/flake8_logging_format/mod.rs b/crates/ruff/src/rules/flake8_logging_format/mod.rs index c3341174826c2..81c2595e60da4 100644 --- a/crates/ruff/src/rules/flake8_logging_format/mod.rs +++ b/crates/ruff/src/rules/flake8_logging_format/mod.rs @@ -31,16 +31,19 @@ mod tests { let snapshot = path.to_string_lossy().into_owned(); let diagnostics = test_path( Path::new("flake8_logging_format").join(path).as_path(), - &settings::Settings::for_rules(vec![ - Rule::LoggingStringFormat, - Rule::LoggingPercentFormat, - Rule::LoggingStringConcat, - Rule::LoggingFString, - Rule::LoggingWarn, - Rule::LoggingExtraAttrClash, - Rule::LoggingExcInfo, - Rule::LoggingRedundantExcInfo, - ]), + &settings::Settings { + logger_objects: vec!["logging_setup.logger".to_string()], + ..settings::Settings::for_rules(vec![ + Rule::LoggingStringFormat, + Rule::LoggingPercentFormat, + Rule::LoggingStringConcat, + Rule::LoggingFString, + Rule::LoggingWarn, + Rule::LoggingExtraAttrClash, + Rule::LoggingExcInfo, + Rule::LoggingRedundantExcInfo, + ]) + }, )?; assert_messages!(snapshot, diagnostics); Ok(()) diff --git a/crates/ruff/src/rules/flake8_logging_format/rules/logging_call.rs b/crates/ruff/src/rules/flake8_logging_format/rules/logging_call.rs index cbfebaf121284..bd5de7e19e8d5 100644 --- a/crates/ruff/src/rules/flake8_logging_format/rules/logging_call.rs +++ b/crates/ruff/src/rules/flake8_logging_format/rules/logging_call.rs @@ -158,7 +158,7 @@ pub(crate) fn logging_call( args: &[Expr], keywords: &[Keyword], ) { - if !logging::is_logger_candidate(func, checker.semantic()) { + if !logging::is_logger_candidate(func, checker.semantic(), &checker.settings.logger_objects) { return; } diff --git a/crates/ruff/src/rules/flake8_logging_format/snapshots/ruff__rules__flake8_logging_format__tests__G010.py.snap b/crates/ruff/src/rules/flake8_logging_format/snapshots/ruff__rules__flake8_logging_format__tests__G010.py.snap index b0a57cc6d18c3..e426e51f942bc 100644 --- a/crates/ruff/src/rules/flake8_logging_format/snapshots/ruff__rules__flake8_logging_format__tests__G010.py.snap +++ b/crates/ruff/src/rules/flake8_logging_format/snapshots/ruff__rules__flake8_logging_format__tests__G010.py.snap @@ -1,22 +1,40 @@ --- source: crates/ruff/src/rules/flake8_logging_format/mod.rs --- -G010.py:4:9: G010 [*] Logging statement uses `warn` instead of `warning` +G010.py:6:9: G010 [*] Logging statement uses `warn` instead of `warning` | -2 | from distutils import log -3 | -4 | logging.warn("Hello World!") +4 | from logging_setup import logger +5 | +6 | logging.warn("Hello World!") | ^^^^ G010 -5 | log.warn("Hello world!") # This shouldn't be considered as a logger candidate +7 | log.warn("Hello world!") # This shouldn't be considered as a logger candidate +8 | logger.warn("Hello world!") | = help: Convert to `warn` ℹ Fix -1 1 | import logging -2 2 | from distutils import log 3 3 | -4 |-logging.warn("Hello World!") - 4 |+logging.warning("Hello World!") -5 5 | log.warn("Hello world!") # This shouldn't be considered as a logger candidate +4 4 | from logging_setup import logger +5 5 | +6 |-logging.warn("Hello World!") + 6 |+logging.warning("Hello World!") +7 7 | log.warn("Hello world!") # This shouldn't be considered as a logger candidate +8 8 | logger.warn("Hello world!") + +G010.py:8:8: G010 [*] Logging statement uses `warn` instead of `warning` + | +6 | logging.warn("Hello World!") +7 | log.warn("Hello world!") # This shouldn't be considered as a logger candidate +8 | logger.warn("Hello world!") + | ^^^^ G010 + | + = help: Convert to `warn` + +ℹ Fix +5 5 | +6 6 | logging.warn("Hello World!") +7 7 | log.warn("Hello world!") # This shouldn't be considered as a logger candidate +8 |-logger.warn("Hello world!") + 8 |+logger.warning("Hello world!") diff --git a/crates/ruff/src/rules/pylint/rules/logging.rs b/crates/ruff/src/rules/pylint/rules/logging.rs index 8391113612c7e..b301530737601 100644 --- a/crates/ruff/src/rules/pylint/rules/logging.rs +++ b/crates/ruff/src/rules/pylint/rules/logging.rs @@ -102,48 +102,55 @@ pub(crate) fn logging_call( return; } - if !logging::is_logger_candidate(func, checker.semantic()) { + if !logging::is_logger_candidate(func, checker.semantic(), &checker.settings.logger_objects) { return; } - if let Expr::Attribute(ast::ExprAttribute { attr, .. }) = func { - if LoggingLevel::from_attribute(attr.as_str()).is_some() { - let call_args = SimpleCallArgs::new(args, keywords); - if let Some(Expr::Constant(ast::ExprConstant { - value: Constant::Str(value), - .. - })) = call_args.argument("msg", 0) - { - if let Ok(summary) = CFormatSummary::try_from(value.as_str()) { - if summary.starred { - return; - } - if !summary.keywords.is_empty() { - return; - } - - let message_args = call_args.num_args() - 1; - - if checker.enabled(Rule::LoggingTooManyArgs) { - if summary.num_positional < message_args { - checker - .diagnostics - .push(Diagnostic::new(LoggingTooManyArgs, func.range())); - } - } - - if checker.enabled(Rule::LoggingTooFewArgs) { - if message_args > 0 - && call_args.num_kwargs() == 0 - && summary.num_positional > message_args - { - checker - .diagnostics - .push(Diagnostic::new(LoggingTooFewArgs, func.range())); - } - } - } - } + let Expr::Attribute(ast::ExprAttribute { attr, .. }) = func else { + return; + }; + + if LoggingLevel::from_attribute(attr.as_str()).is_none() { + return; + } + + let call_args = SimpleCallArgs::new(args, keywords); + let Some(Expr::Constant(ast::ExprConstant { + value: Constant::Str(value), + .. + })) = call_args.argument("msg", 0) + else { + return; + }; + + let Ok(summary) = CFormatSummary::try_from(value.as_str()) else { + return; + }; + + if summary.starred { + return; + } + + if !summary.keywords.is_empty() { + return; + } + + let message_args = call_args.num_args() - 1; + + if checker.enabled(Rule::LoggingTooManyArgs) { + if summary.num_positional < message_args { + checker + .diagnostics + .push(Diagnostic::new(LoggingTooManyArgs, func.range())); + } + } + + if checker.enabled(Rule::LoggingTooFewArgs) { + if message_args > 0 && call_args.num_kwargs() == 0 && summary.num_positional > message_args + { + checker + .diagnostics + .push(Diagnostic::new(LoggingTooFewArgs, func.range())); } } } diff --git a/crates/ruff/src/rules/tryceratops/helpers.rs b/crates/ruff/src/rules/tryceratops/helpers.rs index d4a5655893466..c103922a1a2b7 100644 --- a/crates/ruff/src/rules/tryceratops/helpers.rs +++ b/crates/ruff/src/rules/tryceratops/helpers.rs @@ -8,13 +8,15 @@ use ruff_python_semantic::SemanticModel; /// Collect `logging`-like calls from an AST. pub(super) struct LoggerCandidateVisitor<'a, 'b> { semantic: &'a SemanticModel<'b>, + logger_objects: &'a [String], pub(super) calls: Vec<&'b ast::ExprCall>, } impl<'a, 'b> LoggerCandidateVisitor<'a, 'b> { - pub(super) fn new(semantic: &'a SemanticModel<'b>) -> Self { + pub(super) fn new(semantic: &'a SemanticModel<'b>, logger_objects: &'a [String]) -> Self { LoggerCandidateVisitor { semantic, + logger_objects, calls: Vec::new(), } } @@ -23,7 +25,7 @@ impl<'a, 'b> LoggerCandidateVisitor<'a, 'b> { impl<'a, 'b> Visitor<'b> for LoggerCandidateVisitor<'a, 'b> { fn visit_expr(&mut self, expr: &'b Expr) { if let Expr::Call(call) = expr { - if logging::is_logger_candidate(&call.func, self.semantic) { + if logging::is_logger_candidate(&call.func, self.semantic, self.logger_objects) { self.calls.push(call); } } diff --git a/crates/ruff/src/rules/tryceratops/rules/error_instead_of_exception.rs b/crates/ruff/src/rules/tryceratops/rules/error_instead_of_exception.rs index 4e111e7e9937c..d0a5fd879539f 100644 --- a/crates/ruff/src/rules/tryceratops/rules/error_instead_of_exception.rs +++ b/crates/ruff/src/rules/tryceratops/rules/error_instead_of_exception.rs @@ -58,7 +58,8 @@ pub(crate) fn error_instead_of_exception(checker: &mut Checker, handlers: &[Exce for handler in handlers { let ExceptHandler::ExceptHandler(ast::ExceptHandlerExceptHandler { body, .. }) = handler; let calls = { - let mut visitor = LoggerCandidateVisitor::new(checker.semantic()); + let mut visitor = + LoggerCandidateVisitor::new(checker.semantic(), &checker.settings.logger_objects); visitor.visit_body(body); visitor.calls }; diff --git a/crates/ruff/src/rules/tryceratops/rules/verbose_log_message.rs b/crates/ruff/src/rules/tryceratops/rules/verbose_log_message.rs index da2652d9ede78..5dfedad5e34ef 100644 --- a/crates/ruff/src/rules/tryceratops/rules/verbose_log_message.rs +++ b/crates/ruff/src/rules/tryceratops/rules/verbose_log_message.rs @@ -67,7 +67,8 @@ pub(crate) fn verbose_log_message(checker: &mut Checker, handlers: &[ExceptHandl // Find all calls to `logging.exception`. let calls = { - let mut visitor = LoggerCandidateVisitor::new(checker.semantic()); + let mut visitor = + LoggerCandidateVisitor::new(checker.semantic(), &checker.settings.logger_objects); visitor.visit_body(body); visitor.calls }; diff --git a/crates/ruff/src/settings/configuration.rs b/crates/ruff/src/settings/configuration.rs index d385444d6e2fc..d673fdd1109d3 100644 --- a/crates/ruff/src/settings/configuration.rs +++ b/crates/ruff/src/settings/configuration.rs @@ -59,13 +59,14 @@ pub struct Configuration { pub ignore_init_module_imports: Option, pub include: Option>, pub line_length: Option, - pub tab_size: Option, + pub logger_objects: Option>, pub namespace_packages: Option>, pub required_version: Option, pub respect_gitignore: Option, pub show_fixes: Option, pub show_source: Option, pub src: Option>, + pub tab_size: Option, pub target_version: Option, pub task_tags: Option>, pub typing_modules: Option>, @@ -223,6 +224,7 @@ impl Configuration { .transpose()?, target_version: options.target_version, task_tags: options.task_tags, + logger_objects: options.logger_objects, typing_modules: options.typing_modules, // Plugins flake8_annotations: options.flake8_annotations, @@ -291,6 +293,7 @@ impl Configuration { .ignore_init_module_imports .or(config.ignore_init_module_imports), line_length: self.line_length.or(config.line_length), + logger_objects: self.logger_objects.or(config.logger_objects), tab_size: self.tab_size.or(config.tab_size), namespace_packages: self.namespace_packages.or(config.namespace_packages), per_file_ignores: self.per_file_ignores.or(config.per_file_ignores), diff --git a/crates/ruff/src/settings/defaults.rs b/crates/ruff/src/settings/defaults.rs index 0893fa754326f..6ccc3bf09b9bd 100644 --- a/crates/ruff/src/settings/defaults.rs +++ b/crates/ruff/src/settings/defaults.rs @@ -84,12 +84,13 @@ impl Default for Settings { ignore_init_module_imports: false, include: FilePatternSet::try_from_vec(INCLUDE.clone()).unwrap(), line_length: LineLength::default(), - tab_size: TabSize::default(), + logger_objects: vec![], namespace_packages: vec![], per_file_ignores: vec![], + project_root: path_dedot::CWD.clone(), respect_gitignore: true, src: vec![path_dedot::CWD.clone()], - project_root: path_dedot::CWD.clone(), + tab_size: TabSize::default(), target_version: TARGET_VERSION, task_tags: TASK_TAGS.iter().map(ToString::to_string).collect(), typing_modules: vec![], diff --git a/crates/ruff/src/settings/mod.rs b/crates/ruff/src/settings/mod.rs index a7933b2540f79..3dfc8372cd8c0 100644 --- a/crates/ruff/src/settings/mod.rs +++ b/crates/ruff/src/settings/mod.rs @@ -101,9 +101,10 @@ pub struct Settings { pub external: FxHashSet, pub ignore_init_module_imports: bool, pub line_length: LineLength, - pub tab_size: TabSize, + pub logger_objects: Vec, pub namespace_packages: Vec, pub src: Vec, + pub tab_size: TabSize, pub task_tags: Vec, pub typing_modules: Vec, // Plugins @@ -189,6 +190,7 @@ impl Settings { .map(ToString::to_string) .collect() }), + logger_objects: config.logger_objects.unwrap_or_default(), typing_modules: config.typing_modules.unwrap_or_default(), // Plugins flake8_annotations: config diff --git a/crates/ruff/src/settings/options.rs b/crates/ruff/src/settings/options.rs index 697b630491753..2839f4a9fbc56 100644 --- a/crates/ruff/src/settings/options.rs +++ b/crates/ruff/src/settings/options.rs @@ -330,6 +330,30 @@ pub struct Options { required-version = "0.0.193" "# )] + #[option( + default = r#"[]"#, + value_type = "list[str]", + example = r#"logger-objects = ["logging_setup.logger"]"# + )] + /// A list of objects that should be treated equivalently to a + /// `logging.Logger` object. + /// + /// This is useful for ensuring proper diagnostics (e.g., to identify + /// `logging` deprecations and other best-practices) for projects that + /// re-export a `logging.Logger` object from a common module. + /// + /// For example, if you have a module `logging_setup.py` with the following + /// contents: + /// ```python + /// import logging + /// + /// logger = logging.getLogger(__name__) + /// ``` + /// + /// Adding `"logging_setup.logger"` to `logger-objects` will ensure that + /// `logging_setup.logger` is treated as a `logging.Logger` object when + /// imported from other modules (e.g., `from logging_setup import logger`). + pub logger_objects: Option>, /// Require a specific version of Ruff to be running (useful for unifying /// results across many environments, e.g., with a `pyproject.toml` /// file). @@ -463,7 +487,7 @@ pub struct Options { value_type = "list[str]", example = r#"typing-modules = ["airflow.typing_compat"]"# )] - /// A list of modules whose imports should be treated equivalently to + /// A list of modules whose exports should be treated equivalently to /// members of the `typing` module. /// /// This is useful for ensuring proper type annotation inference for diff --git a/crates/ruff_python_semantic/src/analyze/logging.rs b/crates/ruff_python_semantic/src/analyze/logging.rs index 48fbc3fb16392..0aa4624de4c39 100644 --- a/crates/ruff_python_semantic/src/analyze/logging.rs +++ b/crates/ruff_python_semantic/src/analyze/logging.rs @@ -1,7 +1,7 @@ -use rustpython_parser::ast::{self, Constant, Expr, Keyword}; +use rustpython_parser::ast::{self, Expr, Keyword}; -use ruff_python_ast::call_path::collect_call_path; -use ruff_python_ast::helpers::find_keyword; +use ruff_python_ast::call_path::{collect_call_path, from_qualified_name}; +use ruff_python_ast::helpers::{find_keyword, is_const_true}; use crate::model::SemanticModel; @@ -9,37 +9,53 @@ use crate::model::SemanticModel; /// `logging.error`, `logger.error`, `self.logger.error`, etc., but not /// arbitrary `foo.error` calls. /// -/// It even matches direct `logging.error` calls even if the `logging` module +/// It also matches direct `logging.error` calls when the `logging` module /// is aliased. Example: /// ```python /// import logging as bar /// -/// # This is detected to be a logger candidate +/// # This is detected to be a logger candidate. /// bar.error() /// ``` -pub fn is_logger_candidate(func: &Expr, semantic: &SemanticModel) -> bool { - if let Expr::Attribute(ast::ExprAttribute { value, .. }) = func { - let Some(call_path) = (if let Some(call_path) = semantic.resolve_call_path(value) { - if call_path - .first() - .map_or(false, |module| *module == "logging") - || call_path.as_slice() == ["flask", "current_app", "logger"] - { - Some(call_path) - } else { - None - } - } else { - collect_call_path(value) - }) else { - return false; - }; +pub fn is_logger_candidate( + func: &Expr, + semantic: &SemanticModel, + logger_objects: &[String], +) -> bool { + let Expr::Attribute(ast::ExprAttribute { value, .. }) = func else { + return false; + }; + + // If the symbol was imported from another module, ensure that it's either a user-specified + // logger object, the `logging` module itself, or `flask.current_app.logger`. + if let Some(call_path) = semantic.resolve_call_path(value) { + if matches!( + call_path.as_slice(), + ["logging"] | ["flask", "current_app", "logger"] + ) { + return true; + } + + if logger_objects + .iter() + .any(|logger| from_qualified_name(logger) == call_path) + { + return true; + } + + return false; + } + + // Otherwise, if the symbol was defined in the current module, match against some common + // logger names. + if let Some(call_path) = collect_call_path(value) { if let Some(tail) = call_path.last() { if tail.starts_with("log") || tail.ends_with("logger") || tail.ends_with("logging") { return true; } } } + false } @@ -49,23 +65,20 @@ pub fn exc_info<'a>(keywords: &'a [Keyword], semantic: &SemanticModel) -> Option let exc_info = find_keyword(keywords, "exc_info")?; // Ex) `logging.error("...", exc_info=True)` - if matches!( - exc_info.value, - Expr::Constant(ast::ExprConstant { - value: Constant::Bool(true), - .. - }) - ) { + if is_const_true(&exc_info.value) { return Some(exc_info); } // Ex) `logging.error("...", exc_info=sys.exc_info())` - if let Expr::Call(ast::ExprCall { func, .. }) = &exc_info.value { - if semantic.resolve_call_path(func).map_or(false, |call_path| { + if exc_info + .value + .as_call_expr() + .and_then(|call| semantic.resolve_call_path(&call.func)) + .map_or(false, |call_path| { matches!(call_path.as_slice(), ["sys", "exc_info"]) - }) { - return Some(exc_info); - } + }) + { + return Some(exc_info); } None diff --git a/crates/ruff_wasm/src/lib.rs b/crates/ruff_wasm/src/lib.rs index 1bd5f67bef68d..12887cc0c34a7 100644 --- a/crates/ruff_wasm/src/lib.rs +++ b/crates/ruff_wasm/src/lib.rs @@ -130,8 +130,8 @@ impl Workspace { external: Some(Vec::default()), ignore: Some(Vec::default()), line_length: Some(LineLength::default()), - tab_size: Some(TabSize::default()), select: Some(defaults::PREFIXES.to_vec()), + tab_size: Some(TabSize::default()), target_version: Some(defaults::TARGET_VERSION), // Ignore a bunch of options that don't make sense in a single-file editor. cache_dir: None, @@ -145,8 +145,9 @@ impl Workspace { fixable: None, force_exclude: None, format: None, - include: None, ignore_init_module_imports: None, + include: None, + logger_objects: None, namespace_packages: None, per_file_ignores: None, required_version: None, diff --git a/ruff.schema.json b/ruff.schema.json index 5eae79f20eb05..a8744b5b6150b 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -386,6 +386,16 @@ } ] }, + "logger-objects": { + "description": "A list of objects that should be treated equivalently to a `logging.Logger` object.\n\nThis is useful for ensuring proper diagnostics (e.g., to identify `logging` deprecations and other best-practices) for projects that re-export a `logging.Logger` object from a common module.\n\nFor example, if you have a module `logging_setup.py` with the following contents: ```python import logging\n\nlogger = logging.getLogger(__name__) ```\n\nAdding `\"logging_setup.logger\"` to `logger-objects` will ensure that `logging_setup.logger` is treated as a `logging.Logger` object when imported from other modules (e.g., `from logging_setup import logger`).", + "type": [ + "array", + "null" + ], + "items": { + "type": "string" + } + }, "mccabe": { "description": "Options for the `mccabe` plugin.", "anyOf": [ @@ -571,7 +581,7 @@ } }, "typing-modules": { - "description": "A list of modules whose imports should be treated equivalently to members of the `typing` module.\n\nThis is useful for ensuring proper type annotation inference for projects that re-export `typing` and `typing_extensions` members from a compatibility module. If omitted, any members imported from modules apart from `typing` and `typing_extensions` will be treated as ordinary Python objects.", + "description": "A list of modules whose exports should be treated equivalently to members of the `typing` module.\n\nThis is useful for ensuring proper type annotation inference for projects that re-export `typing` and `typing_extensions` members from a compatibility module. If omitted, any members imported from modules apart from `typing` and `typing_extensions` will be treated as ordinary Python objects.", "type": [ "array", "null"