Reapply "ast.NodeVisitor for import tracking"#7241
Merged
youknowone merged 5 commits intoRustPython:mainfrom Feb 28, 2026
Merged
Reapply "ast.NodeVisitor for import tracking"#7241youknowone merged 5 commits intoRustPython:mainfrom
ast.NodeVisitor for import tracking"#7241youknowone merged 5 commits intoRustPython:mainfrom
Conversation
…stPython#7230) This reverts commit a47572c.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughRefactors import parsing in Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
youknowone
changed the title
ast.NodeVisitor for import tracking (#7229)" (#7230)ast.NodeVisitor for import tracking"
Contributor
Author
|
New output:
Standalone Tests
Untracked Files
Original Files
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
scripts/update_lib/deps.py (4)
53-60: Remove commented-out code.Line 56 contains a commented-out expression
# module.split(".", 1)[0]that appears to be a debug artifact or deferred decision.♻️ Clean up
`@property` def lib_imports(self) -> frozenset[str]: return frozenset( - # module.split(".", 1)[0] module for module in self.__imports if not module.startswith("test.") )🤖 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 53 - 60, Remove the leftover commented-out expression from the lib_imports property: delete the line containing "# module.split(\".\", 1)[0]" inside the lib_imports getter so the generator comprehension only yields module from self.__imports when not starting with "test."; ensure no other logic is altered in the lib_imports property or references to __imports.
110-112: PotentialIndexErrorifsupport.findfile()is called without arguments.Line 110 accesses
node.args[0]without checking ifargsis non-empty. While unlikely in practice (a call tofindfile()without arguments would be a bug in the test code), defensive coding would prevent a crash in the import parser.🛡️ Proposed defensive check
if (value.id != "support") or (func.attr != "findfile"): return + if not node.args: + return + arg = node.args[0] if not isinstance(arg, ast.Constant): 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 110 - 112, The code accesses node.args[0] without ensuring node.args is non-empty which can raise IndexError when parsing a call like support.findfile() with no arguments; update the logic around the node handling (where node.args is read) to first check that node.args is truthy/has length > 0 before accessing node.args[0], and early-return (as done for the ast.Constant check) if there are no args so support.findfile() calls without args are safely ignored.
122-139: Return type annotation mismatch.
parse_test_importsandparse_lib_importsare annotated to returnset[str], but they returnvisitor.test_importsandvisitor.lib_importswhich arefrozenset[str]. This works at runtime (frozenset is compatible), but the type hints are inaccurate.♻️ Fix type annotations
-def parse_test_imports(content: str) -> set[str]: +def parse_test_imports(content: str) -> frozenset[str]: """Parse test file content and extract test package dependencies.""" if not (tree := safe_parse_ast(content)): - return set() + return frozenset() visitor = ImportVisitor() visitor.visit(tree) return visitor.test_imports -def parse_lib_imports(content: str) -> set[str]: +def parse_lib_imports(content: str) -> frozenset[str]: """Parse library file and extract all imported module names.""" if not (tree := safe_parse_ast(content)): - return set() + return frozenset() visitor = ImportVisitor() visitor.visit(tree) return visitor.lib_imports🤖 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 122 - 139, The functions parse_test_imports and parse_lib_imports are annotated to return set[str] but actually return frozenset values (visitor.test_imports and visitor.lib_imports); either change the return type annotations to frozenset[str] or explicitly convert the returned frozenset to a set() before returning. Update the signatures of parse_test_imports and parse_lib_imports to return frozenset[str] (or wrap visitor.test_imports/visitor.lib_imports with set(...) if you prefer a mutable set) so the type annotations match the actual return values from ImportVisitor.
67-74: Unnecessarytry/exceptblock:ast.ImportFrom.modulenever raisesAttributeError.The
moduleattribute always exists onast.ImportFromnodes—it'sNonefor relative imports likefrom . import x, not missing. Theexcept AttributeErrorbranch is unreachable. The subsequentif module is Nonecheck at line 73 correctly handles this case.♻️ Simplified version
def visit_ImportFrom(self, node): - try: - module = node.module - except AttributeError: - # Ignore `from . import my_internal_module` - return - - if module is None: # Ignore `from . import my_internal_module` + module = node.module + if module is None: # Relative import: `from . import my_internal_module` 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 67 - 74, Remove the unreachable try/except around accessing node.module: replace the try block that sets "module = node.module" and the except AttributeError branch with a direct assignment (module = node.module) and keep the existing None check (if module is None: return); remove the except AttributeError branch and any comment about ignoring AttributeError so the code simply reads module = node.module followed by the None-check.scripts/update_lib/cmd_todo.py (1)
337-338: Potential edge case:_testprefix extraction leaves leading underscore.For test names like
_test_atexitnot found inDEPENDENCIES:
"_test_atexit".removeprefix("test_")→"_test_atexit"(unchanged).removeprefix("_test")→"_atexit"(still has leading underscore)This likely works in practice because
_test_*files (e.g.,_test_atexit.py) are typically listed inDEPENDENCIESand handled at lines 331-334. If a_test_*file isn't inDEPENDENCIES, the extractedlib_namewould retain a leading underscore.💡 If full extraction is needed
- lib_name = test_name.removeprefix("test_").removeprefix("_test") + lib_name = test_name.removeprefix("test_").removeprefix("_test").lstrip("_")Or handle the
_test_prefix explicitly:- lib_name = test_name.removeprefix("test_").removeprefix("_test") + if test_name.startswith("_test_"): + lib_name = test_name.removeprefix("_test_") + else: + lib_name = test_name.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` around lines 337 - 338, When extracting lib_name from test_name in the block using test_name.removeprefix("test_").removeprefix("_test"), handle the edge case where a leading underscore remains (e.g., "_test_atexit"); after the two removeprefix calls normalize the result by stripping a leading underscore (e.g., call lstrip or conditional trim) so lib_name has no leading underscore before checking DEPENDENCIES and assigning test_order; update the logic around the lib_name assignment (the variable named lib_name and its use with DEPENDENCIES/test_order) accordingly.
🤖 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/deps.py`:
- Line 119: The call to self.add_import(f"test.{target}") from
ImportVisitor.visit_Call fails because ImportVisitor lacks a public add_import
method and only has the private self.__imports set; add a public method named
add_import(self, name) on the ImportVisitor class that appends/inserts the given
name into self.__imports (or otherwise exposes a safe API to modify the set) and
update any tests/uses to call that method; alternatively, if you prefer not to
add a new method, change the visit_Call usage to directly use the private set
(e.g., self._ImportVisitor__imports.add(f"test.{target}")), but the preferred
fix is to implement add_import on ImportVisitor so visit_Call can call it
safely.
---
Nitpick comments:
In `@scripts/update_lib/cmd_todo.py`:
- Around line 337-338: When extracting lib_name from test_name in the block
using test_name.removeprefix("test_").removeprefix("_test"), handle the edge
case where a leading underscore remains (e.g., "_test_atexit"); after the two
removeprefix calls normalize the result by stripping a leading underscore (e.g.,
call lstrip or conditional trim) so lib_name has no leading underscore before
checking DEPENDENCIES and assigning test_order; update the logic around the
lib_name assignment (the variable named lib_name and its use with
DEPENDENCIES/test_order) accordingly.
In `@scripts/update_lib/deps.py`:
- Around line 53-60: Remove the leftover commented-out expression from the
lib_imports property: delete the line containing "# module.split(\".\", 1)[0]"
inside the lib_imports getter so the generator comprehension only yields module
from self.__imports when not starting with "test."; ensure no other logic is
altered in the lib_imports property or references to __imports.
- Around line 110-112: The code accesses node.args[0] without ensuring node.args
is non-empty which can raise IndexError when parsing a call like
support.findfile() with no arguments; update the logic around the node handling
(where node.args is read) to first check that node.args is truthy/has length > 0
before accessing node.args[0], and early-return (as done for the ast.Constant
check) if there are no args so support.findfile() calls without args are safely
ignored.
- Around line 122-139: The functions parse_test_imports and parse_lib_imports
are annotated to return set[str] but actually return frozenset values
(visitor.test_imports and visitor.lib_imports); either change the return type
annotations to frozenset[str] or explicitly convert the returned frozenset to a
set() before returning. Update the signatures of parse_test_imports and
parse_lib_imports to return frozenset[str] (or wrap
visitor.test_imports/visitor.lib_imports with set(...) if you prefer a mutable
set) so the type annotations match the actual return values from ImportVisitor.
- Around line 67-74: Remove the unreachable try/except around accessing
node.module: replace the try block that sets "module = node.module" and the
except AttributeError branch with a direct assignment (module = node.module) and
keep the existing None check (if module is None: return); remove the except
AttributeError branch and any comment about ignoring AttributeError so the code
simply reads module = node.module followed by the None-check.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
scripts/update_lib/deps.py (1)
66-74: Unnecessarytry/except AttributeErrorblock.The
moduleattribute always exists onast.ImportFromnodes—it'sNonefor relative imports likefrom . import x, not missing. Thetry/except AttributeErrorblock (lines 67-71) will never catch anything; theif module is Nonecheck on line 73 already handles the relative import case correctly.♻️ Simplify by removing dead code
def visit_ImportFrom(self, node): - try: - module = node.module - except AttributeError: - # Ignore `from . import my_internal_module` - return - - if module is None: # Ignore `from . import my_internal_module` + if node.module is None: # Ignore `from . import my_internal_module` return + module = node.module for alias in node.names:🤖 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 66 - 74, Remove the dead try/except block in the visit_ImportFrom method: the AST ImportFrom node always has a module attribute (it may be None for relative imports), so delete the try/except AttributeError and keep the existing if module is None early-return logic in visit_ImportFrom to ignore relative imports; update the method body to directly read node.module and proceed.
🤖 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/deps.py`:
- Around line 122-139: The functions parse_test_imports and parse_lib_imports
declare -> set[str] but actually return visitor.test_imports and
visitor.lib_imports (which are frozenset[str]); update the return type
annotations to -> frozenset[str] for both functions to match the actual return
values (or alternatively convert the frozensets to set(...) if you prefer a
mutable collection), and ensure the references to visitor.test_imports and
visitor.lib_imports remain unchanged.
- Around line 110-112: The code accesses node.args[0] without ensuring an
argument exists, which can raise IndexError for calls like support.findfile();
add a defensive bounds check before accessing node.args[0] (e.g. if not
node.args: return or if len(node.args) == 0: return) so the existing
isinstance(arg, ast.Constant) check remains safe; update the block that
currently reads "arg = node.args[0]" to first verify node.args is non-empty.
---
Nitpick comments:
In `@scripts/update_lib/deps.py`:
- Around line 66-74: Remove the dead try/except block in the visit_ImportFrom
method: the AST ImportFrom node always has a module attribute (it may be None
for relative imports), so delete the try/except AttributeError and keep the
existing if module is None early-return logic in visit_ImportFrom to ignore
relative imports; update the method body to directly read node.module and
proceed.
Comment on lines
+110
to
+112
| arg = node.args[0] | ||
| if not isinstance(arg, ast.Constant): | ||
| return |
Contributor
There was a problem hiding this comment.
Missing bounds check for node.args could raise IndexError.
If a malformed call like support.findfile() (no arguments) exists, accessing node.args[0] will raise an IndexError, potentially crashing the dependency scan for that file.
🛡️ Add defensive check
if (value.id != "support") or (func.attr != "findfile"):
return
+ if not node.args:
+ return
+
arg = node.args[0]
if not isinstance(arg, ast.Constant):
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 110 - 112, The code accesses
node.args[0] without ensuring an argument exists, which can raise IndexError for
calls like support.findfile(); add a defensive bounds check before accessing
node.args[0] (e.g. if not node.args: return or if len(node.args) == 0: return)
so the existing isinstance(arg, ast.Constant) check remains safe; update the
block that currently reads "arg = node.args[0]" to first verify node.args is
non-empty.
Comment on lines
+122
to
+139
| def parse_test_imports(content: str) -> set[str]: | ||
| """Parse test file content and extract test package dependencies.""" | ||
| if not (tree := safe_parse_ast(content)): | ||
| return set() | ||
|
|
||
| visitor = ImportVisitor() | ||
| visitor.visit(tree) | ||
| return visitor.test_imports | ||
|
|
||
|
|
||
| def parse_lib_imports(content: str) -> set[str]: | ||
| """Parse library file and extract all imported module names.""" | ||
| if not (tree := safe_parse_ast(content)): | ||
| return set() | ||
|
|
||
| visitor = ImportVisitor() | ||
| visitor.visit(tree) | ||
| return visitor.lib_imports |
Contributor
There was a problem hiding this comment.
Return type annotations don't match actual return types.
Both functions declare -> set[str] but return frozenset[str] from the visitor properties. This mismatch could cause type checker warnings and is misleading to callers.
🔧 Fix type annotations
-def parse_test_imports(content: str) -> set[str]:
+def parse_test_imports(content: str) -> frozenset[str]:
"""Parse test file content and extract test package dependencies."""
if not (tree := safe_parse_ast(content)):
- return set()
+ return frozenset()
visitor = ImportVisitor()
visitor.visit(tree)
return visitor.test_imports
-def parse_lib_imports(content: str) -> set[str]:
+def parse_lib_imports(content: str) -> frozenset[str]:
"""Parse library file and extract all imported module names."""
if not (tree := safe_parse_ast(content)):
- return set()
+ return frozenset()
visitor = ImportVisitor()
visitor.visit(tree)
return visitor.lib_imports🤖 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 122 - 139, The functions
parse_test_imports and parse_lib_imports declare -> set[str] but actually return
visitor.test_imports and visitor.lib_imports (which are frozenset[str]); update
the return type annotations to -> frozenset[str] for both functions to match the
actual return values (or alternatively convert the frozensets to set(...) if you
prefer a mutable collection), and ensure the references to visitor.test_imports
and visitor.lib_imports remain unchanged.
Contributor
|
Code has been automatically formatted The code in this PR has been formatted using:
git pull origin deps-nodevisitor |
ShaharNaveh
force-pushed
the
deps-nodevisitor
branch
from
e9703d5 to
50ca5c1
Compare
youknowone
approved these changes
Feb 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
This reverts commit a47572c.
Lets see what breaks
Summary by CodeRabbit
Bug Fixes
Refactor
Tests