Skip to content

Commit c29570f

Browse files
committed
fix(impact): removed definitions from the grammar, base vs head
A removed definition is one the file declares at the base and no longer declares at the head, read through parsers::declarations: the named declarations a grammar sees (functions, methods, classes, interfaces, member signatures), a function's locals left out, each with its container. Read off removed lines by regex, a Rust tail expression `Err("zero")` became a removed member `Err` whose callers were every `Err(` in the crate, a bare `int(text)` in a Python method a removed `int`, and a private fn beside an `impl` a removed API. A member is as public as its real container. Deleted: REMOVED_DECLARATION, REMOVED_MEMBER, member_declared, MEMBER_MODIFIERS, NOT_MEMBER_NAMES, declared_name and the nearest-class-above guess (production +183/-176, tests +57). Gate: only_declarations_the_head_lost_are_removed_definitions; the pre-change binary answers ["int", "Err", "shown"], this one ["shown"]. Fixes #515 Refs #458 Refs #506 Claude-Session: https://claude.ai/code/session_01Fzbq9adLJyZGLxqHrCiW29
1 parent c5f6f3c commit c29570f

5 files changed

Lines changed: 247 additions & 176 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -100,6 +100,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
100100
file's grammar, interpolations kept, in every language with one; the
101101
lexer and its special cases (`//` as floor division, a Rust lifetime,
102102
`*` block-comment lines) are gone (#506).
103+
- A removed definition is one the file declared at the base and does not
104+
declare at the head, both read off the grammar. Read off removed lines by
105+
regex, a Rust tail expression `Err("zero")` became a removed member named
106+
`Err` with every `Err(` in the crate as its callers, a bare `int(text)` in
107+
a Python method a removed `int`, and a private function next to an
108+
`impl` a removed API (#515, #458). A member is as public as the class or
109+
interface it sits in, by nesting, not by the nearest class above it.
103110
- A dependency bump is a change: impact names the lock files a range
104111
changes (`lockfile_changes`) and never answers "No changes to analyse" for
105112
them (#495).

‎crates/diffctx-native/src/impact.rs‎

Lines changed: 83 additions & 175 deletions
Original file line numberDiff line numberDiff line change
@@ -2393,10 +2393,16 @@ pub fn build_impact(
23932393
})
23942394
};
23952395
let kotlin_removed = removed_kotlin(state, &kotlin, &base, head_rev.as_deref());
2396-
for (sym, contract) in
2397-
removed_definitions(state, &defined, head_rev.as_deref(), &in_a_hunk, &binds_to)
2398-
.into_iter()
2399-
.chain(kotlin_removed)
2396+
for (sym, contract) in removed_definitions(
2397+
state,
2398+
&defined,
2399+
&base,
2400+
head_rev.as_deref(),
2401+
&in_a_hunk,
2402+
&binds_to,
2403+
)
2404+
.into_iter()
2405+
.chain(kotlin_removed)
24002406
{
24012407
if let Some(c) = contract {
24022408
contracts.push(c);
@@ -2521,101 +2527,12 @@ pub fn build_impact(
25212527
}
25222528
}
25232529

2524-
/// A declaration a removed line spells: `function f(`, `def f(`, `fn f(`,
2525-
/// `class C`, `interface I`, `func (r T) f(`, `def self.f`, `const f = () =>`.
2526-
static REMOVED_DECLARATION: once_cell::sync::Lazy<regex::Regex> = once_cell::sync::Lazy::new(
2527-
|| {
2528-
regex::Regex::new(
2529-
r"^\s*((?:(?:export|default|pub(?:\([^)]*\))?|public|private|protected|static|async|abstract|readonly|override|declare)\s+)*)(?:(?:function\*?|def|fn|func|class|interface|type|enum|struct|trait)\s+(?:\([^)]*\)\s*)?(?:self\.)?([A-Za-z_$][\w$]*)|(?:const|let|var)\s+([A-Za-z_$][\w$]*)\s*(?::[^=]+)?=\s*(?:async\s+)?(?:function\b|\([^)]*\)\s*(?::[^=]*)?=>|[A-Za-z_$][\w$]*\s*=>))",
2530-
)
2531-
.expect("removed declaration regex")
2532-
},
2533-
);
2534-
2535-
/// A member a removed line declares inside a class or an interface:
2536-
/// `m?(): void;`, `m(x: T) {`, `async m() {`.
2537-
static REMOVED_MEMBER: once_cell::sync::Lazy<regex::Regex> = once_cell::sync::Lazy::new(|| {
2538-
regex::Regex::new(
2539-
r"^\s*(?:(?:public|private|protected|static|async|abstract|readonly|override|get|set)\s+)*([A-Za-z_$][\w$]*)\??\s*(?:<[^>]*>)?\s*\([^)]*\)\s*(?::[^;{=]*)?\s*[;{]?\s*$",
2540-
)
2541-
.expect("removed member regex")
2542-
});
2543-
2544-
/// The member a class-body line declares. `notify(x);` with no modifier
2545-
/// and no return type is a call statement inside a method body, not a
2546-
/// declaration: read as one, deleting the call reported a removed API.
2547-
fn member_declared(line: &str) -> Option<&str> {
2548-
let name = REMOVED_MEMBER.captures(line)?.get(1)?.as_str();
2549-
let code = line.trim();
2550-
let modified = code
2551-
.split_whitespace()
2552-
.next()
2553-
.is_some_and(|w| MEMBER_MODIFIERS.contains(&w));
2554-
let call = code
2555-
.strip_suffix(';')
2556-
.is_some_and(|c| c.trim_end().ends_with(')'));
2557-
(modified || !call).then_some(name)
2558-
}
2559-
2560-
const MEMBER_MODIFIERS: &[&str] = &[
2561-
"public",
2562-
"private",
2563-
"protected",
2564-
"static",
2565-
"async",
2566-
"abstract",
2567-
"readonly",
2568-
"override",
2569-
"declare",
2570-
"get",
2571-
"set",
2572-
];
2573-
25742530
/// Directory names every project has: naming one says nothing about which.
25752531
const GENERIC_DIRECTORIES: &[&str] = &[
25762532
"src", "main", "java", "kotlin", "scala", "lib", "test", "tests", "app", "pkg", "internal",
25772533
"cmd", "include", "source", "sources",
25782534
];
25792535

2580-
// Every supported language's `keyword (…) {`: a control statement removed
2581-
// from a body reads like a method header.
2582-
const NOT_MEMBER_NAMES: &[&str] = &[
2583-
"if",
2584-
"for",
2585-
"while",
2586-
"switch",
2587-
"catch",
2588-
"match",
2589-
"when",
2590-
"try",
2591-
"synchronized",
2592-
"using",
2593-
"lock",
2594-
"fixed",
2595-
"foreach",
2596-
"elseif",
2597-
"return",
2598-
"function",
2599-
"super",
2600-
"constructor",
2601-
"await",
2602-
"new",
2603-
"typeof",
2604-
"self",
2605-
"cls",
2606-
"this",
2607-
];
2608-
2609-
fn declared_name(line: &str) -> Option<(&str, bool)> {
2610-
let c = REMOVED_DECLARATION.captures(line)?;
2611-
let prefix = c.get(1).map_or("", |m| m.as_str());
2612-
let name = c.get(2).or_else(|| c.get(3))?.as_str();
2613-
Some((
2614-
name,
2615-
prefix.contains("export") || prefix.contains("pub ") || prefix.trim() == "pub",
2616-
))
2617-
}
2618-
26192536
/// Definitions the change removed and nothing at the head defines again,
26202537
/// with the lines outside the diff that still name them (#367). They have
26212538
/// no fragment at the head, so the callers graph never reaches them: an
@@ -2772,90 +2689,85 @@ fn removed_kotlin(
27722689
fn removed_definitions(
27732690
state: &ScoredState,
27742691
defined: &FxHashMap<&str, FxHashSet<String>>,
2692+
base: &str,
27752693
head: Option<&str>,
27762694
in_a_hunk: &dyn Fn(&FragmentId) -> bool,
27772695
binds_to: &dyn Fn(&str, &str, &str, &str) -> bool,
27782696
) -> Vec<(ChangedSymbol, Option<Contract>)> {
2779-
let mut removed: Vec<(String, &crate::change_class::HunkText, bool)> = Vec::new();
2697+
// Read off the grammar, the file at the base against the file at the
2698+
// head: a removed definition is one the base declared and the head does
2699+
// not. Read off removed lines by regex, a match arm became a removed
2700+
// member named `Err`, a call statement a removed API, and an indented
2701+
// line of a free function a member of the nearest class above it (#515).
2702+
let old_path_of: FxHashMap<&str, &str> = state
2703+
.renamed_files
2704+
.iter()
2705+
.map(|(old, new)| (new.as_str(), old.as_str()))
2706+
.collect();
2707+
let mut seen: FxHashSet<&str> = FxHashSet::default();
2708+
// (name, path, its lines at the base, exported)
2709+
let mut removed: Vec<(String, String, String, bool)> = Vec::new();
27802710
for h in &state.hunk_texts {
2781-
// Kotlin's removals come from its facts (`removed_kotlin`): a call
2782-
// statement there ends with no `;` and read as a declaration.
2783-
if !is_code_file(&h.path) || is_kotlin(&h.path) {
2711+
// Kotlin's removals come from its facts (`removed_kotlin`).
2712+
if h.removed.is_empty()
2713+
|| !is_code_file(&h.path)
2714+
|| is_kotlin(&h.path)
2715+
|| !seen.insert(h.path.as_str())
2716+
{
27842717
continue;
27852718
}
2786-
let (anchor, _) = h.new_range();
2787-
let classes: Vec<&Fragment> = state
2788-
.all_fragments
2789-
.iter()
2790-
.filter(|f| {
2791-
f.kind.is_container() && f.start_line() <= anchor && rel(state, f.path()) == h.path
2792-
})
2793-
.collect();
2794-
// As in `enclosing_class`: a class may be fragmented as its header
2795-
// alone, so an indented line belongs to the nearest shallower class
2796-
// above it.
2797-
let container_of = |line: &str| {
2798-
let own = indent_of(line);
2799-
classes
2800-
.iter()
2801-
.copied()
2802-
.filter(|c| c.end_line() >= anchor)
2803-
.min_by_key(|c| c.line_count())
2804-
.or_else(|| {
2805-
(own > 0)
2806-
.then(|| {
2807-
classes
2808-
.iter()
2809-
.copied()
2810-
.filter(|c| indent_of(&c.content) < own)
2811-
.max_by_key(|c| c.start_line())
2812-
})
2813-
.flatten()
2719+
let old_path = old_path_of.get(h.path.as_str()).copied().unwrap_or(&h.path);
2720+
let before = read_or_note(
2721+
state,
2722+
crate::git::show_file_at_revision(&state.root_dir, base, Path::new(old_path)),
2723+
);
2724+
let (Some(at_base), Some(at_head)) = (
2725+
crate::parsers::declarations(old_path, &before),
2726+
crate::parsers::declarations(
2727+
&h.path,
2728+
&state
2729+
.run
2730+
.source()
2731+
.read_to_string(&state.root_dir.join(&h.path))
2732+
.unwrap_or_default(),
2733+
),
2734+
) else {
2735+
continue;
2736+
};
2737+
let still: FxHashSet<&str> = at_head.iter().map(|d| d.name.as_str()).collect();
2738+
let public = |d: &crate::parsers::Declaration| {
2739+
let head = d.head.trim_start();
2740+
is_public(old_path, Some(&d.name), head)
2741+
|| d.container.is_some_and(|c| {
2742+
!head.starts_with("private")
2743+
&& !head.starts_with("protected")
2744+
&& !head.starts_with('#')
2745+
&& is_public(
2746+
old_path,
2747+
Some(&at_base[c].name),
2748+
at_base[c].head.trim_start(),
2749+
)
28142750
})
28152751
};
2816-
let readded: FxHashSet<String> = h
2817-
.added
2818-
.iter()
2819-
.filter_map(|l| {
2820-
declared_name(l)
2821-
.map(|(n, _)| n)
2822-
.or_else(|| container_of(l).and_then(|_| member_declared(l)))
2823-
.map(str::to_string)
2824-
})
2825-
.collect();
2826-
for line in &h.removed {
2827-
let container = container_of(line);
2828-
let (name, exported) = if let Some((n, exported)) = declared_name(line) {
2829-
(Some(n), exported)
2830-
} else if let Some(n) = container.and_then(|_| member_declared(line)) {
2831-
let exported = container.is_some_and(|f| {
2832-
is_public(
2833-
f.path(),
2834-
f.symbol_name.as_deref(),
2835-
without_decorators(&f.content),
2836-
)
2837-
});
2838-
(Some(n), exported)
2839-
} else {
2840-
(None, false)
2841-
};
2842-
let Some(name) = name.filter(|n| {
2843-
n.len() >= 3
2844-
&& !NOT_MEMBER_NAMES.contains(n)
2845-
&& !defined.get(n).is_some_and(|at| at.contains(&h.path))
2846-
&& !readded.contains(*n)
2847-
}) else {
2752+
for d in &at_base {
2753+
if d.name.len() < 3
2754+
|| still.contains(d.name.as_str())
2755+
|| removed.iter().any(|(n, _, _, _)| *n == d.name)
2756+
{
28482757
continue;
2849-
};
2850-
if !removed.iter().any(|(n, _, _)| n == name) {
2851-
removed.push((name.to_string(), h, exported));
28522758
}
2759+
removed.push((
2760+
d.name.clone(),
2761+
h.path.clone(),
2762+
format!("{}-{}", d.start_line, d.end_line),
2763+
public(d),
2764+
));
28532765
}
28542766
}
28552767
// One grep for every removed name, its lines handed to the names they
28562768
// spell as a word: a deleted 1000-function module ran 1000 greps and
28572769
// outlived the deadline.
2858-
let names: Vec<String> = removed.iter().map(|(n, _, _)| n.clone()).collect();
2770+
let names: Vec<String> = removed.iter().map(|(n, _, _, _)| n.clone()).collect();
28592771
let wanted: FxHashSet<&str> = names.iter().map(String::as_str).collect();
28602772
let mut hits_of: FxHashMap<&str, Vec<(String, u32, String)>> = FxHashMap::default();
28612773
let grepped = read_or_note(
@@ -2884,7 +2796,7 @@ fn removed_definitions(
28842796
holders.entry(rel(state, f.path())).or_default().push(f);
28852797
}
28862798
let mut out = Vec::new();
2887-
for (name, h, exported) in removed {
2799+
for (name, path_removed, lines, exported) in removed {
28882800
if !state.run.check() {
28892801
break;
28902802
}
@@ -2923,9 +2835,9 @@ fn removed_definitions(
29232835
}
29242836
if let Some(at) = moved_to {
29252837
let reaches_new = if bindings::lang_of(&path).is_some() {
2926-
!binds_to(&path, &text, &name, &h.path)
2838+
!binds_to(&path, &text, &name, &path_removed)
29272839
} else {
2928-
at.iter().any(|f| qualified_by(&text, f)) && !qualified_by(&text, &h.path)
2840+
at.iter().any(|f| qualified_by(&text, f)) && !qualified_by(&text, &path_removed)
29292841
};
29302842
if reaches_new {
29312843
continue;
@@ -2951,12 +2863,13 @@ fn removed_definitions(
29512863
.filter(|f| f.start_line() <= line && f.end_line() >= line)
29522864
.min_by_key(|f| f.line_count())
29532865
.copied();
2954-
// A line declaring the name is where it went (a move), never a
2866+
// The line declaring the name is where it went (a move), never a
29552867
// caller of the removed definition; a method of the same name
29562868
// that delegates to it still is one.
2957-
if holder.is_some_and(|f| in_a_hunk(&f.id) && f.line_count() <= 1)
2958-
|| declared_name(&text).is_some_and(|(n, _)| n == name)
2959-
{
2869+
if holder.is_some_and(|f| {
2870+
(in_a_hunk(&f.id) && f.line_count() <= 1)
2871+
|| (f.symbol_name.as_deref() == Some(name.as_str()) && f.start_line() == line)
2872+
}) {
29602873
continue;
29612874
}
29622875
let next = code[at + name.len()..].trim_start();
@@ -2980,19 +2893,14 @@ fn removed_definitions(
29802893
});
29812894
}
29822895
let callers = merge_by_symbol(callers);
2983-
let lines = format!(
2984-
"{}-{}",
2985-
h.old_start,
2986-
h.old_start + h.old_len.saturating_sub(1)
2987-
);
29882896
let contract = exported.then(|| Contract {
2989-
path: h.path.clone(),
2897+
path: path_removed.clone(),
29902898
symbol: Some(name.clone()),
29912899
kind: "removed_api",
29922900
});
29932901
out.push((
29942902
ChangedSymbol {
2995-
path: h.path.clone(),
2903+
path: path_removed,
29962904
symbol: Some(name),
29972905
lines,
29982906
kind: "removed".to_string(),

‎crates/diffctx-native/src/parsers/mod.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ static STRATEGIES: Lazy<Vec<Box<dyn FragmentationStrategy>>> = Lazy::new(|| {
2727

2828
pub(crate) use tree_sitter_strategy::{has_grammar, parse_tree};
2929

30-
pub use tree_sitter_strategy::{ParseOutcome, code_only};
30+
pub use tree_sitter_strategy::{Declaration, ParseOutcome, code_only, declarations};
3131

3232
/// Why a code file's definitions were not all read (#454): `no_grammar` when
3333
/// this build has no grammar for its language, `parse_timeout` when the

0 commit comments

Comments
 (0)