Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion scripts/update_lib/cmd_todo.py
Original file line number Diff line number Diff line change
Expand Up @@ -334,7 +334,7 @@ def compute_test_todo_list(
test_order = lib_test_order[lib_name].index(test_name)
else:
# Extract lib name from test name (test_foo -> foo)
lib_name = test_name.removeprefix("test_")
lib_name = test_name.removeprefix("test_").removeprefix("_test")
test_order = 0 # Default order for tests not in DEPENDENCIES

# Check if corresponding lib is up-to-date
Expand Down
138 changes: 92 additions & 46 deletions scripts/update_lib/deps.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
import difflib
import functools
import pathlib
import re
import shelve
import subprocess

Expand All @@ -32,62 +31,112 @@
# === Import parsing utilities ===


def _extract_top_level_code(content: str) -> str:
"""Extract only top-level code from Python content for faster parsing."""
def_idx = content.find("\ndef ")
class_idx = content.find("\nclass ")
class ImportVisitor(ast.NodeVisitor):
def __init__(self) -> None:
self.__imports = set()

indices = [i for i in (def_idx, class_idx) if i != -1]
if indices:
content = content[: min(indices)]
return content.rstrip("\n")
@property
def test_imports(self) -> frozenset[str]:
imports = set()
for module in self.__imports:
if not module.startswith("test."):
continue
name = module.removeprefix("test.")

if name == "support" or name.startswith("support."):
continue

_FROM_TEST_IMPORT_RE = re.compile(r"^from test import (.+)", re.MULTILINE)
_FROM_TEST_DOT_RE = re.compile(r"^from test\.(\w+)", re.MULTILINE)
_IMPORT_TEST_DOT_RE = re.compile(r"^import test\.(\w+)", re.MULTILINE)
imports.add(name)

return frozenset(imports)

def parse_test_imports(content: str) -> set[str]:
"""Parse test file content and extract test package dependencies."""
content = _extract_top_level_code(content)
imports = set()
@property
def lib_imports(self) -> frozenset[str]:
return frozenset(
# module.split(".", 1)[0]
module
for module in self.__imports
if not module.startswith("test.")
)

for match in _FROM_TEST_IMPORT_RE.finditer(content):
import_list = match.group(1)
for part in import_list.split(","):
name = part.split()[0].strip()
if name and name not in ("support", "__init__"):
imports.add(name)
def visit_Import(self, node):
for alias in node.names:
self.__imports.add(alias.name)

for match in _FROM_TEST_DOT_RE.finditer(content):
dep = match.group(1)
if dep not in ("support", "__init__"):
imports.add(dep)
def visit_ImportFrom(self, node):
try:
module = node.module
except AttributeError:
# Ignore `from . import my_internal_module`
return

for match in _IMPORT_TEST_DOT_RE.finditer(content):
dep = match.group(1)
if dep not in ("support", "__init__"):
imports.add(dep)
if module is None: # Ignore `from . import my_internal_module`
return

return imports
for alias in node.names:
# We only care about what we import if it was from the "test" module
if module == "test":
name = f"{module}.{alias.name}"
else:
name = module

self.__imports.add(name)

_IMPORT_RE = re.compile(r"^import\s+(\w[\w.]*)", re.MULTILINE)
_FROM_IMPORT_RE = re.compile(r"^from\s+(\w[\w.]*)\s+import", re.MULTILINE)
def visit_Call(self, node) -> None:
"""
In test files, there's sometimes use of:

```python
import test.support
from test.support import script_helper

def parse_lib_imports(content: str) -> set[str]:
"""Parse library file and extract all imported module names."""
imports = set()
script = support.findfile("_test_atexit.py")
script_helper.run_test_script(script)
```

This imports "_test_atexit.py" but does not show as an import node.
"""
func = node.func
if not isinstance(func, ast.Attribute):
return

value = func.value
if not isinstance(value, ast.Name):
return

if (value.id != "support") or (func.attr != "findfile"):
return

arg = node.args[0]
if not isinstance(arg, ast.Constant):
return
Comment on lines +110 to +112

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.


for match in _IMPORT_RE.finditer(content):
imports.add(match.group(1))
>
if not target.endswith(".py"):
return

for match in _FROM_IMPORT_RE.finditer(content):
imports.add(match.group(1))
>
self.add_import(f"test.{target}")
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated

return imports

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
Comment on lines +122 to +139

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.



# === TODO marker utilities ===
Expand All @@ -104,7 +153,7 @@ def filter_rustpython_todo(content: str) -> str:

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)


def count_todo_in_path(path: pathlib.Path) -> int:
Expand All @@ -113,10 +162,7 @@ def count_todo_in_path(path: pathlib.Path) -> int:
content = safe_read_text(path)
return count_rustpython_todo(content) if content else 0

total = 0
for _, content in read_python_files(path):
total += count_rustpython_todo(content)
return total
return sum(count_rustpython_todo(content) for _, content in read_python_files(path))


# === Test utilities ===
Expand Down