fix/audit-findings #5
2 changed files with 138 additions and 8 deletions
fix(python,swift): keep calls in their function and resolve only visible names
Three lexical-preview bugs, reproduced on one probe: - A blank or comment-only line in a Python function fell through to the brace-depth scope rule, which popped every open scope, so every call after the first blank line in a body was attributed to the file. Blank/comment lines now leave Python scopes unchanged. - `class Child(Base):` matched the call pattern and produced a call edge from the file to Child. The declared class name is no longer a call on its own header line. - Calls resolved to the only function with that name anywhere in the repository, imported or not (a NameError at runtime became an edge). Resolution now prefers the caller's file; Python then considers only files the caller imports (resolved import edges, cached per file); otherwise the call stays external. Swift keeps the module-wide unique name after the same-file check, because files of one Swift module see each other without imports. Imported bases resolve the same way, so `from pkg.b import Base` disambiguates `class Child(Base)`. Test: languages::python_calls_keep_their_function_and_resolve_only_what_is_visible fails before this change (go() had no call edges at all).
commit
4bdae55923
80
src/scan.rs
80
src/scan.rs
|
|
@ -246,6 +246,8 @@ struct ScanState {
|
||||||
rust: RustIndex,
|
rust: RustIndex,
|
||||||
/// Language -> identifiers used outside call/declaration-name positions.
|
/// Language -> identifiers used outside call/declaration-name positions.
|
||||||
references: HashMap<String, HashSet<String>>,
|
references: HashMap<String, HashSet<String>>,
|
||||||
|
/// Lexical-language file -> repository files it imports (resolved edges).
|
||||||
|
imported_paths: HashMap<String, Rc<HashSet<String>>>,
|
||||||
}
|
}
|
||||||
|
|
||||||
impl ScanState {
|
impl ScanState {
|
||||||
|
|
@ -268,6 +270,7 @@ impl ScanState {
|
||||||
cpp: CppIndex::default(),
|
cpp: CppIndex::default(),
|
||||||
rust: RustIndex::default(),
|
rust: RustIndex::default(),
|
||||||
references: HashMap::new(),
|
references: HashMap::new(),
|
||||||
|
imported_paths: HashMap::new(),
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
@ -421,9 +424,14 @@ impl ScanState {
|
||||||
let line = strip_line_comment(raw_line, language);
|
let line = strip_line_comment(raw_line, language);
|
||||||
let call_line = mask_quoted_strings(line);
|
let call_line = mask_quoted_strings(line);
|
||||||
let indent = raw_line.len() - raw_line.trim_start().len();
|
let indent = raw_line.len() - raw_line.trim_start().len();
|
||||||
if language == "python" && !line.trim().is_empty() {
|
if language == "python" {
|
||||||
while scopes.last().is_some_and(|scope| scope.indent >= indent) {
|
// Blank and comment-only lines say nothing about Python scope. They
|
||||||
scopes.pop();
|
// used to fall through to the brace-depth rule below, which popped
|
||||||
|
// every open scope: calls after a blank line left their function.
|
||||||
|
if !line.trim().is_empty() {
|
||||||
|
while scopes.last().is_some_and(|scope| scope.indent >= indent) {
|
||||||
|
scopes.pop();
|
||||||
|
}
|
||||||
}
|
}
|
||||||
} else {
|
} else {
|
||||||
while scopes
|
while scopes
|
||||||
|
|
@ -434,9 +442,11 @@ impl ScanState {
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
let mut class_name = None;
|
||||||
if let Some(captures) = patterns.class.captures(line)
|
if let Some(captures) = patterns.class.captures(line)
|
||||||
&& let Some(name) = captures.name("name")
|
&& let Some(name) = captures.name("name")
|
||||||
{
|
{
|
||||||
|
class_name = Some(name.as_str().to_owned());
|
||||||
let owner_id = scopes
|
let owner_id = scopes
|
||||||
.last()
|
.last()
|
||||||
.map(|scope| scope.id.clone())
|
.map(|scope| scope.id.clone())
|
||||||
|
|
@ -538,7 +548,11 @@ impl ScanState {
|
||||||
let target = target_match.as_str();
|
let target = target_match.as_str();
|
||||||
let prefix = call_line[..target_match.start()].trim_end();
|
let prefix = call_line[..target_match.start()].trim_end();
|
||||||
let member_call = prefix.ends_with('.') || prefix.ends_with("->");
|
let member_call = prefix.ends_with('.') || prefix.ends_with("->");
|
||||||
if definition_name.as_deref() != Some(target) && !is_keyword(target) {
|
// `class Child(Base):` names the class being declared, not a call.
|
||||||
|
if definition_name.as_deref() != Some(target)
|
||||||
|
&& class_name.as_deref() != Some(target)
|
||||||
|
&& !is_keyword(target)
|
||||||
|
{
|
||||||
self.pending.push(PendingRelationship {
|
self.pending.push(PendingRelationship {
|
||||||
source: source.clone(),
|
source: source.clone(),
|
||||||
target_name: clean_target(target),
|
target_name: clean_target(target),
|
||||||
|
|
@ -1256,6 +1270,32 @@ impl ScanState {
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Repository files that `path` imports through already-resolved edges.
|
||||||
|
/// Imports resolve before calls, so the cached set is complete once built.
|
||||||
|
fn imported_paths(&mut self, path: &str) -> Rc<HashSet<String>> {
|
||||||
|
if let Some(found) = self.imported_paths.get(path) {
|
||||||
|
return Rc::clone(found);
|
||||||
|
}
|
||||||
|
let paths: HashSet<String> = self
|
||||||
|
.nodes
|
||||||
|
.iter()
|
||||||
|
.find(|n| n.kind == NodeKind::File && n.path.as_deref() == Some(path))
|
||||||
|
.map(|file| {
|
||||||
|
self.edges
|
||||||
|
.iter()
|
||||||
|
.filter(|e| e.kind == EdgeKind::Imports && e.source == file.id)
|
||||||
|
.filter_map(|e| self.node(&e.target))
|
||||||
|
.filter(|n| n.kind == NodeKind::File)
|
||||||
|
.filter_map(|n| n.path.clone())
|
||||||
|
.collect()
|
||||||
|
})
|
||||||
|
.unwrap_or_default();
|
||||||
|
let paths = Rc::new(paths);
|
||||||
|
self.imported_paths
|
||||||
|
.insert(path.to_owned(), Rc::clone(&paths));
|
||||||
|
paths
|
||||||
|
}
|
||||||
|
|
||||||
/// Mark functions that may be used without a resolvable call: named outside
|
/// Mark functions that may be used without a resolvable call: named outside
|
||||||
/// a call position anywhere in the language, or C++ virtual via a prototype.
|
/// a call position anywhere in the language, or C++ virtual via a prototype.
|
||||||
fn annotate_indirect_uses(&mut self) {
|
fn annotate_indirect_uses(&mut self) {
|
||||||
|
|
@ -1334,6 +1374,11 @@ impl ScanState {
|
||||||
if let Some(id) = self.resolve_qualified_symbol(relationship) {
|
if let Some(id) = self.resolve_qualified_symbol(relationship) {
|
||||||
return (id, None, None);
|
return (id, None, None);
|
||||||
}
|
}
|
||||||
|
let imported = if language == "python" {
|
||||||
|
self.imported_paths(&relationship.path)
|
||||||
|
} else {
|
||||||
|
Rc::default()
|
||||||
|
};
|
||||||
let candidates: Vec<_> = self
|
let candidates: Vec<_> = self
|
||||||
.symbols_by_name
|
.symbols_by_name
|
||||||
.get(name)
|
.get(name)
|
||||||
|
|
@ -1369,10 +1414,29 @@ impl ScanState {
|
||||||
if let Some(id) = self.resolve_imported_symbol(relationship) {
|
if let Some(id) = self.resolve_imported_symbol(relationship) {
|
||||||
return (id, None, None);
|
return (id, None, None);
|
||||||
}
|
}
|
||||||
} else if candidates.len() == 1 {
|
} else {
|
||||||
return (candidates[0].id.clone(), None, None);
|
// Lexical languages: the caller's own file first, then (Python)
|
||||||
} else if candidates.len() > 1 {
|
// only files it imports. Swift files in one module see each other
|
||||||
reason = "ambiguous";
|
// without imports, so Swift keeps a module-wide unique name.
|
||||||
|
let same_file: Vec<_> = candidates
|
||||||
|
.iter()
|
||||||
|
.filter(|n| n.path.as_deref() == Some(&relationship.path))
|
||||||
|
.collect();
|
||||||
|
let visible: Vec<_> = if !same_file.is_empty() {
|
||||||
|
same_file
|
||||||
|
} else if language == "python" {
|
||||||
|
candidates
|
||||||
|
.iter()
|
||||||
|
.filter(|n| n.path.as_ref().is_some_and(|p| imported.contains(p)))
|
||||||
|
.collect()
|
||||||
|
} else {
|
||||||
|
candidates.iter().collect()
|
||||||
|
};
|
||||||
|
match visible.as_slice() {
|
||||||
|
[only] => return (only.id.clone(), None, None),
|
||||||
|
[] => {}
|
||||||
|
_ => reason = "ambiguous",
|
||||||
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
if !relationship.member_call
|
if !relationship.member_call
|
||||||
|
|
|
||||||
|
|
@ -582,3 +582,69 @@ fn generic_type_arguments_are_not_base_classes() {
|
||||||
["Base"]
|
["Base"]
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn python_calls_keep_their_function_and_resolve_only_what_is_visible() {
|
||||||
|
let (_, model) = project(&[
|
||||||
|
(
|
||||||
|
"pkg/a.py",
|
||||||
|
"def process(x):\n return x\n\ndef unrelated():\n pass\n",
|
||||||
|
),
|
||||||
|
(
|
||||||
|
"pkg/b.py",
|
||||||
|
"class Base:\n pass\n\ndef helper():\n pass\n",
|
||||||
|
),
|
||||||
|
(
|
||||||
|
"pkg/c.py",
|
||||||
|
// A blank line and a comment inside the body used to pop the function
|
||||||
|
// scope, attributing the later calls to the file.
|
||||||
|
"from pkg.b import Base\n\nclass Child(Base):\n def go(self):\n x = 1\n\n # comment\n helper()\n process(1)\n unrelated()\n",
|
||||||
|
),
|
||||||
|
(
|
||||||
|
"pkg/d.py",
|
||||||
|
"from pkg.a import process\n\ndef use():\n return process(2)\n",
|
||||||
|
),
|
||||||
|
]);
|
||||||
|
let go = symbol(&model, "pkg/c.py", "go");
|
||||||
|
let outgoing = |source: &Node| {
|
||||||
|
let mut found: Vec<(String, String)> = model
|
||||||
|
.edges
|
||||||
|
.iter()
|
||||||
|
.filter(|e| e.kind == EdgeKind::Calls && e.source == source.id)
|
||||||
|
.map(|e| {
|
||||||
|
let target = model.nodes.iter().find(|n| n.id == e.target).unwrap();
|
||||||
|
(target.name.clone(), target.path.clone().unwrap_or_default())
|
||||||
|
})
|
||||||
|
.collect();
|
||||||
|
found.sort();
|
||||||
|
found
|
||||||
|
};
|
||||||
|
// helper comes from the imported pkg/b.py; process and unrelated are never
|
||||||
|
// imported by c.py, so they stay external instead of resolving by unique name.
|
||||||
|
assert_eq!(
|
||||||
|
outgoing(go),
|
||||||
|
[
|
||||||
|
("helper".to_owned(), "pkg/b.py".to_owned()),
|
||||||
|
("process".to_owned(), String::new()),
|
||||||
|
("unrelated".to_owned(), String::new()),
|
||||||
|
]
|
||||||
|
);
|
||||||
|
// An imported name still resolves across files.
|
||||||
|
assert_eq!(
|
||||||
|
outgoing(symbol(&model, "pkg/d.py", "use")),
|
||||||
|
[("process".to_owned(), "pkg/a.py".to_owned())]
|
||||||
|
);
|
||||||
|
// A class header is not a call.
|
||||||
|
let file_c = model
|
||||||
|
.nodes
|
||||||
|
.iter()
|
||||||
|
.find(|n| n.kind == NodeKind::File && n.path.as_deref() == Some("pkg/c.py"))
|
||||||
|
.unwrap();
|
||||||
|
assert!(
|
||||||
|
!model
|
||||||
|
.edges
|
||||||
|
.iter()
|
||||||
|
.any(|e| e.kind == EdgeKind::Calls && e.source == file_c.id),
|
||||||
|
"file-level call edges in pkg/c.py"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue