Skip to content

Conversation

jimhoekstra
Copy link
Contributor

@jimhoekstra jimhoekstra commented Jul 23, 2025

Issue: #19498

Summary

missing-required-import inserts the missing import on the line immediately following the last line of the docstring. However, if the dosctring is immediately followed by a continuation token (i.e. backslash) then this leads to a syntax error because Python interprets the docstring and the inserted import to be on the same line.

The proposed solution in this PR is to check if the first token after a file docstring is a continuation character, and if so, to advance an additional line before inserting the missing import.

Test Plan

Added a unit test, and the following example was verified manually:

Given this simple test Python file:

"Hello, World!"\

print(__doc__)

and this ruff linting configuration in the pyproject.toml file:

[tool.ruff.lint]
select = ["I"]

[tool.ruff.lint.isort]
required-imports = ["import sys"]

Without the changes in this PR, the ruff linter would try to insert the missing import in line 2, resulting in a syntax error, and report the following:

error: Fix introduced a syntax error. Reverting all changes.

With the changes in this PR, ruff correctly advances one more line before adding the missing import, resulting in the following output:

"Hello, World!"\

import sys

print(__doc__)

Jim Hoekstra added 2 commits July 23, 2025 11:54
@jimhoekstra jimhoekstra marked this pull request as ready for review July 23, 2025 11:36
@ntBre ntBre self-requested a review July 24, 2025 13:07
@ntBre ntBre added bug Something isn't working fixes Related to suggested fixes for violations labels Jul 25, 2025
@ntBre ntBre linked an issue Jul 25, 2025 that may be closed by this pull request
Copy link
Contributor

@ntBre ntBre left a comment

Choose a reason for hiding this comment

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

This is great, thank you!

Would you mind adding an I002 test too? That would also have the benefit of showing a nice snapshot instead of comparing to a TextSize directly. I think 22 is correct from counting manually, but the snapshot will be an easy way to double check :)

I think you could probably add another test_case on this function or one of the others nearby:

#[test_case(Path::new("whitespace.py"))]
fn required_import(path: &Path) -> Result<()> {
let snapshot = format!("required_import_{}", path.to_string_lossy());

Copy link
Contributor

github-actions bot commented Jul 28, 2025

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@ntBre
Copy link
Contributor

ntBre commented Jul 28, 2025

Don't worry about the ecosystem check yet, I think it's relative to an old commit since I only just approved the workflow. I expect it to clear up after a new commit.

@jimhoekstra
Copy link
Contributor Author

Thanks for your reply! I've added a snapshot test in my latest commit, how does it look?

@@ -794,6 +794,7 @@ mod tests {
#[test_case(Path::new("comments_and_newlines.py"))]
#[test_case(Path::new("docstring.py"))]
#[test_case(Path::new("docstring.pyi"))]
#[test_case(Path::new("docstring_followed_by_continuation.py"))]
#[test_case(Path::new("docstring_only.py"))]
#[test_case(Path::new("docstring_with_continuation.py"))]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ntBre Do you think it would be a good idea to rename this existing test case (docstring_with_continuation.py) to something like docstring_with_semicolon_and_continuation.py to avoid ambiguity with the test case that I just added (i.e. docstring_followed_by_continuation.py)?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's fine, but good catch!

Copy link
Contributor

@ntBre ntBre left a comment

Choose a reason for hiding this comment

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

The new snapshots look great, thank you! This is good to merge once CI passes.

@@ -794,6 +794,7 @@ mod tests {
#[test_case(Path::new("comments_and_newlines.py"))]
#[test_case(Path::new("docstring.py"))]
#[test_case(Path::new("docstring.pyi"))]
#[test_case(Path::new("docstring_followed_by_continuation.py"))]
#[test_case(Path::new("docstring_only.py"))]
#[test_case(Path::new("docstring_with_continuation.py"))]
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's fine, but good catch!

@ntBre ntBre merged commit 38049aa into astral-sh:main Jul 30, 2025
35 checks passed
@ntBre ntBre changed the title fix missing-required-imports introducing syntax error after dosctring ending with backslash [isort] Fix syntax error after docstring ending with backslash (I002) Jul 30, 2025
@jimhoekstra jimhoekstra deleted the bug/insert-required-imports-after-continuation branch July 31, 2025 07:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working fixes Related to suggested fixes for violations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

start_of_file ignores a backslash, making I002 introduce a syntax error
2 participants