Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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")
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 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).

test_order = 0 # Default order for tests not in DEPENDENCIES

# Check if corresponding lib is up-to-date
Expand Down
124 changes: 78 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,98 @@
# === 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.test_imports = set()
self.lib_imports = set()

indices = [i for i in (def_idx, class_idx) if i != -1]
if indices:
content = content[: min(indices)]
return content.rstrip("\n")
def add_import(self, name: str) -> None:
"""
Add an `import` to its correct slot (`test_imports` or `lib_imports`).

Parameters
----------
name : str
Module name.
"""
if name.startswith("test.support"):
return

_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)
real_name = name.split(".", 1)[-1]
if name.startswith("test."):
self.test_imports.add(real_name)
else:
self.lib_imports.add(real_name)
Comment on lines +51 to +55
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

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.


def visit_Import(self, node):
for alias in node.names:
self.add_import(alias.name)

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()
def visit_ImportFrom(self, node):
try:
module = node.module
except AttributeError:
# Ignore `from . import my_internal_module`
return

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)
for name in node.names:
self.add_import(f"{module}.{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_Call(self, node) -> None:
"""
In test files, there's sometimes use of:

for match in _IMPORT_TEST_DOT_RE.finditer(content):
dep = match.group(1)
if dep not in ("support", "__init__"):
imports.add(dep)
```python
import test.support
from test.support import script_helper

return imports
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

_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)
value = func.value
if not isinstance(value, ast.Name):
return

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

def parse_lib_imports(content: str) -> set[str]:
"""Parse library file and extract all imported module names."""
imports = set()
arg = node.args[0]
if not isinstance(arg, ast.Constant):
return

target = arg.value
if not target.endswith(".py"):
return
Comment on lines +96 to +102
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 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.


for match in _IMPORT_RE.finditer(content):
imports.add(match.group(1))
target = target.removesuffix(".py")
self.add_import(f"test.{target}")

for match in _FROM_IMPORT_RE.finditer(content):
imports.add(match.group(1))

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


# === TODO marker utilities ===
Expand All @@ -104,7 +139,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)
Comment on lines 140 to +142
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

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.



def count_todo_in_path(path: pathlib.Path) -> int:
Expand All @@ -113,10 +148,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