Skip to content

Python: fix ChangeImport leaving stray leading newline when import is the only statement#7433

Merged
knutwannheden merged 1 commit intomainfrom
fix-changeimport-leading-newline-bug
Apr 20, 2026
Merged

Python: fix ChangeImport leaving stray leading newline when import is the only statement#7433
knutwannheden merged 1 commit intomainfrom
fix-changeimport-leading-newline-bug

Conversation

@knutwannheden
Copy link
Copy Markdown
Contributor

Motivation

rewrite.python.recipes.ChangeImport left a stray leading \n at the start of the file when it replaced an import that was the only statement in the file. The bug is masked whenever the test snippet has a statement after the import (the leading \n ends up between two lines instead of at file start and isn't visible in print_all() output), but surfaces for import-only files.

Discovered while converting search-only recipes in moderneinc/rewrite-migrate-python to use ChangeImport. As a data point, rewrite-migrate-python's typing_deprecations.py _make_pep585_editor explicitly strips a leading \n from the first remaining statement's prefix in visit_compilation_unit as a workaround — this fix should make that workaround unnecessary.

Example

from rewrite.test import RecipeSpec, python
from rewrite.python.recipes import ChangeImport

spec = RecipeSpec(recipe=ChangeImport(
    old_module="typing",
    old_name="Callable",
    new_module="collections.abc",
))
spec.rewrite_run(python(
    "from typing import Callable\n",
    "from collections.abc import Callable\n",  # expected
))
# Before this PR, actual output was: "\nfrom collections.abc import Callable\n"

Summary

  • When ChangeImport replaced the only statement in a file, the scheduled AddImport pass ran against an empty statement list and fell through to the "inserting after existing imports" branch, prefixing the new import with \n.
  • Handle the empty-statement-list case explicitly in add_import.py::_add_import: the new import becomes the sole statement with empty prefix; the trailing newline stays in cu.eof.
  • Add regression test test_change_import_only_statement covering the import-only case.

Test plan

  • New test_change_import_only_statement regression test reproduces the bug and now passes
  • All 19 existing ChangeImport tests pass
  • All 45 import-related tests (test_add_import.py, test_remove_import.py, tests/recipes/) pass
  • Full Python test suite passes (1165 tests, excluding RPC)

…is the only statement

When `ChangeImport` replaced the only statement in a file, the
scheduled `AddImport` pass ran against an empty statement list and fell
through to the "inserting after existing imports" branch, prefixing the
new import with `\n` and producing `\nfrom collections.abc import Callable\n`.

Handle the empty-statement-list case explicitly: the new import becomes
the sole statement with empty prefix; the trailing newline stays in
`cu.eof`.
@github-project-automation github-project-automation Bot moved this to In Progress in OpenRewrite Apr 20, 2026
@knutwannheden knutwannheden merged commit 55a227d into main Apr 20, 2026
1 check passed
@knutwannheden knutwannheden deleted the fix-changeimport-leading-newline-bug branch April 20, 2026 15:58
@github-project-automation github-project-automation Bot moved this from In Progress to Done in OpenRewrite Apr 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

1 participant