test tidy-import on cpython, and fix a few issues. (#476)

Test tidy-import on cpython, and fix a few issues.

This shallow-clone cpython souce and run tidy import on all those files.

For speed, this ads the NOTHING action that does not actually change the file but print a message if there is an issue on a file.

This also adds more extended error handling, and fix some edge case like removing an import statement that would make a function empty.

Author: Carreau

.github/workflows/test.yml

@@ -92,3 +92,39 @@ jobs:
     - uses: codecov/codecov-action@v5
       with:
         token: ${{ secrets.CODECOV_TOKEN }}
+
+  cpython-smoke:
+    name: tidy-imports smoke test on CPython
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v6
+
+      - name: Install uv
+        uses: astral-sh/setup-uv@v7
+        with:
+          enable-cache: true
+          cache-dependency-glob: "pyproject.toml"
+
+      - name: Install pyflyby
+        run: |
+          uv python install 3.14
+          uv venv
+          uv pip install meson-python meson ninja "pybind11>=2.10.4" "setuptools-scm>=8"
+          uv pip install --no-build-isolation -ve .
+
+      - name: Shallow-clone CPython
+        run: git clone --depth=1 --quiet https://github.com/python/cpython.git /tmp/cpython
+
+      - name: Run tidy-imports on all CPython .py files
+        run: |
+          # Files known to cause issues with tidy-imports (e.g. encoding quirks,
+          # intentionally broken syntax used in tests, etc.)
+          rm /tmp/cpython/Lib/test/encoded_modules/module_iso_8859_1.py
+          rm /tmp/cpython/Lib/test/encoded_modules/module_koi8_r.py
+          rm /tmp/cpython/Lib/test/test_import/data/syntax_warnings.py
+          rm /tmp/cpython/Lib/test/test_tarfile.py
+          rm /tmp/cpython/Lib/test/tokenizedata/bad_coding2.py
+          rm /tmp/cpython/Lib/test/tokenizedata/badsyntax_3131.py
+          rm /tmp/cpython/Lib/test/tokenizedata/badsyntax_pep3120.py
+
+          uv run python bin/tidy-imports --no-add --no-remove-unused --action=NOTHING /tmp/cpython >/dev/null

lib/python/pyflyby/_autoimp.py

@@ -711,6 +711,19 @@ def _visit__all__(self, node):
             return
         if (len(node.targets) == 1 and isinstance(node.targets[0], ast.Name)
             and node.targets[0].id == '__all__'):
+            _DYNAMIC_ALL_TYPES = (
+                ast.ListComp,
+                ast.SetComp,
+                ast.BinOp,
+                ast.Call,
+                ast.IfExp,
+            )
+            if isinstance(node.value, _DYNAMIC_ALL_TYPES):
+                logger.info(
+                    "Can't handle dynamic __all__ (%s); skipping",
+                    type(node.value).__name__,
+                )
+                return
             if not isinstance(node.value, (ast.List, ast.Tuple)):
                 logger.warning("Don't know how to handle __all__ as (%s)" % node.value)
                 return

lib/python/pyflyby/_cmdline.py

@@ -104,11 +104,13 @@ def parse_action(v):
                 return action_exit1
             elif V == "CHANGEDEXIT1":
                 return action_changedexit1
+            elif V == "NOTHING":
+                return action_nothing
             else:
                 raise Exception(
                     "Bad argument %r to --action; "
-                    "expected PRINT or REPLACE or QUERY or IFCHANGED or EXIT1 "
-                    "or CHANGEDEXIT1 or EXECUTE:..." % (v,)
+                    "expected PRINT,REPLACE,QUERY,IFCHANGED,EXIT1,"
+                    "CHANGEDEXIT1,EXECUTE or NOTHING:..." % (v,)
                 )
 
         def set_actions(actions):
@@ -127,7 +129,7 @@ def callback(option, opt_str, value, parser):
         group.add_option(
             "--actions", type='string', action='callback',
             callback=action_callback,
-            metavar="PRINT|REPLACE|IFCHANGED|QUERY|DIFF|EXIT1|CHANGEDEXIT1:EXECUTE:mycommand",
+            metavar="PRINT|REPLACE|IFCHANGED|QUERY|DIFF|EXIT1|CHANGEDEXIT1|NOTHING|EXECUTE:mycommand",
             help=hfmt(
                 """
                    Comma-separated list of action(s) to take.  If PRINT, print
@@ -138,7 +140,8 @@ def callback(option, opt_str, value, parser):
                    If IFCHANGED, then continue actions only if file was
                    changed.  If EXIT1, then exit with exit code 1 after all
                    files/actions are processed.  If CHANGEDEXIT1, then exit
-                   with exit code 1 if any file was changed."""
+                   with exit code 1 if any file was changed. NOTHING can be used
+                   to do nothing and test tidy-import does not crash"""
             ),
         )
         group.add_option(
@@ -465,6 +468,14 @@ def on_error_filename_arg(arg):
         raise SystemExit(exit_code)
 
 
+def action_nothing(m):
+    try:
+        # side effect
+        m.output_content
+    except Exception:
+        logger.error("Error with file %r", m.filename)
+
+
 def action_print(m):
     output_content = m.output_content
     sys.stdout.write(output_content.joined)

lib/python/pyflyby/_file.py

@@ -682,8 +682,14 @@ def read_file(filename: Filename) -> FileText:
     if filename == Filename.STDIN:
         data = sys.stdin.read()
     else:
-        with io.open(str(filename), 'r') as f:
-            data = f.read()
+        try:
+            with io.open(str(filename), 'r') as f:
+                data = f.read()
+        except UnicodeDecodeError as e:
+            raise UnicodeDecodeError(
+                e.encoding, e.object, e.start, e.end,
+                f"{e.reason} (while reading {filename})"
+            ) from None
     return FileText(data, filename=filename)
 
 

lib/python/pyflyby/_imports2s.py

@@ -12,7 +12,8 @@
 from   pyflyby._importclns      import ImportSet, NoSuchImportError
 from   pyflyby._importdb        import ImportDB
 from   pyflyby._importstmt      import (Import, ImportFormatParams,
-                                        ImportStatement)
+                                        ImportStatement,
+                                        NonImportStatementError)
 from   pyflyby._log             import logger
 from   pyflyby._parse           import PythonBlock, PythonStatement
 from   pyflyby._util            import ImportPathCtx, Inf, NullCtx, memoize
@@ -28,7 +29,9 @@
 _IMPORT_TYPES = (ast.Import, ast.ImportFrom)
 
 
-def _group_consecutive_imports(body: list) -> list[list]:
+def _group_consecutive_imports(
+    body: list[ast.stmt],
+) -> list[list[Union[ast.Import, ast.ImportFrom]]]:
     """Group consecutive import statements from an AST body.
 
     Parameters
@@ -194,6 +197,48 @@ class NoImportBlockError(Exception):
 class ImportAlreadyExistsError(Exception):
     pass
 
+
+def _maybe_insert_pass(
+    lines: list[str], idx: int, indent: int, lineno: int
+) -> None:
+    """Insert a ``pass`` statement at *idx* when removing a line would leave a
+    block-opener (a line ending with ``:``) without a body.
+
+    After a line has been deleted at position *idx*, this function walks
+    backwards to find the nearest non-empty preceding line.  If that line ends
+    with ``:`` (a compound-statement header such as ``def``, ``class``,
+    ``if``, etc.) and the next non-empty line after *idx* is at an equal or
+    lower indentation level, the block body is gone and a ``pass`` statement
+    is inserted at *idx* using *indent* spaces.
+
+    :param lines: Source lines of the block, already modified (the import line
+        has been deleted before this call).
+    :param idx: Index into *lines* where the deleted line used to be.
+    :param indent: Column offset (number of leading spaces) of the deleted line,
+        used to indent the inserted ``pass``.
+    :param lineno: Absolute line number in the file (used only for logging).
+    """
+    prev_idx = idx - 1
+    while prev_idx >= 0 and lines[prev_idx].strip() == "":
+        prev_idx -= 1
+    if prev_idx < 0:
+        return
+    prev_line = lines[prev_idx]
+    if not prev_line.rstrip().endswith(":"):
+        return
+    prev_indent = len(prev_line) - len(prev_line.lstrip())
+    next_idx = idx
+    while next_idx < len(lines) and lines[next_idx].strip() == "":
+        next_idx += 1
+    body_gone = (
+        next_idx >= len(lines)
+        or (len(lines[next_idx]) - len(lines[next_idx].lstrip())) <= prev_indent
+    )
+    if body_gone:
+        lines.insert(idx, " " * indent + "pass")
+        logger.debug("Inserted 'pass' at line %d to preserve empty block", lineno)
+
+
 class SourceToSourceFileImportsTransformation(SourceToSourceTransformationBase):
     blocks: list[Union[SourceToSourceImportBlockTransformation, SourceToSourceTransformation]]
     import_blocks: list[Union[SourceToSourceImportBlockTransformation, _LocalImportBlockWrapper]]
@@ -231,7 +276,11 @@ def preprocess(self) -> None:
         logger.debug("preprocess: extracted %d total import blocks", len(self.import_blocks))
 
     def _create_import_block_from_group(
-        self, group: list, lines: list, start_line: int, end_line: int
+        self,
+        group: list[Union[ast.Import, ast.ImportFrom]],
+        lines: list[str],
+        start_line: int,
+        end_line: int,
     ) -> None:
         """Create an import block from a group of import statements.
 
@@ -240,34 +289,49 @@ def _create_import_block_from_group(
 
         Parameters
         ----------
-        group : list
-            List of consecutive import AST nodes
-        lines : list
-            Lines of the full source text
+        group : list[ast.Import | ast.ImportFrom]
+            Consecutive import AST nodes to extract.
+        lines : list[str]
+            All source lines of the file (1-indexed via ``lines[lineno - 1]``).
         start_line : int
-            Starting line number of the group
+            First line number of the group (1-indexed).
         end_line : int
-            Ending line number of the group
+            Last line number of the group, accounting for multiline imports.
         """
         import_text_parts = []
         semicolon_suffixes = {}
 
         for node in group:
             lineno = node.lineno
-            line: str = lines[lineno - 1]
+            end_lineno = getattr(node, "end_lineno", lineno)
 
             if hasattr(node, "col_offset") and hasattr(node, "end_col_offset"):
-                # WARNING offsets seem to be in number of bytes!
-                import_stmt = line.encode()[
-                    node.col_offset : node.end_col_offset
-                ].decode()
-                remaining = line.encode()[node.end_col_offset :].decode().lstrip()
-                if remaining and remaining.startswith(";"):
+                # WARNING: col_offset/end_col_offset are in bytes, not characters.
+                if end_lineno > lineno:
+                    # Multiline import (e.g. "from foo import (\n    a,\n    b\n)").
+                    # end_col_offset refers to the last line, not the first, so we
+                    # must collect all source lines and trim each end independently.
+                    first = lines[lineno - 1].encode()
+                    last = lines[end_lineno - 1].encode()
+                    node_lines = [first[node.col_offset :].decode()]
+                    for mid in range(lineno + 1, end_lineno):
+                        node_lines.append(lines[mid - 1])
+                    node_lines.append(last[: node.end_col_offset].decode())
+                    import_stmt = "\n".join(node_lines)
+                    remaining = last[node.end_col_offset :].decode().lstrip()
+                else:
+                    line: str = lines[lineno - 1]
+                    import_stmt = line.encode()[
+                        node.col_offset : node.end_col_offset
+                    ].decode()
+                    remaining = line.encode()[node.end_col_offset :].decode().lstrip()
+
+                if remaining.startswith(";"):
                     suffix = remaining[1:].lstrip()
                     if suffix:
                         semicolon_suffixes[lineno] = suffix
             else:
-                import_stmt = line
+                import_stmt = lines[lineno - 1]
 
             import_text_parts.append(import_stmt)
 
@@ -276,7 +340,7 @@ def _create_import_block_from_group(
             try:
                 import_block = PythonBlock(import_text)
                 trans = SourceToSourceImportBlockTransformation(import_block)
-            except (SyntaxError, ValueError) as e:
+            except (SyntaxError, ValueError, NonImportStatementError) as e:
                 logger.debug(
                     "Failed to create import block for lines %d-%d: %s",
                     start_line,
@@ -329,7 +393,7 @@ def _extract_imports_from_statement(
 
         for group in import_groups:
             start_line = group[0].lineno
-            end_line = group[-1].lineno
+            end_line = getattr(group[-1], "end_lineno", group[-1].lineno)
             self._create_import_block_from_group(group, lines, start_line, end_line)
 
         # Recursively check nested function/class definitions
@@ -589,6 +653,7 @@ def remove_import(self, imp, lineno):
         return imp
 
     def _remove_local_import_from_blocks(self, imp, lineno):
+
         """
         Remove a local import from the actual code blocks.
         This modifies the _output of blocks in self.blocks to remove the import line.
@@ -627,8 +692,9 @@ def _remove_local_import_from_blocks(self, imp, lineno):
                         offset,
                         line_to_remove.strip(),
                     )
-                    # Remove this line
+                    removed_indent = len(line_to_remove) - len(line_to_remove.lstrip())
                     del lines[relative_lineno]
+                    _maybe_insert_pass(lines, relative_lineno, removed_indent, lineno)
 
                     # Track that we removed a line from this block
                     self._removed_lines_per_block[block_idx] = offset + 1

tests/test_autoimp.py

@@ -1894,7 +1894,6 @@ def defaultdict():
     assert unused == []
 
 
-
 def test_load_symbol_1():
     assert load_symbol("os.path.join", {"os": os}) is os.path.join