-
Notifications
You must be signed in to change notification settings - Fork 1.4k
ast.NodeVisitor for import tracking
#7229
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -11,7 +11,6 @@ | |||||||||||||||||||||||||||||||||||||
| import difflib | ||||||||||||||||||||||||||||||||||||||
| import functools | ||||||||||||||||||||||||||||||||||||||
| import pathlib | ||||||||||||||||||||||||||||||||||||||
| import re | ||||||||||||||||||||||||||||||||||||||
| import shelve | ||||||||||||||||||||||||||||||||||||||
| import subprocess | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
|
|
@@ -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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Import parsing currently loses module roots and misclassifies At Lines 51-55, 💡 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}")
PYAlso applies to: 61-70 🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)
PYRepository: 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.pyRepository: RustPython/RustPython Length of output: 107
At line 96, indexing 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
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| 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 === | ||||||||||||||||||||||||||||||||||||||
|
|
@@ -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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| def count_todo_in_path(path: pathlib.Path) -> int: | ||||||||||||||||||||||||||||||||||||||
|
|
@@ -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 === | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
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:
Repository: RustPython/RustPython
Length of output: 1000
🏁 Script executed:
Repository: RustPython/RustPython
Length of output: 1932
🏁 Script executed:
Repository: RustPython/RustPython
Length of output: 144
🏁 Script executed:
Repository: RustPython/RustPython
Length of output: 964
🏁 Script executed:
Repository: RustPython/RustPython
Length of output: 611
🏁 Script executed:
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 chainedremoveprefixleaves a leading underscore:_test_atexit→_atexitinstead ofatexit. This causes subsequentlib_statuslookup 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
📝 Committable suggestion
🤖 Prompt for AI Agents