GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — `ast.NodeVisitor` for import tracking by ShaharNaveh · Pull Request #7229 · RustPython/RustPython Conversation
|
📝 WalkthroughWalkthroughThe pull request refactors import parsing logic in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
ShaharNaveh
marked this pull request as ready for review
February 26, 2026 12:27
Contributor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/update_lib/cmd_todo.py`:
- Line 337: The code that derives lib_name from test_name using chained
removeprefix calls produces a leftover leading underscore for names like
"_test_atexit" (e.g., test_name -> lib_name via lib_name =
test_name.removeprefix("test_").removeprefix("_test")), causing lib_status
lookups to fail; fix by normalizing test_name first (strip leading underscores
or check both "test_" and "_test_" patterns) and then remove the correct prefix
so that lib_name becomes "atexit" (update the logic around lib_name derivation
where test_name is processed, ensuring downstream lib_status and DEPENDENCIES
lookups receive the cleaned library name).
In `@scripts/update_lib/deps.py`:
- Around line 140-142: The docstring for count_rustpython_todo says “Count lines
containing RustPython TODO markers” but the implementation returns
content.count(TODO_MARKER) (counting occurrences); update one to match the
other: either change the docstring to “Count occurrences of TODO_MARKER in
content” or change the implementation in count_rustpython_todo to perform a
line-based count (e.g., iterate over content.splitlines() and sum 1 for lines
that contain TODO_MARKER) so the behavior matches the docstring; references:
function count_rustpython_todo and symbol TODO_MARKER.
- Around line 96-102: In visit_Call, guard access to node.args[0] by first
checking that node.args is non-empty and then ensure the constant's value is a
string before calling endswith; specifically, before "arg = node.args[0]" check
"if not node.args: return" and after confirming isinstance(arg, ast.Constant)
verify "isinstance(arg.value, str)" (or similar) before assigning target/using
target.endswith(".py") so non-string constants or missing args won't raise
IndexError or AttributeError.
- Around line 51-55: The code currently strips module roots by using
name.split(".",1)[-1] and also constructs from-import entries as "module.alias"
which breaks for relative imports (module is None); change the import parsing so
Import names use the top-level component (use name.split(".",1)[0] for
real_name) and change the ImportFrom handling to: if node.module is None
(relative import) use the alias name as the import key, otherwise use the full
module (or module + "." + alias if you need the specific attribute) instead of
blindly synthesizing "module.alias"; update where real_name is added to
self.test_imports / self.lib_imports accordingly.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
| else: | ||
| # Extract lib name from test name (test_foo -> foo) | ||
| lib_name = test_name.removeprefix("test_") | ||
| lib_name = test_name.removeprefix("test_").removeprefix("_test") |
Contributor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
cat -n scripts/update_lib/cmd_todo.py | sed -n '330,345p'
Repository: RustPython/RustPython
Length of output: 1000
🏁 Script executed:
cat -n scripts/update_lib/cmd_todo.py | sed -n '320,360p'
Repository: RustPython/RustPython
Length of output: 1932
🏁 Script executed:
find . -name "*.py" -type f | xargs grep -l "_test_atexit\|_test_eintr" 2>/dev/null | head -10
Repository: RustPython/RustPython
Length of output: 144
🏁 Script executed:
cat -n scripts/update_lib/deps.py | grep -A 5 -B 5 "atexit\|eintr" | head -30
Repository: RustPython/RustPython
Length of output: 964
🏁 Script executed:
python3 <<'PY' # Test the actual behavior cases = [ "test_os", "test_atexit", "_test_atexit", "_test_eintr", "test_code", "_test_code" ] print("Testing current implementation:") for t in cases: result = t.removeprefix("test_").removeprefix("_test") print(f" {t:20} → {result:15}") print("\nTesting proposed fix:") for t in cases: result = t.removeprefix("test_").removeprefix("_test_") print(f" {t:20} → {result:15}") PY
Repository: RustPython/RustPython
Length of output: 611
🏁 Script executed:
grep -A 200 "DEPENDENCIES = {" scripts/update_lib/deps.py | head -100
Repository: RustPython/RustPython
Length of output: 2702
Fix _test_* prefix fallback mapping to correctly extract library name.
At line 337, tests with _test_ prefix (e.g., _test_atexit) fail to map correctly. The current chained removeprefix leaves a leading underscore: _test_atexit → _atexit instead of atexit. This causes subsequent lib_status lookup at line 342 to miss the correct library entry, breaking todo prioritization and scoring for these edge-case tests that exist in the DEPENDENCIES mapping but not in the primary test-to-library lookup.
Suggested fix
- lib_name = test_name.removeprefix("test_").removeprefix("_test") + lib_name = test_name.removeprefix("test_").removeprefix("_test_")
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| lib_name = test_name.removeprefix("test_").removeprefix("_test") | |
| lib_name = test_name.removeprefix("test_").removeprefix("_test_") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/update_lib/cmd_todo.py` at line 337, The code that derives lib_name
from test_name using chained removeprefix calls produces a leftover leading
underscore for names like "_test_atexit" (e.g., test_name -> lib_name via
lib_name = test_name.removeprefix("test_").removeprefix("_test")), causing
lib_status lookups to fail; fix by normalizing test_name first (strip leading
underscores or check both "test_" and "_test_" patterns) and then remove the
correct prefix so that lib_name becomes "atexit" (update the logic around
lib_name derivation where test_name is processed, ensuring downstream lib_status
and DEPENDENCIES lookups receive the cleaned library name).
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
Comment on lines
+51
to
+55
| real_name = name.split(".", 1)[-1] | ||
| if name.startswith("test."): | ||
| self.test_imports.add(real_name) | ||
| else: | ||
| self.lib_imports.add(real_name) |
Contributor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Import parsing currently loses module roots and misclassifies from imports.
At Lines 51-55, name.split(".", 1)[-1] turns os.path into path; at Lines 61-70, from imports are synthesized as module.alias, which also yields bad entries for relative imports (module is None). This will skew dependency resolution broadly.
💡 Suggested fix
def add_import(self, name: str) -> None: @@ - real_name = name.split(".", 1)[-1] - if name.startswith("test."): - self.test_imports.add(real_name) - else: - self.lib_imports.add(real_name) + if name.startswith("test."): + # Keep first component under test.* (e.g. test.test_foo.bar -> test_foo) + rest = name.removeprefix("test.") + self.test_imports.add(rest.split(".", 1)[0]) + return + # Track top-level stdlib module (e.g. os.path -> os) + self.lib_imports.add(name.split(".", 1)[0]) @@ def visit_ImportFrom(self, node): - try: - module = node.module - except AttributeError: - # Ignore `from . import my_internal_module` - return - - for name in node.names: - self.add_import(f"{module}.{name}") + # Ignore relative-only imports like: from . import foo + if not node.module: + return + # Keep module-level dependency; alias names are often attributes, not modules + self.add_import(node.module)
#!/bin/bash python - <<'PY' import ast samples = [ "import os.path", "from xml.etree import ElementTree", "from . import _local", ] for s in samples: n = ast.parse(s).body[0] if isinstance(n, ast.Import): name = n.names[0].name print(f"{s!r} -> current split gives: {name.split('.',1)[-1]!r}") else: module = n.module alias = n.names[0].name print(f"{s!r} -> synthesized: {module}.{alias}") PY
Also applies to: 61-70
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/update_lib/deps.py` around lines 51 - 55, The code currently strips
module roots by using name.split(".",1)[-1] and also constructs from-import
entries as "module.alias" which breaks for relative imports (module is None);
change the import parsing so Import names use the top-level component (use
name.split(".",1)[0] for real_name) and change the ImportFrom handling to: if
node.module is None (relative import) use the alias name as the import key,
otherwise use the full module (or module + "." + alias if you need the specific
attribute) instead of blindly synthesizing "module.alias"; update where
real_name is added to self.test_imports / self.lib_imports accordingly.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
Comment on lines
+96
to
+102
| arg = node.args[0] | ||
| if not isinstance(arg, ast.Constant): | ||
| return | ||
|
|
||
| target = arg.value | ||
| if not target.endswith(".py"): | ||
| return |
Contributor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # First, let's examine the file to understand the context cat -n scripts/update_lib/deps.py | sed -n '85,110p'
Repository: RustPython/RustPython
Length of output: 951
🏁 Script executed:
#!/bin/bash # Run the provided AST demonstration python - <<'PY' import ast for src in ["support.findfile()", "support.findfile(123)", "support.findfile('_test_atexit.py')"]: call = ast.parse(src).body[0].value print(src, "args_len=", len(call.args), "arg0_type=", type(call.args[0]).__name__ if call.args else None) PY
Repository: RustPython/RustPython
Length of output: 233
🏁 Script executed:
#!/bin/bash # Let's check the broader context around the visit_Call method grep -n "def visit_Call" scripts/update_lib/deps.py
Repository: RustPython/RustPython
Length of output: 107
visit_Call should validate findfile arguments to prevent crashes.
At line 96, indexing node.args[0] raises IndexError when no arguments are provided. At line 101, target.endswith(".py") assumes arg.value is a string, but ast.Constant can hold any constant type (int, float, None, etc.), causing AttributeError on non-string values.
Suggested fix
+ if not node.args: + return arg = node.args[0] if not isinstance(arg, ast.Constant): return target = arg.value + if not isinstance(target, str): + return if not target.endswith(".py"): return
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| arg = node.args[0] | |
| if not isinstance(arg, ast.Constant): | |
| return | |
| target = arg.value | |
| if not target.endswith(".py"): | |
| return | |
| if not node.args: | |
| return | |
| arg = node.args[0] | |
| if not isinstance(arg, ast.Constant): | |
| return | |
| target = arg.value | |
| if not isinstance(target, str): | |
| return | |
| if not target.endswith(".py"): | |
| return |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/update_lib/deps.py` around lines 96 - 102, In visit_Call, guard
access to node.args[0] by first checking that node.args is non-empty and then
ensure the constant's value is a string before calling endswith; specifically,
before "arg = node.args[0]" check "if not node.args: return" and after
confirming isinstance(arg, ast.Constant) verify "isinstance(arg.value, str)" (or
similar) before assigning target/using target.endswith(".py") so non-string
constants or missing args won't raise IndexError or AttributeError.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
Comment on lines
140
to
+142
| def count_rustpython_todo(content: str) -> int: | ||
| """Count lines containing RustPython TODO markers.""" | ||
| return sum(1 for line in content.splitlines() if TODO_MARKER in line) | ||
| return content.count(TODO_MARKER) |
Contributor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Docstring and implementation disagree on TODO semantics.
At Line 141 the doc says “Count lines…”, but Line 142 counts marker occurrences. Either update the docstring or switch to line-based counting to avoid ambiguity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/update_lib/deps.py` around lines 140 - 142, The docstring for
count_rustpython_todo says “Count lines containing RustPython TODO markers” but
the implementation returns content.count(TODO_MARKER) (counting occurrences);
update one to match the other: either change the docstring to “Count occurrences
of TODO_MARKER in content” or change the implementation in count_rustpython_todo
to perform a line-based count (e.g., iterate over content.splitlines() and sum 1
for lines that contain TODO_MARKER) so the behavior matches the docstring;
references: function count_rustpython_todo and symbol TODO_MARKER.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
Member
|
isn't this very slow? I removed ast because it was too slow for interactive tools to parse every test files. |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Contributor
Author
$ hyperfine --warmup 1 --command-name before --prepare 'git switch main || true' 'python scripts/update_lib/ todo --done --lib ~/Work/cpython/' --command-name after --prepare 'git switch deps-nodevisitor' 'python scripts/update_lib/ todo --done --lib ~/Work/cpython' --export-markdown ~/out.md
It's actually faster this way 🤷 |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Contributor
It's within the margin of error. |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
youknowone
added a commit
to youknowone/RustPython
that referenced
this pull request
Feb 26, 2026
This reverts commit 804a7e4.
youknowone
added a commit
that referenced
this pull request
Feb 26, 2026
This reverts commit 804a7e4.
Member
|
@ShaharNaveh I reverted this in #7230 due to CI. Let's fix update_libs and merge this again. Sorry for inconvenience! |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
ShaharNaveh
added a commit
to ShaharNaveh/RustPython
that referenced
this pull request
Feb 27, 2026
…stPython#7230) This reverts commit a47572c.
youknowone
pushed a commit
to youknowone/RustPython
that referenced
this pull request
Mar 22, 2026
youknowone
added a commit
to youknowone/RustPython
that referenced
this pull request
Mar 22, 2026
…tPython#7230) This reverts commit 804a7e4.
youknowone
pushed a commit
to youknowone/RustPython
that referenced
this pull request
Mar 22, 2026
* Reapply "`ast.NodeVisitor` for import tracking (RustPython#7229)" (RustPython#7230) This reverts commit a47572c.
youknowone
pushed a commit
that referenced
this pull request
Sep 16, 2026
youknowone
added a commit
that referenced
this pull request
Sep 16, 2026
This reverts commit 804a7e4.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Reviewers
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assignedLabels
skip:ci
Skip running the ci
Projects
Milestone
No milestoneDevelopment
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.