fix: review follow-ups for #5 and the Swift lexical preview #8
2 changed files with 131 additions and 1 deletions
fix(python): resolve relative, package and dotted imports to the right file
Since #5 Python calls resolve only through import edges, but the lexical importer could not produce those edges for the common forms: - `from .util import helper` never matched (the regex needed a leading letter), so the call went unresolved; - `from pkg import f` looked up the stem "pkg" instead of pkg/__init__.py; - `from x.common import f` failed when y/common.py also existed, and `from other.common import f` (a module the repository does not have) resolved to y/common.py by basename. Imports now resolve Python-specifically: leading dots are relative to the importing package, and a dotted name maps to `a/b.py` or `a/b/__init__.py` from the repository root or `src/`. Only a single-component name (`import foo`) may still fall back to a unique module basename; a dotted name never does. `from . import x` stays unresolved (the module is in the import list, not the name). Tests (both failed before): relative, package and duplicate-stem imports resolve their calls to pkg/util.py, pkg/__init__.py and x/common.py; an unknown dotted module does not bind to y/common.py.
commit
67334bec59
52
src/scan.rs
52
src/scan.rs
|
|
@ -2208,6 +2208,55 @@ impl ScanState {
|
|||
(candidates.len() == 1).then(|| candidates[0].clone())
|
||||
}
|
||||
|
||||
/// A Python module name as a repository file. Leading dots are relative to the
|
||||
/// importing package; a dotted name maps to `a/b.py` or `a/b/__init__.py` from the
|
||||
/// repository root or `src/`. Only a single-component name (`import foo`) may
|
||||
/// fall back to a unique module basename elsewhere; a dotted name never does,
|
||||
/// since `x.common` is not just any `common.py`.
|
||||
fn resolve_python_import(&self, name: &str, directory: &str) -> Option<String> {
|
||||
let level = name.chars().take_while(|ch| *ch == '.').count();
|
||||
let module = &name[level..];
|
||||
if module.is_empty() {
|
||||
// `from . import x`: the module is in the import list, not the name.
|
||||
return None;
|
||||
}
|
||||
let relative = module.replace('.', "/");
|
||||
let bases: Vec<String> = if level > 0 {
|
||||
let mut base = Path::new(directory).to_path_buf();
|
||||
for _ in 1..level {
|
||||
base = base.parent()?.to_path_buf();
|
||||
}
|
||||
vec![base.to_str()?.to_owned()]
|
||||
} else {
|
||||
vec![".".into(), "src".into()]
|
||||
};
|
||||
for base in &bases {
|
||||
let base = if base.is_empty() { "." } else { base.as_str() };
|
||||
for candidate in [
|
||||
format!("{base}/{relative}.py"),
|
||||
format!("{base}/{relative}/__init__.py"),
|
||||
] {
|
||||
if let Some(path) = normalize_import_path(&candidate)
|
||||
&& let Some(node) = self.node(&stable_id("file", &path)).filter(|n| {
|
||||
n.kind == NodeKind::File && n.language.as_deref() == Some("python")
|
||||
})
|
||||
{
|
||||
return Some(node.id.clone());
|
||||
}
|
||||
}
|
||||
}
|
||||
if level == 0 && !module.contains('.') {
|
||||
for key in import_keys(module, "python") {
|
||||
if let Some(ids) = self.files_by_key.get(&key)
|
||||
&& ids.len() == 1
|
||||
{
|
||||
return Some(ids[0].clone());
|
||||
}
|
||||
}
|
||||
}
|
||||
None
|
||||
}
|
||||
|
||||
fn resolve_import(&self, name: &str, language: &str, source_path: &str) -> Option<String> {
|
||||
let directory = Path::new(source_path).parent()?.to_str()?;
|
||||
let directory = if directory.is_empty() { "." } else { directory };
|
||||
|
|
@ -2267,6 +2316,7 @@ impl ScanState {
|
|||
}
|
||||
}
|
||||
"rust" => paths.extend(rust_import_paths(name, directory)?),
|
||||
"python" => return self.resolve_python_import(name, directory),
|
||||
_ => {
|
||||
for key in import_keys(name, language) {
|
||||
if let Some(ids) = self.files_by_key.get(&key)
|
||||
|
|
@ -2485,7 +2535,7 @@ impl Patterns {
|
|||
"python" => (
|
||||
r"^\s*class\s+(?P<name>[A-Za-z_]\w*)(?:\((?P<base>[A-Za-z_][\w.]*)\))?",
|
||||
r"^\s*(?:async\s+)?def\s+(?P<name>[A-Za-z_]\w*)\s*\(",
|
||||
r"^\s*(?:from\s+(?P<name>[A-Za-z_][\w.]*)\s+import|import\s+(?P<name2>[A-Za-z_][\w.]*))",
|
||||
r"^\s*(?:from\s+(?P<name>\.*[A-Za-z_][\w.]*|\.+)\s+import|import\s+(?P<name2>[A-Za-z_][\w.]*))",
|
||||
),
|
||||
_ => (
|
||||
r"^\s*(?:class|struct)\s+(?P<name>[A-Za-z_]\w*)(?:\s*:\s*(?:public\s+|protected\s+|private\s+)?(?P<base>[A-Za-z_:]\w*(?:::\w+)*))?",
|
||||
|
|
|
|||
80
tests/python_imports.rs
Normal file
80
tests/python_imports.rs
Normal file
|
|
@ -0,0 +1,80 @@
|
|||
use brick::model::{Building, EdgeKind};
|
||||
use brick::scan;
|
||||
use std::fs;
|
||||
|
||||
fn scan_files(files: &[(&str, &str)]) -> Building {
|
||||
let root = tempfile::tempdir().unwrap();
|
||||
for (path, content) in files {
|
||||
let file = root.path().join(path);
|
||||
fs::create_dir_all(file.parent().unwrap()).unwrap();
|
||||
fs::write(file, content).unwrap();
|
||||
}
|
||||
scan::scan(root.path()).unwrap()
|
||||
}
|
||||
|
||||
/// (caller name, target path) for every resolved call.
|
||||
fn resolved_calls(model: &Building) -> Vec<(String, String)> {
|
||||
model
|
||||
.edges
|
||||
.iter()
|
||||
.filter(|edge| edge.kind == EdgeKind::Calls)
|
||||
.filter_map(|edge| {
|
||||
let source = model.nodes.iter().find(|n| n.id == edge.source)?;
|
||||
let target = model.nodes.iter().find(|n| n.id == edge.target)?;
|
||||
Some((source.name.clone(), target.path.clone()?))
|
||||
})
|
||||
.collect()
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn relative_package_and_qualified_python_imports_resolve_to_the_right_file() {
|
||||
// Python calls resolve only through import edges, and the lexical importer
|
||||
// produced none for relative imports (the regex needed a leading letter),
|
||||
// for packages (`from pkg import f` looked up the stem "pkg", not
|
||||
// pkg/__init__.py), or when two files shared a stem.
|
||||
let model = scan_files(&[
|
||||
("pkg/__init__.py", "def from_package():\n return 1\n"),
|
||||
("pkg/util.py", "def helper():\n return 1\n"),
|
||||
(
|
||||
"pkg/rel.py",
|
||||
"from .util import helper\n\ndef use_relative():\n return helper()\n",
|
||||
),
|
||||
("x/common.py", "def only_here():\n return 1\n"),
|
||||
("y/common.py", "def elsewhere():\n return 2\n"),
|
||||
(
|
||||
"main.py",
|
||||
"from x.common import only_here\nfrom pkg import from_package\n\ndef run():\n only_here()\n return from_package()\n",
|
||||
),
|
||||
]);
|
||||
let calls = resolved_calls(&model);
|
||||
for (caller, target) in [
|
||||
("use_relative", "pkg/util.py"),
|
||||
("run", "x/common.py"),
|
||||
("run", "pkg/__init__.py"),
|
||||
] {
|
||||
assert!(
|
||||
calls
|
||||
.iter()
|
||||
.any(|(source, path)| source == caller && path == target),
|
||||
"{caller} -> {target} missing: {calls:?}"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn an_unknown_dotted_python_module_does_not_fall_back_to_a_basename() {
|
||||
// `from other.common import f` names a module this repository does not have;
|
||||
// matching the stem "common" to y/common.py would invent a dependency.
|
||||
let model = scan_files(&[
|
||||
("y/common.py", "def f():\n return 2\n"),
|
||||
(
|
||||
"main.py",
|
||||
"from other.common import f\n\ndef run():\n return f()\n",
|
||||
),
|
||||
]);
|
||||
let calls = resolved_calls(&model);
|
||||
assert!(
|
||||
!calls.iter().any(|(_, path)| path == "y/common.py"),
|
||||
"{calls:?}"
|
||||
);
|
||||
}
|
||||
Loading…
Add table
Add a link
Reference in a new issue