diff --git a/crates/sem-cli/src/formatters/terminal.rs b/crates/sem-cli/src/formatters/terminal.rs index 5fafa6ea..6ecab04f 100644 --- a/crates/sem-cli/src/formatters/terminal.rs +++ b/crates/sem-cli/src/formatters/terminal.rs @@ -85,12 +85,21 @@ pub fn format_terminal(result: &DiffResult, verbose: bool) -> String { }; let type_label = format!("{:<10}", change.entity_type); - let name_display = if let Some(ref old_name) = change.old_entity_name { + let base_name = if let Some(ref old_name) = change.old_entity_name { format!("{old_name} -> {}", change.entity_name) } else { change.entity_name.clone() }; - let name_label = format!("{:<25}", name_display); + let display_name = match &change.parent_name { + Some(p) => format!("{}::{}", p, base_name), + None => base_name, + }; + let truncated = if display_name.len() > 25 { + format!("{}…", &display_name[..24]) + } else { + display_name + }; + let name_label = format!("{:<25}", truncated); lines.push(format!( "{} {} {} {} {}", diff --git a/crates/sem-core/src/model/change.rs b/crates/sem-core/src/model/change.rs index f844dcd8..eadf4f70 100644 --- a/crates/sem-core/src/model/change.rs +++ b/crates/sem-core/src/model/change.rs @@ -32,6 +32,8 @@ pub struct SemanticChange { pub entity_name: String, #[serde(default)] pub entity_line: usize, + #[serde(skip_serializing_if = "Option::is_none")] + pub parent_name: Option, pub file_path: String, #[serde(skip_serializing_if = "Option::is_none")] pub old_entity_name: Option, diff --git a/crates/sem-core/src/model/identity.rs b/crates/sem-core/src/model/identity.rs index 0f43b723..9bc0a32c 100644 --- a/crates/sem-core/src/model/identity.rs +++ b/crates/sem-core/src/model/identity.rs @@ -3,6 +3,14 @@ use std::collections::{HashMap, HashSet}; use super::change::{ChangeType, SemanticChange}; use super::entity::SemanticEntity; +/// Extracts the leaf name from a parent_id string. +/// parent_id format: "{file_path}::{entity_type}::{name}" (for top-level parents) +/// The name is always the last "::" segment. +fn parent_name(entity: &SemanticEntity) -> Option { + let pid = entity.parent_id.as_ref()?; + pid.rsplit("::").next().map(String::from) +} + pub struct MatchResult { pub changes: Vec, } @@ -46,6 +54,7 @@ pub fn match_entities( entity_type: after_entity.entity_type.clone(), entity_name: after_entity.name.clone(), entity_line: after_entity.start_line, + parent_name: parent_name(after_entity), file_path: after_entity.file_path.clone(), old_entity_name: None, old_file_path: None, @@ -143,6 +152,7 @@ pub fn match_entities( entity_type: after_entity.entity_type.clone(), entity_name: after_entity.name.clone(), entity_line: after_entity.start_line, + parent_name: parent_name(after_entity), file_path: after_entity.file_path.clone(), old_entity_name, old_file_path, @@ -274,6 +284,7 @@ pub fn match_entities( entity_type: after_entity.entity_type.clone(), entity_name: after_entity.name.clone(), entity_line: after_entity.start_line, + parent_name: parent_name(after_entity), file_path: after_entity.file_path.clone(), old_entity_name, old_file_path, @@ -297,6 +308,7 @@ pub fn match_entities( entity_type: entity.entity_type.clone(), entity_name: entity.name.clone(), entity_line: entity.start_line, + parent_name: parent_name(entity), file_path: entity.file_path.clone(), old_entity_name: None, old_file_path: None, @@ -318,6 +330,7 @@ pub fn match_entities( entity_type: entity.entity_type.clone(), entity_name: entity.name.clone(), entity_line: entity.start_line, + parent_name: parent_name(entity), file_path: entity.file_path.clone(), old_entity_name: None, old_file_path: None, @@ -330,37 +343,135 @@ pub fn match_entities( }); } - // Deduplicate: when a parent (class) is Modified and one or more of its - // children (methods) are also Modified, drop the parent. The child diffs - // are more specific and the parent body overlaps with them. - // Only applies to Modified; Added/Deleted should still show all entities. - let modified_ids: HashSet<&str> = changes - .iter() - .filter(|c| c.change_type == ChangeType::Modified) - .map(|c| c.entity_id.as_str()) - .collect(); + suppress_redundant_parent_modified(&mut changes, before, after); - if modified_ids.len() > 1 { - let mut parents_to_remove: HashSet<&str> = HashSet::new(); - for entity in after.iter().chain(before.iter()) { - if let Some(ref pid) = entity.parent_id { - if modified_ids.contains(entity.id.as_str()) - && modified_ids.contains(pid.as_str()) - { - parents_to_remove.insert(pid.as_str()); - } + MatchResult { changes } +} + +/// Strips child entity content from a parent entity's content using line numbers, +/// then normalizes whitespace. Returns a string representing only the parent's +/// "own" content (its declaration, signature, etc.) without any child body content. +fn strip_children_content( + content: &str, + parent_start_line: usize, + children: &[&SemanticEntity], +) -> String { + let lines: Vec<&str> = content.lines().collect(); + let mut excluded: HashSet = HashSet::new(); + for child in children { + debug_assert!( + child.start_line >= parent_start_line, + "child start_line ({}) < parent start_line ({}): extraction bug", + child.start_line, + parent_start_line + ); + // Convert absolute 1-based line numbers to 0-based indices within this entity's content + let start_idx = child.start_line.saturating_sub(parent_start_line); + let end_idx = child.end_line.saturating_sub(parent_start_line); + for i in start_idx..=end_idx { + if i < lines.len() { + excluded.insert(i); } } + } + lines + .iter() + .enumerate() + .filter(|(i, _)| !excluded.contains(i)) + .map(|(_, l)| l.trim()) + .filter(|l| !l.is_empty()) + .collect::>() + .join(" ") +} - if !parents_to_remove.is_empty() { - changes.retain(|c| { - !(c.change_type == ChangeType::Modified - && parents_to_remove.contains(c.entity_id.as_str())) - }); +/// Post-processing pass: remove `Modified` changes for parent entities whose +/// modification is entirely a side-effect of their children changing. +/// +/// Example: renaming a method inside a class causes the class `content_hash` to +/// change (because class content includes method bodies), but the class's own +/// declaration line didn't change — only a child did. This function suppresses +/// that spurious `Modified` entry so the output shows only the child's change. +fn suppress_redundant_parent_modified( + changes: &mut Vec, + before: &[SemanticEntity], + after: &[SemanticEntity], +) { + let before_by_id: HashMap<&str, &SemanticEntity> = + before.iter().map(|e| (e.id.as_str(), e)).collect(); + let after_by_id: HashMap<&str, &SemanticEntity> = + after.iter().map(|e| (e.id.as_str(), e)).collect(); + + // Map parent entity ID → its direct children in before/after + let mut before_children: HashMap<&str, Vec<&SemanticEntity>> = HashMap::new(); + for e in before { + if let Some(ref pid) = e.parent_id { + before_children.entry(pid.as_str()).or_default().push(e); + } + } + let mut after_children: HashMap<&str, Vec<&SemanticEntity>> = HashMap::new(); + for e in after { + if let Some(ref pid) = e.parent_id { + after_children.entry(pid.as_str()).or_default().push(e); } } - MatchResult { changes } + // All entity IDs that appear in any change (across before and after) + let changed_ids: HashSet<&str> = changes.iter().map(|c| c.entity_id.as_str()).collect(); + + let mut to_suppress: HashSet = HashSet::new(); + + for change in changes.iter() { + if change.change_type != ChangeType::Modified { + continue; + } + let eid = change.entity_id.as_str(); + + let b_children = before_children.get(eid).map(|v| v.as_slice()).unwrap_or(&[]); + let a_children = after_children.get(eid).map(|v| v.as_slice()).unwrap_or(&[]); + + // Only consider container entities (those that have children) + if b_children.is_empty() && a_children.is_empty() { + continue; + } + + // At least one child must have a change to justify suppression + let has_changed_child = b_children.iter().any(|c| changed_ids.contains(c.id.as_str())) + || a_children.iter().any(|c| changed_ids.contains(c.id.as_str())); + if !has_changed_child { + continue; + } + + let before_parent = match before_by_id.get(eid) { + Some(e) => e, + None => continue, + }; + let after_parent = match after_by_id.get(eid) { + Some(e) => e, + None => continue, + }; + + // Strip child content from both sides and compare what remains. + // If the parent's own content (declaration, fields, etc.) is unchanged, + // the Modified is purely a consequence of child changes — suppress it. + let before_own = strip_children_content( + &before_parent.content, + before_parent.start_line, + b_children, + ); + let after_own = strip_children_content( + &after_parent.content, + after_parent.start_line, + a_children, + ); + + if before_own == after_own { + to_suppress.insert(change.entity_id.clone()); + } + } + + changes.retain(|c| { + !(c.change_type == ChangeType::Modified && to_suppress.contains(&c.entity_id)) + }); } /// Default content similarity using Jaccard index on whitespace-split tokens @@ -451,19 +562,19 @@ mod tests { #[test] fn test_parent_child_dedup_class_method() { - // Class entity contains the method body in its content. - // parent_id stores the full entity ID of the parent. + // Use realistic multi-line content so line-number-based child stripping works. + // Line 1: class header, lines 2-3: constructor, lines 4-6: genPg, line 7: closing brace. let class_before = SemanticEntity { id: "a.ts::class::DataStack".to_string(), file_path: "a.ts".to_string(), entity_type: "class".to_string(), name: "DataStack".to_string(), parent_id: None, - content: "class DataStack { constructor() {} genPg() { old } }".to_string(), - content_hash: content_hash("class DataStack { constructor() {} genPg() { old } }"), + content: "class DataStack {\n constructor() {}\n genPg() {\n old\n }\n}".to_string(), + content_hash: content_hash("class DataStack {\n constructor() {}\n genPg() {\n old\n }\n}"), structural_hash: None, start_line: 1, - end_line: 10, + end_line: 6, metadata: None, }; let method_before = SemanticEntity { @@ -472,11 +583,11 @@ mod tests { entity_type: "method".to_string(), name: "genPg".to_string(), parent_id: Some("a.ts::class::DataStack".to_string()), - content: "genPg() { old }".to_string(), - content_hash: content_hash("genPg() { old }"), + content: "genPg() {\n old\n }".to_string(), + content_hash: content_hash("genPg() {\n old\n }"), structural_hash: None, - start_line: 5, - end_line: 8, + start_line: 3, + end_line: 5, metadata: None, }; @@ -486,11 +597,11 @@ mod tests { entity_type: "class".to_string(), name: "DataStack".to_string(), parent_id: None, - content: "class DataStack { constructor() {} genPg() { new } }".to_string(), - content_hash: content_hash("class DataStack { constructor() {} genPg() { new } }"), + content: "class DataStack {\n constructor() {}\n genPg() {\n new\n }\n}".to_string(), + content_hash: content_hash("class DataStack {\n constructor() {}\n genPg() {\n new\n }\n}"), structural_hash: None, start_line: 1, - end_line: 10, + end_line: 6, metadata: None, }; let method_after = SemanticEntity { @@ -499,11 +610,11 @@ mod tests { entity_type: "method".to_string(), name: "genPg".to_string(), parent_id: Some("a.ts::class::DataStack".to_string()), - content: "genPg() { new }".to_string(), - content_hash: content_hash("genPg() { new }"), + content: "genPg() {\n new\n }".to_string(), + content_hash: content_hash("genPg() {\n new\n }"), structural_hash: None, - start_line: 5, - end_line: 8, + start_line: 3, + end_line: 5, metadata: None, }; @@ -592,4 +703,314 @@ mod tests { assert!(score > 0.5); assert!(score < 1.0); } + + // ---- suppress_redundant_parent_modified tests ---- + + fn make_entity_with_parent( + id: &str, + name: &str, + content: &str, + file_path: &str, + parent_id: Option<&str>, + start_line: usize, + end_line: usize, + ) -> SemanticEntity { + SemanticEntity { + id: id.to_string(), + file_path: file_path.to_string(), + entity_type: "function".to_string(), + name: name.to_string(), + parent_id: parent_id.map(String::from), + content: content.to_string(), + content_hash: content_hash(content), + structural_hash: None, + start_line, + end_line, + metadata: None, + } + } + + // A realistic method body with enough tokens that Jaccard similarity stays >0.8 + // even when only the method name differs (one token changes out of ~15 unique). + const METHOD_BODY: &str = + "x = 1\n y = 2\n z = 3\n w = x + y\n return w + z"; + + /// When a child is renamed, the parent should NOT appear as Modified. + #[test] + fn test_parent_not_modified_when_child_renamed() { + let before_method_content = + format!("def old_method(self):\n {METHOD_BODY}"); + let after_method_content = + format!("def new_method(self):\n {METHOD_BODY}"); + let before_class_content = + format!("class Svc:\n {before_method_content}"); + let after_class_content = + format!("class Svc:\n {after_method_content}"); + + let before_class = make_entity_with_parent( + "a.py::class::Svc", "Svc", &before_class_content, "a.py", None, 1, 6, + ); + let before_method = make_entity_with_parent( + "a.py::a.py::class::Svc::old_method", + "old_method", + &before_method_content, + "a.py", + Some("a.py::class::Svc"), + 2, + 6, + ); + let after_class = make_entity_with_parent( + "a.py::class::Svc", "Svc", &after_class_content, "a.py", None, 1, 6, + ); + let after_method = make_entity_with_parent( + "a.py::a.py::class::Svc::new_method", + "new_method", + &after_method_content, + "a.py", + Some("a.py::class::Svc"), + 2, + 6, + ); + + let before = vec![before_class, before_method]; + let after = vec![after_class, after_method]; + + let result = match_entities( + &before, + &after, + "a.py", + Some(&default_similarity), + None, + None, + ); + + let types: Vec = result.changes.iter().map(|c| c.change_type).collect(); + assert!(types.contains(&ChangeType::Renamed), "expected method Renamed"); + assert!( + !types.contains(&ChangeType::Modified), + "parent class should not appear as Modified when only child renamed" + ); + } + + /// When a method is added to a class, the class should NOT appear as Modified. + #[test] + fn test_parent_not_modified_when_child_added() { + let method_content = format!("def bar(self):\n {METHOD_BODY}"); + let new_method_content = format!("def baz(self):\n {METHOD_BODY}"); + + let before_class = make_entity_with_parent( + "a.py::class::Svc", + "Svc", + &format!("class Svc:\n {method_content}"), + "a.py", + None, + 1, + 6, + ); + let before_method = make_entity_with_parent( + "a.py::a.py::class::Svc::bar", + "bar", + &method_content, + "a.py", + Some("a.py::class::Svc"), + 2, + 6, + ); + let after_class = make_entity_with_parent( + "a.py::class::Svc", + "Svc", + &format!("class Svc:\n {method_content}\n {new_method_content}"), + "a.py", + None, + 1, + 12, + ); + let after_method_bar = make_entity_with_parent( + "a.py::a.py::class::Svc::bar", + "bar", + &method_content, + "a.py", + Some("a.py::class::Svc"), + 2, + 6, + ); + let after_method_baz = make_entity_with_parent( + "a.py::a.py::class::Svc::baz", + "baz", + &new_method_content, + "a.py", + Some("a.py::class::Svc"), + 7, + 12, + ); + + let before = vec![before_class, before_method]; + let after = vec![after_class, after_method_bar, after_method_baz]; + + let result = match_entities(&before, &after, "a.py", None, None, None); + + let types: Vec = result.changes.iter().map(|c| c.change_type).collect(); + assert!(types.contains(&ChangeType::Added), "expected new method Added"); + assert!( + !types.contains(&ChangeType::Modified), + "parent class should not appear as Modified when only child added" + ); + } + + /// When the class's own declaration changes (e.g. base class added) in addition + /// to a child rename, the parent SHOULD remain as Modified. + #[test] + fn test_parent_still_modified_when_own_content_changes() { + let before_method_content = + format!("def old_method(self):\n {METHOD_BODY}"); + let after_method_content = + format!("def new_method(self):\n {METHOD_BODY}"); + + // Before: "class Svc:" — After: "class Svc(Base):" — declaration changed + let before_class = make_entity_with_parent( + "a.py::class::Svc", + "Svc", + &format!("class Svc:\n {before_method_content}"), + "a.py", + None, + 1, + 6, + ); + let before_method = make_entity_with_parent( + "a.py::a.py::class::Svc::old_method", + "old_method", + &before_method_content, + "a.py", + Some("a.py::class::Svc"), + 2, + 6, + ); + let after_class = make_entity_with_parent( + "a.py::class::Svc", + "Svc", + &format!("class Svc(Base):\n {after_method_content}"), + "a.py", + None, + 1, + 6, + ); + let after_method = make_entity_with_parent( + "a.py::a.py::class::Svc::new_method", + "new_method", + &after_method_content, + "a.py", + Some("a.py::class::Svc"), + 2, + 6, + ); + + let before = vec![before_class, before_method]; + let after = vec![after_class, after_method]; + + let result = match_entities( + &before, + &after, + "a.py", + Some(&default_similarity), + None, + None, + ); + + let types: Vec = result.changes.iter().map(|c| c.change_type).collect(); + assert!(types.contains(&ChangeType::Renamed), "expected method Renamed"); + assert!( + types.contains(&ChangeType::Modified), + "parent class should still be Modified when its own declaration changed" + ); + } + + /// `parent_name` is None for top-level entities and Some("ClassName") for nested ones. + #[test] + fn test_parent_name_populated_on_changes() { + let method_content = format!("def bar(self):\n {METHOD_BODY}"); + + let before_class = make_entity_with_parent( + "a.py::class::Svc", "Svc", + &format!("class Svc:\n {method_content}"), + "a.py", None, 1, 6, + ); + let before_method = make_entity_with_parent( + "a.py::a.py::class::Svc::bar", "bar", &method_content, + "a.py", Some("a.py::class::Svc"), 2, 6, + ); + + // Modify the method body so it shows as Modified + let after_method_content = format!("def bar(self):\n {METHOD_BODY}\n return 0"); + let after_class = make_entity_with_parent( + "a.py::class::Svc", "Svc", + &format!("class Svc:\n {after_method_content}"), + "a.py", None, 1, 7, + ); + let after_method = make_entity_with_parent( + "a.py::a.py::class::Svc::bar", "bar", &after_method_content, + "a.py", Some("a.py::class::Svc"), 2, 7, + ); + + let before = vec![before_class, before_method]; + let after = vec![after_class, after_method]; + let result = match_entities(&before, &after, "a.py", None, None, None); + + let method_change = result.changes.iter() + .find(|c| c.entity_name == "bar") + .expect("expected change for bar"); + + assert_eq!(method_change.change_type, ChangeType::Modified); + assert_eq!( + method_change.parent_name.as_deref(), + Some("Svc"), + "nested method should carry parent class name" + ); + + // Top-level entity (the class itself) should not appear due to suppression, + // but if we check a top-level entity directly it should have no parent_name. + let top_level = make_entity("a.py::function::standalone", "standalone", "def standalone(): pass", "a.py"); + let top_level_after = make_entity("a.py::function::standalone", "standalone", "def standalone(): return 1", "a.py"); + let top_result = match_entities(&[top_level], &[top_level_after], "a.py", None, None, None); + assert_eq!(top_result.changes.len(), 1); + assert_eq!( + top_result.changes[0].parent_name, + None, + "top-level entity should have no parent_name" + ); + } + + /// When all children are deleted the parent body changes structurally, + /// so the parent should remain visible as Modified. + #[test] + fn test_parent_still_modified_when_all_children_deleted() { + let method_content = format!("def bar(self):\n {METHOD_BODY}"); + + let before_class = make_entity_with_parent( + "a.py::class::Svc", "Svc", + &format!("class Svc:\n {method_content}"), + "a.py", None, 1, 6, + ); + let before_method = make_entity_with_parent( + "a.py::a.py::class::Svc::bar", "bar", &method_content, + "a.py", Some("a.py::class::Svc"), 2, 6, + ); + + // After: class body is now just `pass` — completely different from before + let after_class = make_entity_with_parent( + "a.py::class::Svc", "Svc", + "class Svc:\n pass", + "a.py", None, 1, 2, + ); + + let before = vec![before_class, before_method]; + let after = vec![after_class]; + let result = match_entities(&before, &after, "a.py", None, None, None); + + let types: Vec = result.changes.iter().map(|c| c.change_type).collect(); + assert!(types.contains(&ChangeType::Deleted), "method should be Deleted"); + assert!( + types.contains(&ChangeType::Modified), + "parent class should remain Modified when all children are deleted and body changes" + ); + } }