passes: gate step-2 renames on the virtual family index (fixes override-chain split)

crates/noxis-passes/src/rename/assign.rs

@@ -13,6 +13,9 @@ use crate::namegen::NameGenerator;
 /// `(owner, name, descriptor)` - matches a JVM reference.
 pub(super) type MemberKey = (String, String, String);
 
+/// `(method name, params)` - one virtual family.
+pub(super) type FamilyKey = (String, String);
+
 /// Rename maps for one run: `declared` is where a member was declared,
 /// `alias` is subclasses that inherit the name without declaring it themselves.
 #[derive(Debug, Default)]
@@ -85,19 +88,18 @@ impl RenameMaps {
     }
 }
 
-/// Rename virtual methods as a "family": one name for the whole hierarchy,
+/// Family = (name, params): the return type is ignored, so covariant
-/// including bridge methods with covariant returns.
+/// returns and bridge methods (`ACC_BRIDGE`) are captured together.
-pub(super) fn rename_virtual_families(
+///
+/// Also returns the families that must keep their original name: a locked
+/// class or a `keep_member` name in the family locks it for everybody -
+/// renaming only the other members would split the override chain.
+pub(super) fn build_virtual_families(
     program: &Program,
     analysis: &Analysis,
-    maps: &mut RenameMaps,
+) -> (BTreeMap<FamilyKey, Vec<MemberKey>>, BTreeSet<FamilyKey>) {
-    cfg: &Config,
+    let mut families: BTreeMap<FamilyKey, Vec<MemberKey>> = BTreeMap::new();
-    ctx: &mut Context,
+    let mut locked: BTreeSet<FamilyKey> = BTreeSet::new();
-) -> usize {
-    // Family = (name, params): the return type is ignored, so covariant
-    // returns and bridge methods (`ACC_BRIDGE`) are captured together.
-    let mut families: BTreeMap<(String, String), Vec<MemberKey>> = BTreeMap::new();
-    let mut locked_families: BTreeSet<(String, String)> = BTreeSet::new();
 
     for class in &program.classes {
         let locked_class = analysis.is_locked(&class.this_class);
@@ -113,7 +115,7 @@ pub(super) fn rename_virtual_families(
             };
             let family = (method.name.clone(), params);
             if locked_class || analysis.keep.keeps_member(&method.name) {
-                locked_families.insert(family.clone());
+                locked.insert(family.clone());
             }
             families.entry(family).or_default().push((
                 class.this_class.clone(),
@@ -122,7 +124,18 @@ pub(super) fn rename_virtual_families(
             ));
         }
     }
+    (families, locked)
+}
 
+/// Rename virtual methods as a "family": one name for the whole hierarchy,
+/// including bridge methods with covariant returns.
+pub(super) fn rename_virtual_families(
+    families: BTreeMap<(String, String), Vec<MemberKey>>,
+    locked_families: BTreeSet<FamilyKey>,
+    maps: &mut RenameMaps,
+    cfg: &Config,
+    ctx: &mut Context,
+) -> usize {
     let mut renamed = 0usize;
     let mut family_index = 0u64;
     for (family_key, members) in families {
@@ -150,7 +163,7 @@ pub(super) fn rename_virtual_families(
 }
 
 /// `(<params>)<ret>` → `(<params>)`, or `None` if the descriptor is malformed.
-fn params_of(descriptor: &str) -> Option<String> {
+pub(super) fn params_of(descriptor: &str) -> Option<String> {
     let close = descriptor.find(')')?;
     if !descriptor.starts_with('(') {
         return None;

crates/noxis-passes/src/rename/mod.rs

@@ -23,7 +23,8 @@ use noxis_classfile::Program;
 use noxis_core::{Context, CoreError, Pass, PassId, PassReport, field_key, method_key};
 
 use self::assign::{
-    RenameMaps, generate_packages, parent_package, rename_virtual_families, simple_name, widen_all,
+    RenameMaps, build_virtual_families, generate_packages, params_of, parent_package,
+    rename_virtual_families, simple_name, widen_all,
 };
 use self::rewrite::rewrite_class;
 use crate::namegen::NameGenerator;
@@ -58,6 +59,13 @@ impl Pass for RenamePass {
         let mut maps = RenameMaps::default();
         let mut renamed_classes = 0usize;
 
+        // Family membership up front: step 2 must not rename a method that
+        // belongs to a shared or locked family on its own - a per-class
+        // rename splits the override chain (the JVM then fails dispatch
+        // with AbstractMethodError, real case: shaded gson's package-private
+        // `ReflectionHelper$RecordHelper.isRecord` chain).
+        let (families, locked_families) = build_virtual_families(program, &analysis);
+
         // 1. Class names.
         {
             let packages = generate_packages(&cfg, &mut ctx.rng("rename/packages"));
@@ -117,6 +125,35 @@ impl Pass for RenamePass {
                         method_names.reserve(method.name.clone());
                         continue;
                     }
+                    // A method shared with another class must not get its
+                    // name here: the family pass assigns one common name
+                    // (aggressive mode), or the whole family keeps the
+                    // original when it is locked. A per-class rename splits
+                    // the override chain - the JVM then fails dispatch with
+                    // `AbstractMethodError` (real case: shaded gson's
+                    // package-private `ReflectionHelper$RecordHelper.isRecord`
+                    // chain, each class renamed apart).
+                    if let Some(params) = params_of(&method.descriptor) {
+                        let key = (method.name.clone(), params);
+                        if let Some(members) = families.get(&key) {
+                            let hierarchy_related = members.iter().any(|(other, _, _)| {
+                                other != &owner
+                                    && (hierarchy.is_subclass_of(other, &owner)
+                                        || hierarchy.is_subclass_of(&owner, other))
+                            });
+                            // Aggressive mode renames every shared family
+                            // uniformly. Without it the family pass does not
+                            // run, so only hierarchy-related members must stay
+                            // together; unrelated same-name methods never meet
+                            // at dispatch and may rename individually.
+                            let shared = members.len() > 1
+                                && (cfg.rename.rename_public_members || hierarchy_related);
+                            if shared || locked_families.contains(&key) {
+                                method_names.reserve(method.name.clone());
+                                continue;
+                            }
+                        }
+                    }
                     let safety = method_rename_safety(
                         program,
                         &hierarchy,
@@ -181,7 +218,8 @@ impl Pass for RenamePass {
 
         // 3. Virtual families - aggressive mode only.
         if cfg.rename.rename_public_members {
-            renamed_methods += rename_virtual_families(program, &analysis, &mut maps, &cfg, ctx);
+            renamed_methods +=
+                rename_virtual_families(families, locked_families, &mut maps, &cfg, ctx);
         }
 
         // 4. Aliases for subclasses.

crates/noxis-passes/tests/family.rs

@@ -0,0 +1,105 @@
+//! Virtual-family integrity: a package-private override chain must be
+//! renamed as one family, never member by member.
+//!
+//! `method_rename_safety` used to classify package-private overrides as
+//! `Safe` (its subclass check never fired: `declaring_chain` only walks
+//! upwards). The individual renames then split one family into different
+//! names, and the JVM failed at dispatch with `AbstractMethodError`
+//! (real case: shaded gson `ReflectionHelper$RecordHelper.isRecord`).
+
+#![allow(clippy::expect_used, clippy::unwrap_used)]
+
+use noxis_classfile::{ClassBuilder, Code, Const, Insn, Method, Prim, Program, Version};
+use noxis_core::{Config, Context, Pass, RenameConfig};
+use noxis_passes::RenamePass;
+
+fn check_code() -> Code {
+    Code {
+        max_stack: 1,
+        max_locals: 2,
+        insns: vec![Insn::Const(Const::Int(1)), Insn::Return(Some(Prim::Int))],
+        exception_table: Vec::new(),
+        frames: Vec::new(),
+        attributes: Vec::new(),
+    }
+}
+
+/// `demo/Base` declares package-private abstract `check(LClass;)Z`;
+/// `demo/ImplA`/`demo/ImplB` override it.
+fn sample() -> Program {
+    let mut base = ClassBuilder::new("demo/Base", Some("java/lang/Object"))
+        .version(Version::JAVA_21)
+        .build();
+    base.methods.push(Method {
+        access_flags: 0x0400, // abstract, package-private
+        name: "check".to_string(),
+        descriptor: "(Ljava/lang/Class;)Z".to_string(),
+        code: None,
+        attributes: Vec::new(),
+    });
+    let impl_a = ClassBuilder::new("demo/ImplA", Some("demo/Base"))
+        .version(Version::JAVA_21)
+        .method_with_code(0x0000, "check", "(Ljava/lang/Class;)Z", check_code())
+        .build();
+    let impl_b = ClassBuilder::new("demo/ImplB", Some("demo/Base"))
+        .version(Version::JAVA_21)
+        .method_with_code(0x0000, "check", "(Ljava/lang/Class;)Z", check_code())
+        .build();
+    Program {
+        classes: vec![base, impl_a, impl_b],
+        resources: Vec::new(),
+    }
+}
+
+fn run(program: &mut Program, rename_public_members: bool) {
+    let config = Config {
+        seed: Some(7),
+        rename: RenameConfig {
+            rename_public_members,
+            ..RenameConfig::default()
+        },
+        ..Config::default()
+    };
+    let mut ctx = Context::new(config);
+    RenamePass.apply(&mut ctx, program).expect("rename pass");
+}
+
+/// The three `check(LClass;)Z` methods after the pass: one shared name.
+fn family_names(program: &Program) -> Vec<String> {
+    let mut out = Vec::new();
+    for class in &program.classes {
+        for method in &class.methods {
+            if method.descriptor == "(Ljava/lang/Class;)Z" {
+                out.push(method.name.clone());
+            }
+        }
+    }
+    out
+}
+
+#[test]
+fn package_private_override_chain_renames_as_one_family() {
+    let mut program = sample();
+    run(&mut program, true);
+
+    let names = family_names(&program);
+    assert_eq!(names.len(), 3, "all three overrides must be present");
+    assert!(
+        names.iter().all(|name| name == &names[0]),
+        "one family - one name, got {names:?}"
+    );
+    assert_ne!(names[0], "check", "the family must actually be renamed");
+}
+
+#[test]
+fn package_private_overrides_stay_consistent_without_public_rename() {
+    let mut program = sample();
+    run(&mut program, false);
+
+    let names = family_names(&program);
+    assert_eq!(names.len(), 3, "all three overrides must be present");
+    assert!(
+        names.iter().all(|name| name == &names[0]),
+        "families off must keep the chain untouched, got {names:?}"
+    );
+}