Allow specification of `logging.Logger` re-exports via `logger-objects` (#5750)

## Summary

This PR adds a `logger-objects` setting that allows users to mark
specific symbols a `logging.Logger` objects. Currently, if a `logger` is
imported, we only flagged it as a `logging.Logger` if it comes exactly
from the `logging` module or is `flask.current_app.logger`.

This PR allows users to mark specific loggers, like
`logging_setup.logger`, to ensure that they're covered by the
`flake8-logging-format` rules and others.

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`).

Closes https://github.com/astral-sh/ruff/issues/5694.
This commit is contained in:
Charlie Marsh 2023-07-24 00:38:20 -04:00 committed by GitHub
parent 727153cf45
commit f9726af4ef
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
16 changed files with 197 additions and 104 deletions

View File

@ -1,5 +1,8 @@
import logging import logging
from distutils import log from distutils import log
from logging_setup import logger
logging.warn("Hello World!") logging.warn("Hello World!")
log.warn("Hello world!") # This shouldn't be considered as a logger candidate log.warn("Hello world!") # This shouldn't be considered as a logger candidate
logger.warn("Hello world!")

View File

@ -95,7 +95,11 @@ pub(crate) fn blind_except(
if body.iter().any(|stmt| { if body.iter().any(|stmt| {
if let Stmt::Expr(ast::StmtExpr { value, range: _ }) = stmt { if let Stmt::Expr(ast::StmtExpr { value, range: _ }) = stmt {
if let Expr::Call(ast::ExprCall { func, keywords, .. }) = value.as_ref() { 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() { if let Some(attribute) = func.as_attribute_expr() {
let attr = attribute.attr.as_str(); let attr = attribute.attr.as_str();
if attr == "exception" { if attr == "exception" {

View File

@ -31,16 +31,19 @@ mod tests {
let snapshot = path.to_string_lossy().into_owned(); let snapshot = path.to_string_lossy().into_owned();
let diagnostics = test_path( let diagnostics = test_path(
Path::new("flake8_logging_format").join(path).as_path(), Path::new("flake8_logging_format").join(path).as_path(),
&settings::Settings::for_rules(vec![ &settings::Settings {
Rule::LoggingStringFormat, logger_objects: vec!["logging_setup.logger".to_string()],
Rule::LoggingPercentFormat, ..settings::Settings::for_rules(vec![
Rule::LoggingStringConcat, Rule::LoggingStringFormat,
Rule::LoggingFString, Rule::LoggingPercentFormat,
Rule::LoggingWarn, Rule::LoggingStringConcat,
Rule::LoggingExtraAttrClash, Rule::LoggingFString,
Rule::LoggingExcInfo, Rule::LoggingWarn,
Rule::LoggingRedundantExcInfo, Rule::LoggingExtraAttrClash,
]), Rule::LoggingExcInfo,
Rule::LoggingRedundantExcInfo,
])
},
)?; )?;
assert_messages!(snapshot, diagnostics); assert_messages!(snapshot, diagnostics);
Ok(()) Ok(())

View File

@ -158,7 +158,7 @@ pub(crate) fn logging_call(
args: &[Expr], args: &[Expr],
keywords: &[Keyword], keywords: &[Keyword],
) { ) {
if !logging::is_logger_candidate(func, checker.semantic()) { if !logging::is_logger_candidate(func, checker.semantic(), &checker.settings.logger_objects) {
return; return;
} }

View File

@ -1,22 +1,40 @@
--- ---
source: crates/ruff/src/rules/flake8_logging_format/mod.rs 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 4 | from logging_setup import logger
3 | 5 |
4 | logging.warn("Hello World!") 6 | logging.warn("Hello World!")
| ^^^^ G010 | ^^^^ 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` = help: Convert to `warn`
Fix Fix
1 1 | import logging
2 2 | from distutils import log
3 3 | 3 3 |
4 |-logging.warn("Hello World!") 4 4 | from logging_setup import logger
4 |+logging.warning("Hello World!") 5 5 |
5 5 | log.warn("Hello world!") # This shouldn't be considered as a logger candidate 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!")

View File

@ -102,48 +102,55 @@ pub(crate) fn logging_call(
return; return;
} }
if !logging::is_logger_candidate(func, checker.semantic()) { if !logging::is_logger_candidate(func, checker.semantic(), &checker.settings.logger_objects) {
return; return;
} }
if let Expr::Attribute(ast::ExprAttribute { attr, .. }) = func { let Expr::Attribute(ast::ExprAttribute { attr, .. }) = func else {
if LoggingLevel::from_attribute(attr.as_str()).is_some() { return;
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 LoggingLevel::from_attribute(attr.as_str()).is_none() {
return;
}
if checker.enabled(Rule::LoggingTooManyArgs) { let call_args = SimpleCallArgs::new(args, keywords);
if summary.num_positional < message_args { let Some(Expr::Constant(ast::ExprConstant {
checker value: Constant::Str(value),
.diagnostics ..
.push(Diagnostic::new(LoggingTooManyArgs, func.range())); })) = call_args.argument("msg", 0)
} else {
} return;
};
if checker.enabled(Rule::LoggingTooFewArgs) { let Ok(summary) = CFormatSummary::try_from(value.as_str()) else {
if message_args > 0 return;
&& call_args.num_kwargs() == 0 };
&& summary.num_positional > message_args
{ if summary.starred {
checker return;
.diagnostics }
.push(Diagnostic::new(LoggingTooFewArgs, func.range()));
} 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()));
} }
} }
} }

View File

@ -8,13 +8,15 @@ use ruff_python_semantic::SemanticModel;
/// Collect `logging`-like calls from an AST. /// Collect `logging`-like calls from an AST.
pub(super) struct LoggerCandidateVisitor<'a, 'b> { pub(super) struct LoggerCandidateVisitor<'a, 'b> {
semantic: &'a SemanticModel<'b>, semantic: &'a SemanticModel<'b>,
logger_objects: &'a [String],
pub(super) calls: Vec<&'b ast::ExprCall>, pub(super) calls: Vec<&'b ast::ExprCall>,
} }
impl<'a, 'b> LoggerCandidateVisitor<'a, 'b> { 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 { LoggerCandidateVisitor {
semantic, semantic,
logger_objects,
calls: Vec::new(), calls: Vec::new(),
} }
} }
@ -23,7 +25,7 @@ impl<'a, 'b> LoggerCandidateVisitor<'a, 'b> {
impl<'a, 'b> Visitor<'b> for LoggerCandidateVisitor<'a, 'b> { impl<'a, 'b> Visitor<'b> for LoggerCandidateVisitor<'a, 'b> {
fn visit_expr(&mut self, expr: &'b Expr) { fn visit_expr(&mut self, expr: &'b Expr) {
if let Expr::Call(call) = 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); self.calls.push(call);
} }
} }

View File

@ -58,7 +58,8 @@ pub(crate) fn error_instead_of_exception(checker: &mut Checker, handlers: &[Exce
for handler in handlers { for handler in handlers {
let ExceptHandler::ExceptHandler(ast::ExceptHandlerExceptHandler { body, .. }) = handler; let ExceptHandler::ExceptHandler(ast::ExceptHandlerExceptHandler { body, .. }) = handler;
let calls = { 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.visit_body(body);
visitor.calls visitor.calls
}; };

View File

@ -67,7 +67,8 @@ pub(crate) fn verbose_log_message(checker: &mut Checker, handlers: &[ExceptHandl
// Find all calls to `logging.exception`. // Find all calls to `logging.exception`.
let calls = { 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.visit_body(body);
visitor.calls visitor.calls
}; };

View File

@ -59,13 +59,14 @@ pub struct Configuration {
pub ignore_init_module_imports: Option<bool>, pub ignore_init_module_imports: Option<bool>,
pub include: Option<Vec<FilePattern>>, pub include: Option<Vec<FilePattern>>,
pub line_length: Option<LineLength>, pub line_length: Option<LineLength>,
pub tab_size: Option<TabSize>, pub logger_objects: Option<Vec<String>>,
pub namespace_packages: Option<Vec<PathBuf>>, pub namespace_packages: Option<Vec<PathBuf>>,
pub required_version: Option<Version>, pub required_version: Option<Version>,
pub respect_gitignore: Option<bool>, pub respect_gitignore: Option<bool>,
pub show_fixes: Option<bool>, pub show_fixes: Option<bool>,
pub show_source: Option<bool>, pub show_source: Option<bool>,
pub src: Option<Vec<PathBuf>>, pub src: Option<Vec<PathBuf>>,
pub tab_size: Option<TabSize>,
pub target_version: Option<PythonVersion>, pub target_version: Option<PythonVersion>,
pub task_tags: Option<Vec<String>>, pub task_tags: Option<Vec<String>>,
pub typing_modules: Option<Vec<String>>, pub typing_modules: Option<Vec<String>>,
@ -223,6 +224,7 @@ impl Configuration {
.transpose()?, .transpose()?,
target_version: options.target_version, target_version: options.target_version,
task_tags: options.task_tags, task_tags: options.task_tags,
logger_objects: options.logger_objects,
typing_modules: options.typing_modules, typing_modules: options.typing_modules,
// Plugins // Plugins
flake8_annotations: options.flake8_annotations, flake8_annotations: options.flake8_annotations,
@ -291,6 +293,7 @@ impl Configuration {
.ignore_init_module_imports .ignore_init_module_imports
.or(config.ignore_init_module_imports), .or(config.ignore_init_module_imports),
line_length: self.line_length.or(config.line_length), 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), tab_size: self.tab_size.or(config.tab_size),
namespace_packages: self.namespace_packages.or(config.namespace_packages), namespace_packages: self.namespace_packages.or(config.namespace_packages),
per_file_ignores: self.per_file_ignores.or(config.per_file_ignores), per_file_ignores: self.per_file_ignores.or(config.per_file_ignores),

View File

@ -84,12 +84,13 @@ impl Default for Settings {
ignore_init_module_imports: false, ignore_init_module_imports: false,
include: FilePatternSet::try_from_vec(INCLUDE.clone()).unwrap(), include: FilePatternSet::try_from_vec(INCLUDE.clone()).unwrap(),
line_length: LineLength::default(), line_length: LineLength::default(),
tab_size: TabSize::default(), logger_objects: vec![],
namespace_packages: vec![], namespace_packages: vec![],
per_file_ignores: vec![], per_file_ignores: vec![],
project_root: path_dedot::CWD.clone(),
respect_gitignore: true, respect_gitignore: true,
src: vec![path_dedot::CWD.clone()], src: vec![path_dedot::CWD.clone()],
project_root: path_dedot::CWD.clone(), tab_size: TabSize::default(),
target_version: TARGET_VERSION, target_version: TARGET_VERSION,
task_tags: TASK_TAGS.iter().map(ToString::to_string).collect(), task_tags: TASK_TAGS.iter().map(ToString::to_string).collect(),
typing_modules: vec![], typing_modules: vec![],

View File

@ -101,9 +101,10 @@ pub struct Settings {
pub external: FxHashSet<String>, pub external: FxHashSet<String>,
pub ignore_init_module_imports: bool, pub ignore_init_module_imports: bool,
pub line_length: LineLength, pub line_length: LineLength,
pub tab_size: TabSize, pub logger_objects: Vec<String>,
pub namespace_packages: Vec<PathBuf>, pub namespace_packages: Vec<PathBuf>,
pub src: Vec<PathBuf>, pub src: Vec<PathBuf>,
pub tab_size: TabSize,
pub task_tags: Vec<String>, pub task_tags: Vec<String>,
pub typing_modules: Vec<String>, pub typing_modules: Vec<String>,
// Plugins // Plugins
@ -189,6 +190,7 @@ impl Settings {
.map(ToString::to_string) .map(ToString::to_string)
.collect() .collect()
}), }),
logger_objects: config.logger_objects.unwrap_or_default(),
typing_modules: config.typing_modules.unwrap_or_default(), typing_modules: config.typing_modules.unwrap_or_default(),
// Plugins // Plugins
flake8_annotations: config flake8_annotations: config

View File

@ -330,6 +330,30 @@ pub struct Options {
required-version = "0.0.193" 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<Vec<String>>,
/// Require a specific version of Ruff to be running (useful for unifying /// Require a specific version of Ruff to be running (useful for unifying
/// results across many environments, e.g., with a `pyproject.toml` /// results across many environments, e.g., with a `pyproject.toml`
/// file). /// file).
@ -463,7 +487,7 @@ pub struct Options {
value_type = "list[str]", value_type = "list[str]",
example = r#"typing-modules = ["airflow.typing_compat"]"# 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. /// members of the `typing` module.
/// ///
/// This is useful for ensuring proper type annotation inference for /// This is useful for ensuring proper type annotation inference for

View File

@ -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::call_path::{collect_call_path, from_qualified_name};
use ruff_python_ast::helpers::find_keyword; use ruff_python_ast::helpers::{find_keyword, is_const_true};
use crate::model::SemanticModel; use crate::model::SemanticModel;
@ -9,37 +9,53 @@ use crate::model::SemanticModel;
/// `logging.error`, `logger.error`, `self.logger.error`, etc., but not /// `logging.error`, `logger.error`, `self.logger.error`, etc., but not
/// arbitrary `foo.error` calls. /// 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: /// is aliased. Example:
/// ```python /// ```python
/// import logging as bar /// import logging as bar
/// ///
/// # This is detected to be a logger candidate /// # This is detected to be a logger candidate.
/// bar.error() /// bar.error()
/// ``` /// ```
pub fn is_logger_candidate(func: &Expr, semantic: &SemanticModel) -> bool { pub fn is_logger_candidate(
if let Expr::Attribute(ast::ExprAttribute { value, .. }) = func { func: &Expr,
let Some(call_path) = (if let Some(call_path) = semantic.resolve_call_path(value) { semantic: &SemanticModel,
if call_path logger_objects: &[String],
.first() ) -> bool {
.map_or(false, |module| *module == "logging") let Expr::Attribute(ast::ExprAttribute { value, .. }) = func else {
|| call_path.as_slice() == ["flask", "current_app", "logger"] return false;
{ };
Some(call_path)
} else { // If the symbol was imported from another module, ensure that it's either a user-specified
None // logger object, the `logging` module itself, or `flask.current_app.logger`.
} if let Some(call_path) = semantic.resolve_call_path(value) {
} else { if matches!(
collect_call_path(value) call_path.as_slice(),
}) else { ["logging"] | ["flask", "current_app", "logger"]
return false; ) {
}; 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 let Some(tail) = call_path.last() {
if tail.starts_with("log") || tail.ends_with("logger") || tail.ends_with("logging") { if tail.starts_with("log") || tail.ends_with("logger") || tail.ends_with("logging") {
return true; return true;
} }
} }
} }
false false
} }
@ -49,23 +65,20 @@ pub fn exc_info<'a>(keywords: &'a [Keyword], semantic: &SemanticModel) -> Option
let exc_info = find_keyword(keywords, "exc_info")?; let exc_info = find_keyword(keywords, "exc_info")?;
// Ex) `logging.error("...", exc_info=True)` // Ex) `logging.error("...", exc_info=True)`
if matches!( if is_const_true(&exc_info.value) {
exc_info.value,
Expr::Constant(ast::ExprConstant {
value: Constant::Bool(true),
..
})
) {
return Some(exc_info); return Some(exc_info);
} }
// Ex) `logging.error("...", exc_info=sys.exc_info())` // Ex) `logging.error("...", exc_info=sys.exc_info())`
if let Expr::Call(ast::ExprCall { func, .. }) = &exc_info.value { if exc_info
if semantic.resolve_call_path(func).map_or(false, |call_path| { .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"]) matches!(call_path.as_slice(), ["sys", "exc_info"])
}) { })
return Some(exc_info); {
} return Some(exc_info);
} }
None None

View File

@ -130,8 +130,8 @@ impl Workspace {
external: Some(Vec::default()), external: Some(Vec::default()),
ignore: Some(Vec::default()), ignore: Some(Vec::default()),
line_length: Some(LineLength::default()), line_length: Some(LineLength::default()),
tab_size: Some(TabSize::default()),
select: Some(defaults::PREFIXES.to_vec()), select: Some(defaults::PREFIXES.to_vec()),
tab_size: Some(TabSize::default()),
target_version: Some(defaults::TARGET_VERSION), target_version: Some(defaults::TARGET_VERSION),
// Ignore a bunch of options that don't make sense in a single-file editor. // Ignore a bunch of options that don't make sense in a single-file editor.
cache_dir: None, cache_dir: None,
@ -145,8 +145,9 @@ impl Workspace {
fixable: None, fixable: None,
force_exclude: None, force_exclude: None,
format: None, format: None,
include: None,
ignore_init_module_imports: None, ignore_init_module_imports: None,
include: None,
logger_objects: None,
namespace_packages: None, namespace_packages: None,
per_file_ignores: None, per_file_ignores: None,
required_version: None, required_version: None,

12
ruff.schema.json generated
View File

@ -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": { "mccabe": {
"description": "Options for the `mccabe` plugin.", "description": "Options for the `mccabe` plugin.",
"anyOf": [ "anyOf": [
@ -571,7 +581,7 @@
} }
}, },
"typing-modules": { "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": [ "type": [
"array", "array",
"null" "null"