Skip to content

[pyflakes] Mark the fix as unsafe when it creates a docstring (F541) - #28258

Merged
ntBre merged 6 commits into
astral-sh:mainfrom
baltasarblanco:feat/18807-f541-docstring-unsafe
Oct 2, 2026
Merged

ntBre merged 6 commits into
astral-sh:mainfrom
baltasarblanco:feat/18807-f541-docstring-unsafe

Conversation

@baltasarblanco

Copy link
Copy Markdown
Contributor

Summary

The fix is now unsafe in docstring positions. The reason is that removing the f turns the f-string into a string literal, and this changes what the program exposes as documentation.

It does not touch the checker or any other rule. The scope is wider than what was reported in the issue. Listing the positions to make the scope clear:

  • the first statement of a module body
  • the first statement of a class body
  • the first statement of a function body
  • after a simple assignment (a = 1) at module level or in a class body
  • after an annotated assignment (b: int = 2), likewise

I say documentation and not runtime because attribute docstrings don't touch __doc__.

c = d = 3 stays safe because this mirrors the checker's own criterion.

Fixes #18807

Test Plan

2828 passed, 0 failed. Tested with:

  • new snapshot with 11 diagnostics: 6 with the unsafe note, 5 without it
  • --fix without the flag on the unsafe case and the file didn't change; with --unsafe-fixes it did. The safe case was fixed without the flag
  • the F541.py snapshot didn't move
  • cargo clippy --workspace --all-targets --all-features -- -D warnings clean
  • prek run --all-files: 14 hooks

The fix is now unsafe in docstring positions. The reason is that removing the `f` turns the f-string into a string literal, and this changes what the program exposes as documentation.

It does not touch the checker or any other rule. The scope is wider than what was reported in the issue. Listing the positions to make the scope clear:

- the first statement of a module body
- the first statement of a class body
- the first statement of a function body
- after a simple assignment (`a = 1`) at module level or in a class body
- after an annotated assignment (`b: int = 2`), likewise
@astral-sh-bot
astral-sh-bot Bot requested a review from ntBre September 2, 2026 19:31
@astral-sh-bot

astral-sh-bot Bot commented Sep 2, 2026

Copy link
Copy Markdown

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Comment thread crates/ruff_linter/src/rules/pyflakes/rules/f_string_missing_placeholders.rs Outdated
Comment thread crates/ruff_linter/src/rules/pyflakes/rules/f_string_missing_placeholders.rs Outdated
Comment thread crates/ruff_linter/src/rules/pyflakes/rules/f_string_missing_placeholders.rs Outdated
Comment thread crates/ruff_linter/src/rules/pyflakes/rules/f_string_missing_placeholders.rs Outdated
Comment thread crates/ruff_linter/resources/test/fixtures/pyflakes/F541_docstring.py Outdated
@ntBre ntBre added bug An issue describing something that isn't working, or a PR that fixes a bug fixes Related to suggested fixes for violations labels Sep 3, 2026
…d ISC003

Mainly the method `Checker::in_docstring_position(range)` in `checkers/ast/mod.rs`, covering the five positions. It's also worth knowing that F541 and ISC003 both use it, so their two helpers go away. ISC003 inherits attribute docstrings. Migration of the F541 tests from `.py` fixture + snapshots to mdtest, with a new case (previous statement that is not an assignment).

2828 tests, 38 mdtests, clippy clean.
@baltasarblanco

Copy link
Copy Markdown
Contributor Author

Changes from the first push:

The helper was extracted to Checker::in_docstring_position in checkers/ast/mod.rs. The helpers F541 and ISC003 were using go away, since they now use this one. Neither rule uses &Expr because it only needs a TextRange.

convert_f_string_to_regular_string returns an Edit because Fix::applicable_edit now happens there, next to where the applicability is computed.

I applied the suggestions @ntBre asked for (## Fix safety is now a single line).

Most importantly: ISC003 inherited attribute docstrings. Its fixture had an x = 1 followed by a concatenation, commented as "Not a docstring position: not the first statement in the module body", and it's unsafe now. I fixed that comment and its snapshot changes on that particular line.

I also think it's worth mentioning that F541_docstring.py and its snapshot were replaced by resources/mdtest/pyflakes/f-string-missing-placeholders.md, with a new case (previous statement that is not an assignment).

Tests came out at 2828, 38 mdtests, clippy clean.

@MichaReiser MichaReiser added the bot:ai-policy-comment Trigger a bot comment explaining the org AI policy label Sep 7, 2026
@astral-sh-bot

astral-sh-bot Bot commented Sep 7, 2026

Copy link
Copy Markdown

We support using AI (i.e., LLMs) while coding. However, AI should not be used to communicate with maintainers.

Please read and follow our AI policy when submitting issues and pull requests.

@baltasarblanco

Copy link
Copy Markdown
Contributor Author

I’ll write shorter and more direct from now on.

@baltasarblanco

Copy link
Copy Markdown
Contributor Author

Hi @ntBre, the changes from the last review are done and the branch is up to date too!

@ntBre ntBre removed the bot:ai-policy-comment Trigger a bot comment explaining the org AI policy label Sep 30, 2026

@ntBre ntBre left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Codex noticed a small issue with weird code like:

class C:
    if condition:
        x = 1
        f"Attribute documentation"

So I pushed a commit addressing it. This otherwise looked good to me.

@ntBre
ntBre enabled auto-merge (squash) October 2, 2026 18:58
@ntBre
ntBre merged commit 3b1c928 into astral-sh:main Oct 2, 2026
51 checks passed
@baltasarblanco

Copy link
Copy Markdown
Contributor Author

Thanks! Codex noticed a small issue with weird code like:

class C:
    if condition:
        x = 1
        f"Attribute documentation"

So I pushed a commit addressing it. This otherwise looked good to me.

Thanks for the review. Im very focused on improving my code for this project!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug An issue describing something that isn't working, or a PR that fixes a bug fixes Related to suggested fixes for violations

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Pyflakes] F541 fix changes behavior if the string becomes a docstring

3 participants