Skip to content

[flake8-logging] Allow closures in except handlers for LOG004#24464

Merged
charliermarsh merged 5 commits intoastral-sh:mainfrom
anishgirianish:fix/log004-closure-in-except-handler
Apr 9, 2026
Merged

[flake8-logging] Allow closures in except handlers for LOG004#24464
charliermarsh merged 5 commits intoastral-sh:mainfrom
anishgirianish:fix/log004-closure-in-except-handler

Conversation

@anishgirianish
Copy link
Copy Markdown
Contributor

@anishgirianish anishgirianish commented Apr 7, 2026

Summary

Fixes #18646.

outside_handlers walks AST ancestors looking for a Try statement whose handler range contains the call offset. It used to bail out at FunctionDef nodes to avoid crossing scope boundaries, which meant any logging.exception() inside a closure defined in an except block got flagged.

The range check applies to where the function is defined, not where it's called (which Ruff can't track). This means functions defined inside an except block but called outside of it won't be flagged. This tradeoff fixes the more common false positive at the cost of a less common false negative.

Dropped the FunctionDef break.

Test Plan

  • Updated fixture to move function-inside-except to no-errors section
  • Added closure-called-within-except test case
  • local ecosystem runs showed expected results

@astral-sh-bot astral-sh-bot bot requested a review from ntBre April 7, 2026 07:36
@astral-sh-bot
Copy link
Copy Markdown

astral-sh-bot bot commented Apr 7, 2026

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

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

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

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

- zerver/worker/email_senders_base.py:38:17: LOG004 `.exception()` call outside exception handlers

Changes by rule (1 rules affected)

code total + violation - violation + fix - fix
LOG004 1 0 1 0 0

@MichaReiser MichaReiser assigned dylwil3 and unassigned ntBre Apr 7, 2026
@dylwil3 dylwil3 requested review from dylwil3 and removed request for ntBre April 7, 2026 13:00
@ntBre ntBre added bug Something isn't working rule Implementing or modifying a lint rule preview Related to preview mode features labels Apr 8, 2026
@charliermarsh
Copy link
Copy Markdown
Member

This would now lead to the following false negative:

import logging

def make_later():
    try:
        raise ValueError()
    except Exception:
        def later():
            logging.exception("x")                     # should be LOG004
            logging.error("x", exc_info=True)   # should be LOG014
        return later

make_later()()

...the range check already handles scoping correctly.

The range check only applies to where the function was defined, not where it's called, which we can't determine / track in Ruff. So if we want to fix this false positive, we'll have to introduce some false negatives. I suppose it might be worth it though.

@anishgirianish
Copy link
Copy Markdown
Contributor Author

anishgirianish commented Apr 9, 2026

This would now lead to the following false negative:

import logging

def make_later():
    try:
        raise ValueError()
    except Exception:
        def later():
            logging.exception("x")                     # should be LOG004
            logging.error("x", exc_info=True)   # should be LOG014
        return later

make_later()()

...the range check already handles scoping correctly.

The range check only applies to where the function was defined, not where it's called, which we can't determine / track in Ruff. So if we want to fix this false positive, we'll have to introduce some false negatives. I suppose it might be worth it though.

Thank you so much for the detailed review, @charliermarsh I've updated the PR description to reflect the limitation around definition site vs call site.

Happy to add your example as a documented known false negative in the test fixture. Would you recommend going with this tradeoff, or would you prefer a different approach?

Thank you

@charliermarsh
Copy link
Copy Markdown
Member

I think we should document this limitation, but otherwise I'm comfortable with it. I'll add a note to the docs before merging.

@charliermarsh charliermarsh enabled auto-merge (squash) April 9, 2026 02:35
@charliermarsh charliermarsh changed the title [flake8-logging] Allow closures in except handlers for LOG004 [flake8-logging] Allow closures in except handlers for LOG004 Apr 9, 2026
@charliermarsh charliermarsh disabled auto-merge April 9, 2026 02:35
@charliermarsh charliermarsh enabled auto-merge (squash) April 9, 2026 02:35
@charliermarsh charliermarsh merged commit 60d4694 into astral-sh:main Apr 9, 2026
43 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working preview Related to preview mode features rule Implementing or modifying a lint rule

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[flake8-logging] Handle closures in log-exception-outside-except-handler (LOG004)

4 participants