Skip to content

Python: search preconditions walk the LST with the generated visitor - #8748

Merged
jkschneider merged 1 commit into
mainfrom
python-sdk-search-preconditions-walk-the-lst-by-reflection
Sep 2, 2026
Merged

Python: search preconditions walk the LST with the generated visitor#8748
jkschneider merged 1 commit into
mainfrom
python-sdk-search-preconditions-walk-the-lst-by-reflection

Conversation

@knutwannheden

@knutwannheden knutwannheden commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Three of the four Python search preconditions walked the LST by reflection — dir(cur) plus a getattr on each of ~70 public attributes, per node — where every other language uses its own visitor. On psf/requests tests/test_requests.py (3094 lines, 15044 LST nodes), per-node cost against a PythonVisitor traversal of the same tree:

                                  before    after
UsesImport (module not present)  35.9 us   6.6 us/node
UsesMethod (method not present)  35.9 us   6.0 us/node
UsesType   (type not present)    35.8 us   5.7 us/node

Those are the miss cases, which is what a gate mostly does. A hit is far cheaper still, because the visitor stops at the first matching node where the walk collected the whole tree first: UsesImport("pytest") on the same file goes from 549 ms to 0.36 ms.

A file is gated once per sub-recipe, so 25 ChangeImport sub-recipes over that one file — the shape of org.openrewrite.python.migrate.ReplaceCollectionsAbcImports — go from 17.95s to 3.20s. None of that is the recipe's own work: the gate never matches, so the run is precondition time end to end.

_FindFirst is a PythonVisitor that stops at the first node its matches accepts, and the three preconditions each supply one. It dispatches straight to accept, since a search never rewrites and nothing reads the cursor the base visit builds. The seen set and the try/except around getattr go with the walk; IsSourceFile never used it.

Do both traversals reach the same nodes

Reflection's rule — any attribute holding something with id or markers — is not the visitor's, so I compared the two node sets over 1499 stdlib files (1.38M visited nodes):

reached only by reflection: Clause 1330, Condition 275, Pattern 3
reached only by visitor   : Kind 3790

Comprehension clauses, conditions and match-case patterns are reached through visit_comprehension_clause and friends rather than through visit, so their children are traversed either way, and none of the three is a TypedTree, a J.MethodInvocation or an import node. J.ClassDeclaration.Kind is exposed only on .padding, which is why reflection missed it.

Answers agreed on all 1881 (query, file) pairs checked: 33 queries × 37 files parsed without attribution, and 33 × 20 with ty-types attached. UsesType gets a sharper check, per FQN rather than per file, because a per-file comparison hides a node class whose type is also reachable from a sibling: every one of the 300 distinct fully-qualified names the old walk could match on across 14 ty-attributed files still matches. That check is why _TypeSearch reads getattr(tree, "type", None)Expression declares its own type alongside TypedTree rather than under it, so an isinstance against either alone silently narrows the gate.

The one behaviour change: depth

The visitor recurses ~5 Python frames per LST level against an interpreter limit of 2000, so it gives out around 350-400 levels of nesting where the iterative walk did not. That is lower than it sounds reachable — a 400-term or chain does it, as does pydoc_data/topics.py at 2295 levels. search catches RecursionError, logs the path at WARNING and answers "no match".

No file loses an edit to this, because every editor in the SDK is an equally recursive PythonVisitor and gives out at the same depth:

400-term `or` chain + `import os`     old gate   new gate   editor
  import first                          match      match     RecursionError
  import last                           match      no match  RecursionError

So the gate's answer changes only for a file the editor was going to fail on anyway, and only when the deep expression precedes the match. What changes is the diagnostic: an error naming a file no recipe could have changed becomes a WARNING naming a file that was skipped. Letting the RecursionError propagate instead would restore the error, on every sweep that touches such a file; the log line is the compromise, so the skip still leaves a trace.

Tests

tests/recipes/test_search_traversal.py parses one fixture that buries each match below a decorated function, a nested class and method, and then below an f-string comprehension, a with, or an except block. It asserts the answer for a present and an absent type, method and import; replacing the descent with return tree fails all three. A fourth test pins that list, attributed to the comprehension node itself, is found — it fails against an isinstance(tree, TypedTree) gate. A fifth builds a 3000-fragment concatenation — deterministic, not dependent on a stdlib version — and fails if either the RecursionError catch or the warning goes.

Full Python SDK suite: 2194 passed, 11 skipped, including the Java-peer RPC tests.

@github-project-automation github-project-automation Bot moved this to In Progress in OpenRewrite Sep 2, 2026
@knutwannheden
knutwannheden force-pushed the python-sdk-search-preconditions-walk-the-lst-by-reflection branch from 17b1d8f to 7d5e491 Compare September 2, 2026 10:56
UsesType, UsesMethod and UsesImport reached every node through dir()/getattr
over ~70 public attributes, at 36 us/node against the 6 us/node a PythonVisitor
traversal costs. A file is gated once per sub-recipe, so a 25-recipe composite
paid it 25 times.
@knutwannheden
knutwannheden force-pushed the python-sdk-search-preconditions-walk-the-lst-by-reflection branch from 7d5e491 to c33ff5d Compare September 2, 2026 11:04
@jkschneider
jkschneider merged commit 22c4a5a into main Sep 2, 2026
1 check passed
@jkschneider
jkschneider deleted the python-sdk-search-preconditions-walk-the-lst-by-reflection branch September 2, 2026 12:02
@github-project-automation github-project-automation Bot moved this from In Progress to Done in OpenRewrite Sep 2, 2026
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.

2 participants