diff --git a/CHANGES.rst b/CHANGES.rst index df14239..20a0c48 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -8,6 +8,17 @@ codesorter follows `semantic versioning `_. Unreleased ************ +**Fixed** + +- Do not treat a class's own attribute as a dependency on a same-named outer definition. + An enum member or class variable named like the module constant that aliases it (for + example ``CACHE_MISS = _Sentinel.CACHE_MISS`` beside ``class _Sentinel(Enum): + CACHE_MISS = auto()``) previously forged a false ``class`` -> ``constant`` edge that + closed a cycle with the real ``constant`` -> ``class`` edge, hoisting the constant + above the class it references and raising ``NameError`` at import. A name bound in a + class's own body is now recognized as belonging to that class's namespace and imposes + no ordering on outer definitions. + ******************** 0.2.6 (2026/06/14) ******************** diff --git a/codesorter/sort_code.py b/codesorter/sort_code.py index 5f127b7..068815b 100644 --- a/codesorter/sort_code.py +++ b/codesorter/sort_code.py @@ -336,7 +336,24 @@ def _outer_scope(scope: object) -> bool: # an annotation forging a false cycle with a real value-level edge. if id(found["name"]) in self._lazy_annotation_names: continue - found_name = cst.ensure_type(found["name"], cst.Name).value + name_node = cst.ensure_type(found["name"], cst.Name) + found_name = name_node.value + # A name bound at this class's own body level (an enum member or + # class variable) belongs to the class's namespace, so it must not + # forge a dependency on a same-named outer definition. Otherwise an + # enum member named like the module constant that aliases it (for + # example ``CACHE_MISS = _Sentinel.CACHE_MISS``) creates a false cycle + # that hoists the constant above the class it depends on. The scope + # must be ``node``'s own class scope; a name bound in the *enclosing* + # class (a sibling method an alias assignment references) is a real + # dependency and is kept. + name_scope = self.get_metadata(md.ScopeProvider, name_node, None) + if ( + isinstance(name_scope, md.ClassScope) + and name_scope.node == node + and name_scope.assignments[found_name] + ): + continue is_import = isinstance( next(iter(meta.assignments[found_name])), md.ImportAssignment, diff --git a/tests/test_files/augmented_assignment_input.py b/tests/test_files/augmented_assignment_input.py index edeeb11..e98bced 100644 --- a/tests/test_files/augmented_assignment_input.py +++ b/tests/test_files/augmented_assignment_input.py @@ -1,12 +1,13 @@ """Augmented assignments stay anchored to the constant they augment.""" -from mod import extra, more +import enum +import json ZEBRA = 1 __version__ = "1.0" __all__ = ["Widget"] -__all__ += extra.__all__ -__all__ += more.__all__ +__all__ += json.__all__ +__all__ += enum.__all__ APPLE = 2 diff --git a/tests/test_files/augmented_assignment_output.py b/tests/test_files/augmented_assignment_output.py index 1e578b2..e44161f 100644 --- a/tests/test_files/augmented_assignment_output.py +++ b/tests/test_files/augmented_assignment_output.py @@ -1,12 +1,13 @@ """Augmented assignments stay anchored to the constant they augment.""" -from mod import extra, more +import enum +import json APPLE = 2 ZEBRA = 1 __all__ = ["Widget"] -__all__ += extra.__all__ -__all__ += more.__all__ +__all__ += json.__all__ +__all__ += enum.__all__ __version__ = "1.0" diff --git a/tests/test_files/barrier_input.py b/tests/test_files/barrier_input.py index faee161..c156372 100644 --- a/tests/test_files/barrier_input.py +++ b/tests/test_files/barrier_input.py @@ -1,11 +1,8 @@ """Definitions never cross a side-effecting statement that may depend on them.""" -import sys -from pathlib import Path - ZEBRA = 1 -REPO_ROOT = Path(__file__).resolve().parent -sys.path.insert(0, str(REPO_ROOT)) +MIDDLE = 5 +repr(MIDDLE) # a barrier: a bare statement splits the surrounding definitions BANANA = 3 APPLE = 2 diff --git a/tests/test_files/barrier_output.py b/tests/test_files/barrier_output.py index 0f648b9..9fdeddb 100644 --- a/tests/test_files/barrier_output.py +++ b/tests/test_files/barrier_output.py @@ -1,11 +1,8 @@ """Definitions never cross a side-effecting statement that may depend on them.""" -import sys -from pathlib import Path - -REPO_ROOT = Path(__file__).resolve().parent +MIDDLE = 5 ZEBRA = 1 -sys.path.insert(0, str(REPO_ROOT)) +repr(MIDDLE) # a barrier: a bare statement splits the surrounding definitions APPLE = 2 BANANA = 3 diff --git a/tests/test_files/comprehension_dependency_input.py b/tests/test_files/comprehension_dependency_input.py index c59aa5c..f1e9a1b 100644 --- a/tests/test_files/comprehension_dependency_input.py +++ b/tests/test_files/comprehension_dependency_input.py @@ -1,13 +1,14 @@ """A module-level comprehension references a function eagerly, so it depends on it.""" -placeholders = {name: build(name) for name in ["a", "b"]} + +def lazy(): + """A comprehension here is deferred, so it imposes no ordering.""" + return [build(name) for name in ["c", "d"]] def build(name): - """Used eagerly by the module-level comprehension above.""" + """Used eagerly by the module-level comprehension below.""" return f"value-{name}" -def lazy(): - """A comprehension here is deferred, so it imposes no ordering.""" - return [build(name) for name in ["c", "d"]] +placeholders = {name: build(name) for name in ["a", "b"]} diff --git a/tests/test_files/comprehension_dependency_output.py b/tests/test_files/comprehension_dependency_output.py index 3cf0a4d..1b4f380 100644 --- a/tests/test_files/comprehension_dependency_output.py +++ b/tests/test_files/comprehension_dependency_output.py @@ -1,7 +1,8 @@ """A module-level comprehension references a function eagerly, so it depends on it.""" + def build(name): - """Used eagerly by the module-level comprehension above.""" + """Used eagerly by the module-level comprehension below.""" return f"value-{name}" diff --git a/tests/test_files/comprehensive_input.py b/tests/test_files/comprehensive_input.py index 17a83bd..10c1297 100644 --- a/tests/test_files/comprehensive_input.py +++ b/tests/test_files/comprehensive_input.py @@ -1,3 +1,5 @@ +"""A comprehensive mix of constants, decorators, inheritance, and pytest fixtures.""" + import os from abc import ABC, abstractmethod from functools import wraps @@ -10,7 +12,34 @@ DEBUG_MODE = os.getenv("DEBUG", "false").lower() == "true" -# Functions with decorators +# Custom decorators (defined before the functions that use them) +def cache_result(func): + """Cache the result of a function.""" + cache = {} + + @wraps(func) + def wrapper(*args, **kwargs): + key = str(args) + str(kwargs) + if key not in cache: + cache[key] = func(*args, **kwargs) + return cache[key] + + return wrapper + + +def validate_input(func): + """Validate input parameters.""" + + @wraps(func) + def wrapper(*args, **kwargs): + for arg in args: + if arg is None: + raise ValueError("Input cannot be None") + return func(*args, **kwargs) + + return wrapper + + @cache_result def expensive_calculation(n): """Perform an expensive calculation.""" @@ -23,24 +52,21 @@ def process_data(data): return [item.upper() for item in data] -class Dog(Mammal): - """Dog class.""" +# Base classes with inheritance (each base defined before its subclass) +class Animal(ABC): + """Base class for all animals.""" - def __init__(self, name, age, fur_color, breed): - super().__init__(name, age, fur_color) - self.breed = breed + def __init__(self, name, age): + self.name = name + self.age = age + @abstractmethod def make_sound(self): - """Make a dog sound.""" - return "Woof!" - - def get_breed_info(self): - """Get breed information.""" - return f"Breed: {self.breed}" + """Make a sound.""" - def fetch(self): - """Fetch behavior.""" - return f"{self.name} is fetching" + def get_info(self): + """Get animal information.""" + return f"{self.name} is {self.age} years old" class Mammal(Animal): @@ -59,29 +85,31 @@ def get_fur_info(self): return f"Fur color: {self.fur_color}" -# Base classes with inheritance -class Animal(ABC): - """Base class for all animals.""" +class Dog(Mammal): + """Dog class.""" - def __init__(self, name, age): - self.name = name - self.age = age + def __init__(self, name, age, fur_color, breed): + super().__init__(name, age, fur_color) + self.breed = breed - @abstractmethod def make_sound(self): - """Make a sound.""" + """Make a dog sound.""" + return "Woof!" - def get_info(self): - """Get animal information.""" - return f"{self.name} is {self.age} years old" + def get_breed_info(self): + """Get breed information.""" + return f"Breed: {self.breed}" + + def fetch(self): + """Fetch behavior.""" + return f"{self.name} is fetching" -# Classes with global dependencies class DatabaseManager: """Manages database connections using global config.""" def __init__(self): - self.host = "localhost" # Would normally use DATABASE_URL + self.host = "localhost" self.port = 5432 self.database = "test_db" @@ -109,40 +137,11 @@ def is_debug_mode(self): return DEBUG_MODE -def validate_input(func): - """Validate input parameters.""" - - @wraps(func) - def wrapper(*args, **kwargs): - for arg in args: - if arg is None: - raise ValueError("Input cannot be None") - return func(*args, **kwargs) - - return wrapper - - def regular_function(): """A regular function without decorators.""" return "regular" -# Custom decorators -def cache_result(func): - """Cache the result of a function.""" - cache = {} - - @wraps(func) - def wrapper(*args, **kwargs): - key = str(args) + str(kwargs) - if key not in cache: - cache[key] = func(*args, **kwargs) - return cache[key] - - return wrapper - - -# Test functions def test_something(database_connection, sample_data): """Test function using fixtures.""" assert database_connection["connected"] @@ -171,7 +170,6 @@ def sample_data(): return ["item1", "item2", "item3"] -# Pytest fixtures @pytest.fixture(scope="session") def database_connection(): """Provide a database connection for testing.""" diff --git a/tests/test_files/comprehensive_output.py b/tests/test_files/comprehensive_output.py index 007d471..26e5ae0 100644 --- a/tests/test_files/comprehensive_output.py +++ b/tests/test_files/comprehensive_output.py @@ -1,10 +1,12 @@ +"""A comprehensive mix of constants, decorators, inheritance, and pytest fixtures.""" + import os from abc import ABC, abstractmethod from functools import wraps import pytest -API_KEY = "test-key-123" +API_KEY = "test-key-123" # Global variables DATABASE_URL = "sqlite:///test.db" DEBUG_MODE = os.getenv("DEBUG", "false").lower() == "true" @@ -25,7 +27,7 @@ def make_request(self, endpoint): return f"GET {self.base_url}{endpoint}" -# Base classes with inheritance +# Base classes with inheritance (each base defined before its subclass) class Animal(ABC): """Base class for all animals.""" @@ -42,12 +44,11 @@ def get_info(self): return f"{self.name} is {self.age} years old" -# Classes with global dependencies class DatabaseManager: """Manages database connections using global config.""" def __init__(self): - self.host = "localhost" # Would normally use DATABASE_URL + self.host = "localhost" self.port = 5432 self.database = "test_db" @@ -105,7 +106,6 @@ def setup_test_environment(): API_KEY = "test-key-123" -# Pytest fixtures @pytest.fixture(scope="session") def database_connection(): """Provide a database connection for testing.""" @@ -118,7 +118,7 @@ def sample_data(): return ["item1", "item2", "item3"] -# Custom decorators +# Custom decorators (defined before the functions that use them) def cache_result(func): """Cache the result of a function.""" cache = {} @@ -133,7 +133,6 @@ def wrapper(*args, **kwargs): return wrapper -# Functions with decorators @cache_result def expensive_calculation(n): """Perform an expensive calculation.""" @@ -161,7 +160,6 @@ def test_global_dependencies(): assert api_client.is_debug_mode() == DEBUG_MODE -# Test functions def test_something(database_connection, sample_data): """Test function using fixtures.""" assert database_connection["connected"] diff --git a/tests/test_files/enum_member_alias_input.py b/tests/test_files/enum_member_alias_input.py new file mode 100644 index 0000000..5778afc --- /dev/null +++ b/tests/test_files/enum_member_alias_input.py @@ -0,0 +1,12 @@ +from enum import Enum, auto + + +def helper(): + return CACHE_MISS + + +class _Sentinel(Enum): + CACHE_MISS = auto() + + +CACHE_MISS = _Sentinel.CACHE_MISS diff --git a/tests/test_files/enum_member_alias_output.py b/tests/test_files/enum_member_alias_output.py new file mode 100644 index 0000000..face945 --- /dev/null +++ b/tests/test_files/enum_member_alias_output.py @@ -0,0 +1,12 @@ +from enum import Enum, auto + + +class _Sentinel(Enum): + CACHE_MISS = auto() + + +CACHE_MISS = _Sentinel.CACHE_MISS + + +def helper(): + return CACHE_MISS diff --git a/tests/test_files/lazy_annotation_cycle_input.py b/tests/test_files/lazy_annotation_cycle_input.py index 288ca60..2fd9684 100644 --- a/tests/test_files/lazy_annotation_cycle_input.py +++ b/tests/test_files/lazy_annotation_cycle_input.py @@ -3,12 +3,6 @@ from __future__ import annotations -class Apple(_Base): - """References the Instr alias only in a lazy annotation.""" - - nxt: list[Instr] - - class _Base: """A shared base class.""" @@ -17,10 +11,16 @@ class Zebra(_Base): """Another member of the Instr union.""" -Instr = Apple | Zebra +class Apple(_Base): + """References the Instr alias only in a lazy annotation.""" + + nxt: list[Instr] class User: """Uses the Instr alias in a lazy annotation.""" items: list[Instr] + + +Instr = Apple | Zebra diff --git a/tests/test_files/name_rebinding_input.py b/tests/test_files/name_rebinding_input.py index fd0fbf0..25d9bb3 100644 --- a/tests/test_files/name_rebinding_input.py +++ b/tests/test_files/name_rebinding_input.py @@ -1,6 +1,11 @@ """An assignment that rebinds a method name stays after the method it wraps.""" +def cachedproperty(method, *, doc=None): + """Minimal stand-in so the rebinding has something to call.""" + return property(method) + + class Klass: def ten(self): return 10 diff --git a/tests/test_files/name_rebinding_output.py b/tests/test_files/name_rebinding_output.py index 68e63cc..132476c 100644 --- a/tests/test_files/name_rebinding_output.py +++ b/tests/test_files/name_rebinding_output.py @@ -1,6 +1,11 @@ """An assignment that rebinds a method name stays after the method it wraps.""" +def cachedproperty(method, *, doc=None): + """Minimal stand-in so the rebinding has something to call.""" + return property(method) + + class Klass: def alpha(self): """A method that sorts first alphabetically.""" diff --git a/tests/test_sort_code.py b/tests/test_sort_code.py index b5e8569..63b4b17 100644 --- a/tests/test_sort_code.py +++ b/tests/test_sort_code.py @@ -1,5 +1,9 @@ """Tests for the SortCodeCommand codemod.""" +import io +from contextlib import redirect_stderr, redirect_stdout +from pathlib import Path + import libcst as cst import pytest from libcst.codemod import CodemodContext @@ -183,6 +187,52 @@ def test_empty_module(self): result = command.transform_module(cst.parse_module(code)) assert result.code.strip() == "" + def test_enum_member_alias(self, test_files): + """Test that a class attribute sharing a name with an outer constant is not a dep. + + An enum member (or class variable) named like the module constant that aliases + it (``CACHE_MISS = _Sentinel.CACHE_MISS``) is bound in the class's own + namespace, so it must not forge a dependency from the class back onto the + constant. Without that guard the false ``_Sentinel`` -> ``CACHE_MISS`` edge + closes a cycle with the real ``CACHE_MISS`` -> ``_Sentinel`` edge, and + cycle-breaking hoists the constant above the class it references (raising + NameError at import). + + """ + input_code, expected_code = test_files + context = CodemodContext() + command = SortCodeCommand(context) + result = command.transform_module(cst.parse_module(input_code)) + + assert expected_code == result.code + + def test_fixture_files_execute(self): + """Every input/output fixture must import cleanly with no side effects. + + Executing each file in a fresh namespace is a stronger guard than parsing: it + catches a fixture that parses but does not run (an invalid input, or a codemod + output that raises ``NameError`` because a definition was hoisted above one it + depends on). Each file is also required to be self-contained and side-effect + free, so anything written to stdout/stderr fails the test. + + """ + test_files_dir = Path(__file__).parent / "test_files" + fixtures = sorted(test_files_dir.glob("*.py")) + assert fixtures, "no fixture files found" + failures = [] + for path in fixtures: + captured = io.StringIO() + try: + code = compile(path.read_text(encoding="utf-8"), str(path), "exec") + with redirect_stdout(captured), redirect_stderr(captured): + exec(code, {"__name__": "__fixture__"}) # noqa: S102 + except Exception as error: # noqa: BLE001 + failures.append(f"{path.name}: {type(error).__name__}: {error}") + continue + if captured.getvalue(): + failures.append(f"{path.name}: wrote to stdout/stderr {captured.getvalue()!r}") + assert not failures, "fixtures that do not execute cleanly:\n" + "\n".join(failures) + def test_keyword_arguments(self, test_files): """Test that keyword arguments, keyword-only params, and dict keys are sorted.""" input_code, expected_code = test_files