diff --git a/README.md b/README.md index 48fab90..b0b9682 100644 --- a/README.md +++ b/README.md @@ -199,7 +199,7 @@ for x in changer.iterate_coordinates(): print(x) print(changer.apply_coordinate(x)) -#> Coordinate(file=None, class_name='SimpleString', start_line=1, start_column=4, end_line=1, end_column=9, converter_id='__main__:change_string:13') +#> Coordinate(file=None, class_name='SimpleString', start_line=1, start_column=4, end_line=1, end_column=16, converter_id='__main__:change_string:10') #> a = "new string" ``` diff --git a/cstvis/changer.py b/cstvis/changer.py index 5ba1538..a6e5c6a 100644 --- a/cstvis/changer.py +++ b/cstvis/changer.py @@ -76,5 +76,5 @@ def iterate_coordinates(self) -> Generator[Coordinate, None, None]: def apply_coordinate(self, coordinate: Coordinate) -> str: wrapper = metadata.MetadataWrapper(self.module) node_ranges = wrapper.resolve(metadata.WhitespaceInclusivePositionProvider) - modified = wrapper.visit(SuperTransformer(coordinate, self.converters_by_types, self._comments_by_lines, set(), SourceOffsetResolver(wrapper.module, self.source, node_ranges.values()))) + modified = wrapper.visit(SuperTransformer(coordinate, self.converters_by_types, self._comments_by_lines, SourceOffsetResolver(wrapper.module, self.source, node_ranges.values()))) return modified.code diff --git a/cstvis/transformers/super_transformer.py b/cstvis/transformers/super_transformer.py index bc3d715..13d1997 100644 --- a/cstvis/transformers/super_transformer.py +++ b/cstvis/transformers/super_transformer.py @@ -1,40 +1,13 @@ -from typing import Any, Callable, Dict, List, Set, Type +from typing import Dict, List, Type -import libcst.matchers as matchers_module -from libcst import CSTNode, metadata -from libcst.matchers import ( - BaseMatcherNode, - MatcherDecoratableTransformer, - TypeOf, - leave, -) +from libcst import CSTNode, CSTTransformer, metadata from cstvis.dto import Context, Coordinate, SourcePosition from cstvis.source_offsets import SourceOffsetResolver from cstvis.wrapper import CallableWrapper -def get_all_matcher_nodes() -> List[BaseMatcherNode]: - result = [] - - for name in dir(matchers_module): - attribute = getattr(matchers_module, name) - try: - if issubclass(attribute, BaseMatcherNode) and attribute is not BaseMatcherNode and attribute is not TypeOf: - result.append(attribute()) - except TypeError: - pass - - return result - -def leave_all(function: Callable[[Any, CSTNode, CSTNode], CSTNode]) -> Callable[[Any, CSTNode, CSTNode], CSTNode]: - for matcher in get_all_matcher_nodes(): - function = leave(matcher)(function) - - return function - - -class SuperTransformer(MatcherDecoratableTransformer): +class SuperTransformer(CSTTransformer): """ Apply one conversion with positions from the original node. @@ -50,23 +23,17 @@ def __init__( target_coordinate: Coordinate, nodes_mapping: Dict[Type[CSTNode], List[CallableWrapper[CSTNode]]], comments: Dict[int, str], - nodes_ids: Set[int], source_offsets: SourceOffsetResolver, ): self.target_coordinate = target_coordinate self.nodes_mapping = nodes_mapping self.comments = comments - self.nodes_ids = nodes_ids self.source_offsets = source_offsets super().__init__() - @leave_all - def leave(self, original_node, updated_node): # type: ignore[no-untyped-def] - if id(original_node) in self.nodes_ids: - return updated_node - self.nodes_ids.add(id(original_node)) - + # LibCST's generic signature cannot express supported cross-type replacements. + def on_leave(self, original_node: CSTNode, updated_node: CSTNode) -> CSTNode: # type: ignore[override] converters = self.nodes_mapping.get(type(original_node), []) + self.nodes_mapping.get(CSTNode, []) # type: ignore[type-abstract] if not converters: return updated_node diff --git a/docs/plans/2.md b/docs/plans/2.md new file mode 100644 index 0000000..20f1b0d --- /dev/null +++ b/docs/plans/2.md @@ -0,0 +1,49 @@ +# Переход на `CSTTransformer.on_leave()` по TDD + +В проект поступило [issue #10 — Replace matcher-wide dispatch with `CSTTransformer.on_leave()`](https://github.com/mutating/cstvis/issues/10), в котором предложено упростить и оптимизировать `SuperTransformer`: отказаться от регистрации одного callback на всех matcher-классах и последующей дедупликации вызовов в пользу штатного `CSTTransformer.on_leave()`. + +Работа не подразумевает изменений публичного API или документированного поведения cstvis и является чистым внутренним рефакторингом. Для защиты от регрессий работа начинается с усиления тестового покрытия: необходимые тесты должны быть добавлены и запущены до любых изменений production-кода. + +## Порядок работы + +1. Перед началом имплементации сохранить этот план в `docs/plans/2.md`. +2. Создать новый transformer-level тест и усилить существующий catch-all тест, не изменяя production-код. +3. Запустить новые тесты на текущей реализации и зафиксировать исходное поведение. +4. Выполнить полный pytest, чтобы подтвердить корректность тестовых изменений. +5. Только после добавления и проверки тестов заменить matcher-wide dispatch на `CSTTransformer.on_leave()`. +6. Повторно запустить новые тесты, полный набор проверок и compatibility matrix. + +Поскольку изменение сохраняет существующее поведение, добавляемые сначала тесты являются characterization-тестами и могут проходить до рефакторинга. Их задача — зафиксировать необходимые гарантии до замены механизма dispatch. + +## Детали изменений + +- В `cstvis/transformers/super_transformer.py` заменить `MatcherDecoratableTransformer` на `CSTTransformer`. +- Удалить reflection по `libcst.matchers`, matcher-декораторы, `get_all_matcher_nodes()`, `leave_all()` и дедупликацию через `nodes_ids`. +- Сохранить `super().__init__()` и `METADATA_DEPENDENCIES`. +- Перенести существующую conversion-логику в `on_leave(original_node, updated_node)`. +- Получать тип, координаты и metadata только из `original_node`. +- Передавать converter’у только `updated_node`. +- Возвращать `updated_node` для всех нетаргетных узлов, чтобы сохранять изменения их детей. +- Использовать сигнатуру `CSTNode → CSTNode` с точечным `# type: ignore[override]` и кратким пояснением межтиповых замен вроде `Add → Subtract`. +- Не вызывать `super().on_leave()`, поскольку `SuperTransformer` не поддерживает внешний subclass-контракт с `leave_*` или matcher hooks. +- В `cstvis/changer.py` удалить пустой `set()` из аргументов конструктора `SuperTransformer`. +- README, версию, зависимости и публичные экспорты не менять. + +## Тесты + +| Название теста | Суть: какое именно поведение тестируем | Как происходит подготовка | Что именно проверяем | Детали, если они нужны | +|---|---|---|---|---| +| `test_converter_receives_updated_parent_with_original_context` | Родительский converter вызывается после обработки детей, получает `updated_node`, а координаты и metadata продолжают описывать `original_node`. | В `tests/transformers/test_super_transformer.py` создать `Changer` для `before = 1\n`, зарегистрировать converter для `Assign` и получить его координату. Создать локальный тестовый подкласс `SuperTransformer`, который при выходе из дочернего `Name('before')` возвращает `Name('after')`, а для остальных узлов делегирует production-реализации. Запустить его через настоящий `MetadataWrapper` и `SourceOffsetResolver`. | Итоговый код равен `after = 1\n`. Converter вызван ровно один раз. Полученный converter’ом `Assign` уже содержит `Name('after')`. `Context.position.coordinate` равен исходной координате `Coordinate(None, 'Assign', 1, 0, 1, 10)`. `Context.position.source` остаётся равным `before = 1\n`. | Создать `tests/transformers/__init__.py` для зеркального расположения и Ruff `INP001`. Не использовать structural assertions на MRO или точный базовый класс. Тест должен обнаруживать передачу `original_node` converter’у, metadata lookup по `updated_node`, потерю изменения ребёнка и повторный dispatch. | +| `test_any_converter_dispatches_once_for_every_coordinate` | Каждая координата, найденная для catch-all converter, приводит ровно к одному вызову converter для того же типа узла. | Усилить и переименовать существующий `test_converter_for_any` в `tests/test_changer.py`. Использовать исходник `5 - 5 + 5`, сохранить список всех координат, а identity-converter должен добавлять в список фактически полученный `node`. Применить каждую координату независимо. Сохранить существующую параметризацию `with_context` и `unfold`. | Количество вызовов converter равно количеству координат. Координат по-прежнему больше десяти. Имя класса каждого переданного узла совпадает с `coordinate.class_name` на той же позиции. Каждый результат равен исходному тексту, поскольку converter возвращает узел без изменений. | Исправить текущий `nodes.append(nodes)` на сохранение самого `node`. Тест остаётся в `tests/test_changer.py`, поскольку проверяет публичную оркестрацию `Changer.iterate_coordinates()` и `Changer.apply_coordinate()`. | + +## Проверка и критерии приёмки + +- План сохранён в `docs/plans/2.md` до начала имплементации. +- Оба тестовых сценария добавлены и запущены до изменения production-кода. +- После рефакторинга оба сценария продолжают проходить без изменения ожидаемого поведения. +- Полный pytest проходит. +- Statement и branch coverage остаются 100%. +- `ruff check cstvis` и `ruff check tests` проходят. +- `mypy --strict cstvis` и `mypy tests` проходят. +- Отдельно проходят Python 3.8 с `libcst==1.1.0` и актуальная поддерживаемая версия Python с LibCST 1.9.0. +- Документированные регистрация converter’ов, координаты, `Context`, комментарии, source fragments и генерируемый код не меняются. diff --git a/pyproject.toml b/pyproject.toml index 43a5a16..a75d80b 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "setuptools.build_meta" [project] name = "cstvis" -version = "0.0.10" +version = "0.0.11" authors = [{ name = "Evgeniy Blinov", email = "zheni-b@yandex.ru" }] description = 'Incremental change of CST' readme = "README.md" @@ -30,6 +30,7 @@ classifiers = [ 'Programming Language :: Python :: 3.12', 'Programming Language :: Python :: 3.13', 'Programming Language :: Python :: 3.14', + 'Programming Language :: Python :: 3.15', 'Programming Language :: Python :: Free Threading', 'Programming Language :: Python :: Free Threading :: 3 - Stable', 'License :: OSI Approved :: MIT License', diff --git a/tests/test_changer.py b/tests/test_changer.py index d5579fa..f1a7b9f 100644 --- a/tests/test_changer.py +++ b/tests/test_changer.py @@ -1058,29 +1058,41 @@ def convert_ints(node: int): assert set(changer.apply_coordinate(coordinate) for coordinate in changer.iterate_coordinates()) == {'6 - 5 + 5', '5 - 6 + 5', '5 - 5 + 6'} -def test_converter_for_any(with_context, unfold): +def test_any_converter_is_invoked_once_per_coordinate(with_context, unfold): """ - An Any-annotated converter is considered for many CST nodes. + Invoke an `Any` converter exactly once per independently applied coordinate. - Applying every coordinate in `Changer('5 - 5 + 5')` invokes the identity converter more than ten times across node-only/context-aware callbacks and both decorator forms. + The captured node's type must match the coordinate's class name, and identity + conversion must preserve the source. Both callback signatures and decorator + forms are covered. """ - changer = Changer('5 - 5 + 5') + source = '5 - 5 + 5' + changer = Changer(source) - nodes = [] + captured_nodes = [] if with_context: @unfold(changer.converter) - def do_something(node: Any, context): # noqa: ARG001 - nodes.append(nodes) + def capture_node(node: Any, context): # noqa: ARG001 + captured_nodes.append(node) return node else: @unfold(changer.converter) - def do_something(node: Any): - nodes.append(nodes) + def capture_node(node: Any): + captured_nodes.append(node) return node - [changer.apply_coordinate(coordinate) for coordinate in changer.iterate_coordinates()] - assert len(nodes) > 10 + coordinates = list(changer.iterate_coordinates()) + assert len(coordinates) > 10 + + for coordinate in coordinates: + call_count_before = len(captured_nodes) + assert changer.apply_coordinate(coordinate) == source + + assert len(captured_nodes) == call_count_before + 1 + assert captured_nodes[-1].__class__.__name__ == coordinate.class_name + + assert len(captured_nodes) == len(coordinates) def test_if_node_is_not_exist_nothing_changed(with_context, unfold): @@ -1122,8 +1134,8 @@ def filter_something(node: float, context): # noqa: ARG001 converter = list(changer.converters_by_types.values())[0][0] # noqa: RUF015 filter = list(changer.filters_by_types.values())[0][0] # noqa: RUF015, A001 - assert converter.get_function_id() == 'tests.test_changer:do_something:1114' - assert filter.get_function_id() == 'tests.test_changer:filter_something:1118' + assert converter.get_function_id() == 'tests.test_changer:do_something:1126' + assert filter.get_function_id() == 'tests.test_changer:filter_something:1130' def test_wrong_converter_and_wrong_filter(unfold): diff --git a/tests/transformers/__init__.py b/tests/transformers/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/tests/transformers/test_super_transformer.py b/tests/transformers/test_super_transformer.py new file mode 100644 index 0000000..ba26a61 --- /dev/null +++ b/tests/transformers/test_super_transformer.py @@ -0,0 +1,54 @@ +from libcst import Assign, CSTNode, Name, metadata +from libcst.metadata import CodePosition, CodeRange + +from cstvis import Changer, Context, Coordinate +from cstvis.source_offsets import SourceOffsetResolver +from cstvis.transformers.super_transformer import SuperTransformer + + +def test_converter_receives_updated_parent_with_original_context(): + """ + Pass a child update to the parent converter with the parent's original context. + + The transformer renames the assignment target on child leave, then delegates + parent leave to `SuperTransformer`. The converter receives the updated + `Assign` exactly once, while its `Context` retains the original coordinate, + whitespace-inclusive range, and source. + """ + source = 'before = 1\n' + changer = Changer(source) + converter_calls = [] + + @changer.converter + def capture_assign(node: Assign, context: Context) -> Assign: + converter_calls.append((node, context)) + return node + + coordinate = next(changer.iterate_coordinates()) + + class ChildUpdatingSuperTransformer(SuperTransformer): + def on_leave(self, original_node: CSTNode, updated_node: CSTNode) -> CSTNode: # type: ignore[override] + if isinstance(original_node, Name) and original_node.value == 'before': + return Name('after') + return super().on_leave(original_node, updated_node) + + wrapper = metadata.MetadataWrapper(changer.module) + node_ranges = wrapper.resolve(metadata.WhitespaceInclusivePositionProvider) + transformer = ChildUpdatingSuperTransformer( + coordinate, + changer.converters_by_types, + changer._comments_by_lines, + SourceOffsetResolver(wrapper.module, source, node_ranges.values()), + ) + transformed_source = wrapper.visit(transformer).code + + assert transformed_source == 'after = 1\n' + assert len(converter_calls) == 1 + updated_assign, context = converter_calls[0] + updated_target = updated_assign.targets[0].target + assert isinstance(updated_target, Name) + assert updated_target.value == 'after' + position = context.position + assert position.coordinate == Coordinate(None, 'Assign', 1, 0, 1, 10) + assert position.node_range == CodeRange(CodePosition(1, 0), CodePosition(1, 10)) + assert position.source == source