[`flake8-logging`] `.exception()` and `exc_info=` outside exception handlers (`LOG004`, `LOG014`) by InSyncWithFoo · Pull Request #15799 · astral-sh/ruff

InSyncWithFoo · 2025-01-29T00:46:36Z

Summary

Part of #7248.

This change adds two new rules: LOG004 and LOG014. They are closely related and thus share the same handlers detecting logic. Tests and documentation are adapted from that of the upstream linter.

Test Plan

cargo nextest run and cargo insta test.

github-actions · 2025-01-29T00:54:18Z

`ruff-ecosystem` results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+38 -0 violations, +0 -0 fixes in 4 projects; 51 projects unchanged)

apache/airflow (+12 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --no-fix --output-format concise --preview --select ALL

+ providers/edge/src/airflow/providers/edge/worker_api/auth.py:59:5: LOG004 `.exception()` call outside exception handlers
+ providers/edge/src/airflow/providers/edge/worker_api/routes/_v2_routes.py:56:5: LOG004 `.exception()` call outside exception handlers
+ providers/src/airflow/providers/cncf/kubernetes/operators/pod.py:1290:13: LOG004 `.exception()` call outside exception handlers
+ providers/src/airflow/providers/google/cloud/operators/cloud_sql.py:1053:13: LOG004 `.exception()` call outside exception handlers
+ providers/src/airflow/providers/google/cloud/operators/dataproc.py:1754:13: LOG004 `.exception()` call outside exception handlers
+ providers/src/airflow/providers/google/cloud/operators/dataproc.py:1915:13: LOG004 `.exception()` call outside exception handlers
+ providers/src/airflow/providers/google/cloud/operators/dataproc.py:2379:17: LOG004 `.exception()` call outside exception handlers
+ providers/src/airflow/providers/google/cloud/operators/kubernetes_engine.py:305:13: LOG004 `.exception()` call outside exception handlers
... 4 additional changes omitted for rule LOG004
+ task_sdk/src/airflow/sdk/execution_time/supervisor.py:741:13: LOG014 `exc_info=` outside exception handlers

apache/superset (+15 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --no-fix --output-format concise --preview --select ALL

+ superset/commands/report/base.py:106:75: LOG014 `exc_info=` outside exception handlers
+ superset/db_engine_specs/clickhouse.py:181:17: LOG014 `exc_info=` outside exception handlers
+ superset/db_engine_specs/hive.py:592:13: LOG014 `exc_info=` outside exception handlers
+ superset/sql_lab.py:137:69: LOG014 `exc_info=` outside exception handlers
+ superset/sql_lab.py:141:75: LOG014 `exc_info=` outside exception handlers
+ superset/tasks/cache.py:298:31: LOG014 `exc_info=` outside exception handlers
+ superset/utils/core.py:572:43: LOG014 `exc_info=` outside exception handlers
+ superset/utils/decorators.py:231:13: LOG004 `.exception()` call outside exception handlers
+ superset/views/error_handling.py:140:50: LOG014 `exc_info=` outside exception handlers
+ superset/views/error_handling.py:145:51: LOG014 `exc_info=` outside exception handlers
+ superset/views/error_handling.py:151:52: LOG014 `exc_info=` outside exception handlers
... 4 additional changes omitted for rule LOG014
+ superset/views/error_handling.py:210:9: LOG004 `.exception()` call outside exception handlers

rotki/rotki (+2 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --no-fix --output-format concise --preview

+ rotkehlchen/api/server.py:438:13: LOG004 `.exception()` call outside exception handlers
+ rotkehlchen/api/server.py:441:13: LOG014 `exc_info=` outside exception handlers

zulip/zulip (+9 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --no-fix --output-format concise --preview --select ALL

+ zerver/decorator.py:333:5: LOG004 `.exception()` call outside exception handlers
+ zerver/decorator.py:342:9: LOG004 `.exception()` call outside exception handlers
+ zerver/decorator.py:344:9: LOG004 `.exception()` call outside exception handlers
+ zerver/decorator.py:346:9: LOG004 `.exception()` call outside exception handlers
+ zerver/lib/push_notifications.py:1334:21: LOG014 `exc_info=` outside exception handlers
+ zerver/lib/remote_server.py:291:9: LOG004 `.exception()` call outside exception handlers
+ zerver/worker/base.py:265:17: LOG004 `.exception()` call outside exception handlers
... 3 additional changes omitted for rule LOG004

Changes by rule (2 rules affected)

code	total	+ violation	- violation	+ fix	- fix
LOG004	22	22	0	0	0
LOG014	16	16	0	0	0

InSyncWithFoo · 2025-01-29T00:59:41Z

They could have been defined in the same module/function, but I was worried that there might be too much coupling between them. I'll merge them if that is indeed the better approach.

InSyncWithFoo · 2025-01-29T01:21:22Z

I just realized #14682 exists (which, unfortunately, hasn't been active for more than a month). I'm willing to remove my LOG014 in favor of that and rewrite LOG004 accordingly if that is desirable.

bluetech · 2025-01-29T16:33:34Z

I see that the code does handle it, but I think it would be good to test that the lint doesn't trigger for exc_info=False, exc_info=exc.

InSyncWithFoo · 2025-01-29T16:41:43Z

@bluetech Done for the former and already done for the latter.

crates/ruff_linter/src/rules/flake8_logging/mod.rs

MichaReiser

Thanks. This overall looks good. We should add some tests with custom logger objects (in dedicated files) to see if the seen_module short-circuit check is valid.

crates/ruff_linter/src/rules/flake8_logging/rules/helpers.rs

crates/ruff_linter/src/rules/flake8_logging/rules/exception_call_outside_handlers.rs

crates/ruff_linter/src/rules/flake8_logging/rules/exc_info_outside_handlers.rs

MichaReiser · 2025-02-03T08:35:14Z

The ecosystem check also report a few false-positives e.g:

https://github.com/apache/airflow/blob/41403c1221025f5ffe737f1a8e2c488212b3c493/providers/edge/src/airflow/providers/edge/worker_api/auth.py#L59

This was discussed in your linked PR, see #14682 (comment) and #14682 (comment)

I'm interested in your opinion on this.

I'm somewhat leaning towards using noqa comments for such functions because:

The function should document that it should only be called from inside a except handler and calling it outside of it may lead to incorrect result. This is similar to documenting unsafe in Rust.

Did you review the other ecosystem checks? Considering that there are more LOG014, maybe consider splitting the PR into two?

…andlers (`LOG004`, `LOG014`)

InSyncWithFoo · 2025-02-03T14:31:27Z

Did you review the other ecosystem checks?

I'm aware of them. The original rule also intentionally reports them, so I think it's fine.

As for splitting, I'm not sure. They are closely related enough.

MichaReiser · 2025-02-03T19:12:49Z

Huh, this one is interesting. Would you mind taking a look at why it is flagged

https://github.com/apache/airflow/blob/3004da95e97ba79eba2ab6b743a75e3f3f8dc170/tests/core/test_settings.py#L168

crates/ruff_linter/src/codes.rs

InSyncWithFoo · 2025-02-03T22:23:59Z

@MichaReiser Fixed. The method name should also have been checked.

MichaReiser · 2025-02-04T08:52:07Z

Thanks

* main: (66 commits) [red-knot] Use ternary decision diagrams (TDDs) for visibility constraints (#15861) [`pyupgrade`] Rename private type parameters in PEP 695 generics (`UP049`) (#15862) Simplify the `StringFlags` trait (#15944) [`flake8-pyi`] Make `PYI019` autofixable for `.py` files in preview mode as well as stubs (#15889) Docs (`linter.md`): clarify that Python files are always searched for in subdirectories (#15882) [`flake8-pyi`] Make PEP-695 functions with multiple type parameters fixable by PYI019 again (#15938) [red-knot] Use unambiguous invalid-syntax-construct for suppression comment test (#15933) Make `Binding::range()` point to the range of a type parameter's name, not the full type parameter (#15935) Update black deviations (#15928) [red-knot] MDTest: Fix line numbers in error messages (#15932) Preserve triple quotes and prefixes for strings (#15818) [red-knot] Hand-written MDTest parser (#15926) [`pylint`] Fix missing parens in unsafe fix for `unnecessary-dunder-call` (`PLC2801`) (#15762) nit: docs for ignore & select (#15883) [airflow] `BashOperator` has been moved to `airflow.providers.standard.operators.bash.BashOperator` (AIR302) (#15922) [`flake8-logging`] `.exception()` and `exc_info=` outside exception handlers (`LOG004`, `LOG014`) (#15799) [red-knot] Enforce specifying paths for mdtest code blocks in a separate preceding line (#15890) [red-knot] Internal refactoring of visibility constraints API (#15913) [red-knot] Implicit instance attributes (#15811) [`flake8-comprehensions`] Handle extraneous parentheses around list comprehension (`C403`) (#15877) ...

dylwil3 added rule Implementing or modifying a lint rule preview Related to preview mode features labels Jan 30, 2025

MichaReiser reviewed Feb 3, 2025

View reviewed changes

crates/ruff_linter/src/rules/flake8_logging/mod.rs Outdated Show resolved Hide resolved

MichaReiser requested changes Feb 3, 2025

View reviewed changes

InSyncWithFoo added 4 commits February 3, 2025 14:08

[flake8-logging] .exception() and exc_info= outside exception h…

8c24952

…andlers (`LOG004`, `LOG014`)

Fixes

7f3a85a

Add a few more testcases

6b5bfda

Per review

476e2b5

InSyncWithFoo force-pushed the LOG004 branch from 91d22e2 to 476e2b5 Compare February 3, 2025 14:31

MichaReiser approved these changes Feb 3, 2025

View reviewed changes

crates/ruff_linter/src/codes.rs Outdated Show resolved Hide resolved

InSyncWithFoo added 2 commits February 3, 2025 22:06

Rename

2f5fc88

Fix false positive (?)

4bfcddf

Not this

1d75e5e

MichaReiser merged commit ba02294 into astral-sh:main Feb 4, 2025
21 checks passed

MichaReiser mentioned this pull request Feb 4, 2025

[flake8-logging] Add ExcInfoOutsideExceptionHandler (LOG014) #14682

Closed

InSyncWithFoo deleted the LOG004 branch February 4, 2025 20:22

BrewTestBot mentioned this pull request Feb 6, 2025

ruff 0.9.5 Homebrew/homebrew-core#206789

Merged

ntBre mentioned this pull request Jun 3, 2025

[flake8-logging] Implement check for logging.exception() outside of exception handler (LOG004) #14245

Closed

Comments

Conversation

InSyncWithFoo commented Jan 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test Plan

Uh oh!

github-actions bot commented Jan 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ruff-ecosystem results

Linter (stable)

Linter (preview)

Uh oh!

InSyncWithFoo commented Jan 29, 2025

Uh oh!

InSyncWithFoo commented Jan 29, 2025

Uh oh!

bluetech commented Jan 29, 2025

Uh oh!

InSyncWithFoo commented Jan 29, 2025

Uh oh!

Uh oh!

MichaReiser left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MichaReiser commented Feb 3, 2025

Uh oh!

InSyncWithFoo commented Feb 3, 2025

Uh oh!

MichaReiser commented Feb 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

InSyncWithFoo commented Feb 3, 2025

Uh oh!

MichaReiser commented Feb 4, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

InSyncWithFoo commented Jan 29, 2025 •

edited

Loading

github-actions bot commented Jan 29, 2025 •

edited

Loading

`ruff-ecosystem` results

MichaReiser commented Feb 3, 2025 •

edited

Loading