JSON: recursive entity extraction with suppress-parent display

crates/sem-core/src/model/identity.rs

@@ -3,10 +3,39 @@ use std::collections::{HashMap, HashSet};
 use super::change::{ChangeType, SemanticChange};
 use super::entity::SemanticEntity;
 
-/// Extracts the leaf name from a parent_id string (last "::" segment).
-fn parent_name(entity: &SemanticEntity) -> Option<String> {
-    let pid = entity.parent_id.as_ref()?;
-    pid.rsplit("::").next().map(String::from)
+fn parent_name(
+    entity: &SemanticEntity,
+    by_id: &HashMap<&str, &SemanticEntity>,
+) -> Option<String> {
+    let mut parts: Vec<&str> = Vec::new();
+    let mut visited: HashSet<&str> = HashSet::new();
+    let mut pid = entity.parent_id.as_deref()?;
+    loop {
+        if !visited.insert(pid) {
+            break;
+        }
+        match by_id.get(pid) {
+            Some(parent) => {
+                // Skip ancestors with empty names (e.g. JSON's empty-string
+                // root-package key in package-lock.json). The full path is
+                // still recoverable from entity_id; the displayed chain is
+                // for human readability.
+                if !parent.name.is_empty() {
+                    parts.push(parent.name.as_str());
+                }
+                match parent.parent_id.as_deref() {
+                    Some(next) => pid = next,
+                    None => break,
+                }
+            }
+            None => break,
+        }
+    }
+    if parts.is_empty() {
+        return None;
+    }
+    parts.reverse();
+    Some(parts.join("::"))
 }
 
 pub struct MatchResult {
@@ -29,6 +58,7 @@ fn make_change(
     before_entity: Option<&SemanticEntity>,
     commit_sha: Option<&str>,
     author: Option<&str>,
+    by_id: &HashMap<&str, &SemanticEntity>,
 ) -> SemanticChange {
     let prefix = match change_type {
         ChangeType::Added => "added::",
@@ -49,7 +79,7 @@ fn make_change(
         entity_type: primary.entity_type.clone(),
         entity_name: primary.name.clone(),
         entity_line: primary.start_line,
-        parent_name: parent_name(primary),
+        parent_name: parent_name(primary, by_id),
         file_path: primary.file_path.clone(),
         old_entity_name: before_entity.and_then(|b| {
             (b.name != after_entity.name).then(|| b.name.clone())
@@ -94,14 +124,22 @@ pub fn match_entities(
     let after_by_id: HashMap<&str, &SemanticEntity> =
         after.iter().map(|e| (e.id.as_str(), e)).collect();
 
+    // Combined map for ancestor-chain lookup: after takes precedence so the
+    // displayed path reflects the post-change tree for non-deleted entities.
+    let combined_by_id: HashMap<&str, &SemanticEntity> = before
+        .iter()
+        .map(|e| (e.id.as_str(), e))
+        .chain(after.iter().map(|e| (e.id.as_str(), e)))
+        .collect();
+
     // Phase 1: Exact ID match
     for (&id, after_entity) in &after_by_id {
         if let Some(before_entity) = before_by_id.get(id) {
             matched_before.insert(id);
             matched_after.insert(id);
 
             if before_entity.content_hash != after_entity.content_hash {
-                let mut change = make_change(after_entity, ChangeType::Modified, Some(before_entity), commit_sha, author);
+                let mut change = make_change(after_entity, ChangeType::Modified, Some(before_entity), commit_sha, author, &combined_by_id);
                 change.structural_change = match (&before_entity.structural_hash, &after_entity.structural_hash) {
                     (Some(before_sh), Some(after_sh)) => Some(before_sh != after_sh),
                     _ => None,
@@ -171,7 +209,7 @@ pub fn match_entities(
                 continue;
             }
 
-            changes.push(make_change(after_entity, classify_match(before_entity, after_entity), Some(before_entity), commit_sha, author));
+            changes.push(make_change(after_entity, classify_match(before_entity, after_entity), Some(before_entity), commit_sha, author, &combined_by_id));
         }
     }
 
@@ -269,24 +307,24 @@ pub fn match_entities(
                     continue;
                 }
 
-                changes.push(make_change(after_entity, classify_match(matched, after_entity), Some(matched), commit_sha, author));
+                changes.push(make_change(after_entity, classify_match(matched, after_entity), Some(matched), commit_sha, author, &combined_by_id));
             }
         }
     }
 
     // Phase 4: Intra-file reorder detection
     // For entities that matched by exact ID with identical content (unchanged),
     // check if their relative ordering changed within the file.
-    detect_reorders(before, after, &matched_before, &matched_after, &mut changes, commit_sha, author);
+    detect_reorders(before, after, &matched_before, &matched_after, &mut changes, commit_sha, author, &combined_by_id);
 
     // Remaining unmatched before = deleted
     for entity in before.iter().filter(|e| !matched_before.contains(e.id.as_str())) {
-        changes.push(make_change(entity, ChangeType::Deleted, Some(entity), commit_sha, author));
+        changes.push(make_change(entity, ChangeType::Deleted, Some(entity), commit_sha, author, &combined_by_id));
     }
 
     // Remaining unmatched after = added
     for entity in after.iter().filter(|e| !matched_after.contains(e.id.as_str())) {
-        changes.push(make_change(entity, ChangeType::Added, None, commit_sha, author));
+        changes.push(make_change(entity, ChangeType::Added, None, commit_sha, author, &combined_by_id));
     }
 
     MatchResult { changes }
@@ -333,6 +371,7 @@ fn detect_reorders(
     changes: &mut Vec<SemanticChange>,
     commit_sha: Option<&str>,
     author: Option<&str>,
+    by_id: &HashMap<&str, &SemanticEntity>,
 ) {
     // Collect unchanged entities: matched by ID with same content_hash
     let before_by_id: HashMap<&str, &SemanticEntity> =
@@ -383,7 +422,7 @@ fn detect_reorders(
             if lis_set.contains(&i) {
                 continue;
             }
-            changes.push(make_change(after_entity, ChangeType::Reordered, None, commit_sha, author));
+            changes.push(make_change(after_entity, ChangeType::Reordered, None, commit_sha, author, by_id));
         }
     }
 }
@@ -730,4 +769,22 @@ mod tests {
         assert!(score > 0.5);
         assert!(score < 1.0);
     }
+
+    #[test]
+    fn parent_name_terminates_on_cyclic_parent_id() {
+        // Two entities whose parent_id chains form a cycle. parent_name
+        // would loop forever without the visited-set guard.
+        let a = make_entity_with_parent("A", "A", "", "f", Some("B"));
+        let b = make_entity_with_parent("B", "B", "", "f", Some("A"));
+        let mut by_id: HashMap<&str, &SemanticEntity> = HashMap::new();
+        by_id.insert("A", &a);
+        by_id.insert("B", &b);
+        // Synthesize a leaf whose parent_id enters the cycle via A.
+        let leaf = make_entity_with_parent("L", "L", "", "f", Some("A"));
+        let chain = parent_name(&leaf, &by_id);
+        // Must terminate. We don't assert exact contents — order/composition
+        // depends on which side of the cycle is reached first; the safety
+        // property is "this returns at all."
+        assert!(chain.is_some());
+    }
 }

crates/sem-core/src/parser/differ.rs

@@ -145,12 +145,6 @@ pub fn compute_semantic_diff(
     }
 }
 
-/// Remove "Modified" parent entities from the change list when at least one
-/// child entity also appears as a change.  This avoids showing e.g. an impl
-/// block as modified when the real change is in a method inside it.
-/// Only suppresses container entity types (impl, trait, module) where the
-/// parent is just a wrapper. Functions, structs, etc. are never suppressed
-/// because they have independent meaningful content.
 fn suppress_redundant_parents(
     changes: &mut Vec<SemanticChange>,
     before: &[SemanticEntity],
@@ -164,6 +158,7 @@ fn suppress_redundant_parents(
         "impl", "trait", "module", "class", "interface", "mixin",
         "extension", "namespace", "export", "package",
         "svelte_instance_script", "svelte_module_script",
+        "object",
     ];
 
     let before_by_id: HashMap<&str, &SemanticEntity> =
@@ -204,11 +199,12 @@ fn suppress_redundant_parents(
             continue;
         }
 
-        // For Added/Deleted containers: suppress unconditionally — the children carry the detail.
-        // For Modified: only suppress if the container's own declaration didn't change.
+        // Added/Deleted: suppress unconditionally; the children carry the detail.
+        // Modified: only suppress if the container's own declaration is unchanged
+        // and the value type didn't transition.
         let should_suppress = if change.change_type == ChangeType::Modified {
             match (before_by_id.get(eid), after_by_id.get(eid)) {
-                (Some(bp), Some(ap)) => {
+                (Some(bp), Some(ap)) if bp.entity_type == ap.entity_type => {
                     let before_own = strip_children_content(&bp.content, bp.start_line, b_children);
                     let after_own = strip_children_content(&ap.content, ap.start_line, a_children);
                     before_own == after_own
@@ -224,11 +220,47 @@ fn suppress_redundant_parents(
         }
     }
 
+    // Suppress an old parent that a Moved child left behind when the old
+    // parent itself appears as a change — handles the parent-rename case
+    // where the parent itself failed to match.
+    for change in changes.iter() {
+        if change.change_type == ChangeType::Moved {
+            if let Some(ref old_pid) = change.old_parent_id {
+                if changed_ids.contains(old_pid.as_str()) {
+                    suppress.insert(old_pid.clone());
+                }
+            }
+        }
+    }
+
     if !suppress.is_empty() {
+        changes.retain(|c| !suppress.contains(&c.entity_id));
+    }
+
+    // Drop a Moved child whose key is unchanged and whose old parent matches
+    // a Renamed entity — the child only "moved" because the parent renamed.
+    let renamed_before_ids: HashSet<&str> = changes
+        .iter()
+        .filter(|c| c.change_type == ChangeType::Renamed)
+        .filter_map(|c| {
+            let old_name = c.old_entity_name.as_deref()?;
+            let after_entity = after_by_id.get(c.entity_id.as_str())?;
+            before.iter()
+                .find(|e| {
+                    e.name == old_name
+                        && e.entity_type == after_entity.entity_type
+                        && e.parent_id == after_entity.parent_id
+                })
+                .map(|e| e.id.as_str())
+        })
+        .collect();
+
+    if !renamed_before_ids.is_empty() {
         changes.retain(|c| {
-            !(matches!(c.change_type, ChangeType::Modified | ChangeType::Added | ChangeType::Deleted)
-                && suppress.contains(&c.entity_id)
-                && CONTAINER_TYPES.contains(&c.entity_type.as_str()))
+            !(c.change_type == ChangeType::Moved
+                && c.old_entity_name.is_none()
+                && c.old_parent_id.as_deref()
+                    .map_or(false, |pid| renamed_before_ids.contains(pid)))
         });
     }
 }