Skip to content

python: implement TreeVisitor.adapt() so generic visitors can visit Py/J nodes#7590

Merged
jkschneider merged 1 commit intomainfrom
perf/python-tree-visitor-adapt
May 6, 2026
Merged

python: implement TreeVisitor.adapt() so generic visitors can visit Py/J nodes#7590
jkschneider merged 1 commit intomainfrom
perf/python-tree-visitor-adapt

Conversation

@jkschneider
Copy link
Copy Markdown
Member

@jkschneider jkschneider commented May 6, 2026

Summary

TreeVisitor.adapt() was stubbed in rewrite-python (# FIXME implement the visitor adapting) — it just returned self. That's load-bearing for correctness because each language's LST root routes its accept(v, p) through the adapter:

class J:
    def accept(self, v, p):
        return self.accept_java(v.adapt(J, JavaVisitor), p)

class Py(J):
    def accept(self, v, p):
        return self.accept_python(v.adapt(Py, PythonVisitor), p)

When v is a bare TreeVisitor (or a direct subclass that overrides only pre_visit/post_visit — a common pattern for cross-language collectors), the stubbed adapt flowed v straight into accept_java/accept_python, which immediately calls language-specific dispatch:

class CompilationUnit:
    def accept_python(self, v, p):
        return v.visit_compilation_unit(self, p)

The bare TreeVisitor doesn't implement visit_compilation_unitAttributeError, wrapped in RecipeRunException by the visitor framework's outer try/except and propagated up. Reproducible failure:

class _Collector(TreeVisitor):
    def pre_visit(self, tree, p):
        return tree

_Collector().visit(some_py_compilation_unit, None)
# RecipeRunException: AttributeError: '_Collector' object has no attribute 'visit_compilation_unit'

Implementation

  • A class-level registry on TreeVisitor maps each language visitor base class to a small adapter class. Each language module (rewrite.java.visitor, rewrite.python.visitor) registers its adapter at import time via TreeVisitor.register_adapter.

  • adapt(tree_type, visitor_type) returns:

    • self if the visitor is already an instance of visitor_type
    • otherwise an adapter instance that is-a visitor_type (so the language-specific visit_* defaults in JavaVisitor / PythonVisitor are available for child traversal) and forwards pre_visit / post_visit / default_value / is_acceptable plus _cursor / _visit_count / _after_visit to the wrapped visitor — so user-defined logic on the original generic visitor still runs against the right cursor and sees every traversed node.
  • When adapt is called on an existing adapter (e.g. cross-language traversal: a Py node visited via a JavaVisitor adapter chain), the wrapped visitor is unwrapped and re-wrapped in the correct adapter rather than nesting adapters.

is_adaptable_to is updated to match the new semantics: a visitor is adaptable to a target type if it's already an instance, or if it's an existing adapter, or if a registered adapter exists for the target type.

Test plan

  • New tests/test_visitor_adapt.py covers:
    • The regression — a bare TreeVisitor subclass visiting a Py.CompilationUnit no longer raises
    • pre_visit actually fires on the visited node
    • End-to-end: _collect_search_result_ids (which uses a TreeVisitor subclass) finds a SearchResult marker on a Py root
    • adapt is a no-op when the visitor already is-a target type
    • adapt unwraps an existing adapter rather than stacking adapters
  • Manual smoke test against the production install path (~/.moderne/cli/python/openrewrite/8.81.6/) — same minimal repro that crashed before this fix now succeeds and pre_visit is invoked.

… Py / J nodes

`TreeVisitor.adapt()` was stubbed (`# FIXME implement the visitor adapting`) —
it just returned `self`. That's load-bearing for correctness because each
language's LST root routes its `accept(v, p)` through the adapter:

```python
class J:
    def accept(self, v, p):
        return self.accept_java(v.adapt(J, JavaVisitor), p)

class Py(J):
    def accept(self, v, p):
        return self.accept_python(v.adapt(Py, PythonVisitor), p)
```

When `v` is a bare `TreeVisitor` (or a direct subclass that overrides only
`pre_visit` / `post_visit` — a common pattern for cross-language collectors),
the stubbed `adapt` flowed `v` straight into `accept_java` / `accept_python`,
which immediately calls language-specific dispatch:

```python
class CompilationUnit:
    def accept_python(self, v, p):
        return v.visit_compilation_unit(self, p)
```

The bare `TreeVisitor` doesn't implement `visit_compilation_unit` →
`AttributeError`, wrapped in `RecipeRunException` by the visitor framework's
outer try/except, propagating up. A reproducible failure case:

```python
class _Collector(TreeVisitor):
    def pre_visit(self, tree, p):
        return tree
_Collector().visit(some_py_compilation_unit, None)
# RecipeRunException: AttributeError: '_Collector' object has no attribute 'visit_compilation_unit'
```

This blocks any correct implementation of `_collect_search_result_ids` (and
similar cross-language tree walks) from extending the bare `TreeVisitor`. PR
#7589 worked around the symptom by replacing the visitor-based collector
with a hand-rolled iterative walker; that's the wrong layer to fix it.

This change implements `adapt()` properly:

* A class-level registry on `TreeVisitor` maps each language visitor base
  class to a small adapter class. Each language module
  (`rewrite.java.visitor`, `rewrite.python.visitor`) registers its adapter
  at import time via `TreeVisitor.register_adapter`.

* `adapt(tree_type, visitor_type)` returns:
  - `self` if the visitor is already an instance of `visitor_type`
  - otherwise an adapter instance that *is-a* `visitor_type` (so the
    language-specific `visit_*` defaults in JavaVisitor / PythonVisitor are
    available for child traversal) and forwards `pre_visit` / `post_visit`
    / `default_value` / `is_acceptable` plus `_cursor` / `_visit_count` /
    `_after_visit` to the wrapped visitor — so user-defined logic on the
    original generic visitor still runs against the right cursor and sees
    every traversed node.

* When `adapt` is called on an existing adapter (e.g. cross-language
  traversal: a Py node visited via a JavaVisitor adapter chain), the
  wrapped visitor is unwrapped and re-wrapped in the correct adapter
  rather than nesting adapters.

`is_adaptable_to` is updated to match the new semantics: a visitor is
adaptable to a target type if it's already an instance, or if it's an
existing adapter, or if a registered adapter exists for the target type.

Tests: a new `tests/test_visitor_adapt.py` covers the regression (a bare
`TreeVisitor` subclass visiting a `Py.CompilationUnit`), the end-to-end
search-result collection through `_collect_search_result_ids`, and the
unwrap-then-readapt behaviour for cross-language traversal.
@github-project-automation github-project-automation Bot moved this to In Progress in OpenRewrite May 6, 2026
@jkschneider jkschneider merged commit 96c08d1 into main May 6, 2026
1 check passed
@jkschneider jkschneider deleted the perf/python-tree-visitor-adapt branch May 6, 2026 23:52
@github-project-automation github-project-automation Bot moved this from In Progress to Done in OpenRewrite May 6, 2026
jkschneider added a commit that referenced this pull request May 7, 2026
…isitors can visit Cs / J nodes (#7591)

Mirrors the Python fix from c89d246 (#7590), but for the parallel .NET
implementation in `rewrite-csharp/csharp/OpenRewrite/`. Two distinct failure
modes existed before this change:

1. A bare `TreeVisitor<J, P>` (or any subclass that overrides only
   `PreVisit` / `PostVisit` — a common pattern for cross-language
   collectors) traversing a parsed C# source would silently no-op past the
   root: the default `Accept` is identity, so traversal never descended
   into children. Quiet, but every per-node hook missed every node.

2. A plain `JavaVisitor<P>` traversing a Cs source would throw
   `InvalidOperationException("Unknown J tree type: ...")` on the first
   `Cs.UsingDirective` / `Cs.PropertyDeclaration` / etc. its switch
   doesn't recognize.

Unlike the JVM side (which inherits a real `TreeVisitor.adapt()` from
`rewrite-core` backed by Quarkus Gizmo bytecode generation), and unlike
the TypeScript side (which sidesteps the problem with a kind-keyed
registry consulted only inside `JavaVisitor.accept`), the C# port had no
adaptation mechanism at all.

This change introduces one:

* `TreeVisitorAdapterRegistry` — a non-generic, lock-protected registry
  of `(treeType, openLangVisitorType, openAdapterType)` triples kept
  sorted with the most specific tree type first, so a Cs node hits the
  `CSharpVisitor` adapter before falling through to the `JavaVisitor`
  one.

* `TreeVisitor<T, P>.Adapt(tree)` — looks up the matching language
  adapter, closes the open generic with `P`, and instantiates via
  `Activator`. Result is cached per visitor instance keyed by the closed
  language-visitor type, so the wrapper allocation happens at most once
  per language per visitor lifetime.

* `Visit()` now dispatches via `Adapt(t).Accept(t, p)` instead of
  `Accept(t, p)`. `Cursor` is made virtual so the adapter can forward it
  to the wrapped visitor.

* `TreeVisitorAsJavaVisitor<P>` and `TreeVisitorAsCSharpVisitor<P>` —
  thin adapter classes that IS-A `JavaVisitor` / `CSharpVisitor` (so the
  language switch and child-traversal defaults are inherited), but
  override `Visit` to forward to the wrapped visitor (so its lifecycle —
  `_visitCount`, `_afterVisit`, `_stopAfterPreVisit`, `PreVisit`,
  `PostVisit`, `DefaultValue` — runs unchanged) and override `Cursor` to
  forward to wrapped state. Each language registers its triple via a
  `[ModuleInitializer]` that fires on assembly load, so registration is
  guaranteed before any `TreeVisitor<J, P>.Visit()` call regardless of
  which generic instantiations the user code happens to reference.

Tests: `TreeVisitorAdaptTest` covers both regression modes — a
`CountingPreVisitor : TreeVisitor<J, ExecutionContext>` overriding only
`PreVisit` must descend into both Cs and J nodes of a parsed file, and a
`MethodCountingJavaVisitor : JavaVisitor<ExecutionContext>` must
traverse the same source without throwing. Full suite stays green
(1917/1917 with `RPC_TEST_SERVER_CLASSPATH` wired via
`./gradlew :rewrite-csharp:rpcTestClasspath`).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

1 participant