diff --git a/crates/noxis-passes/src/rename/assign.rs b/crates/noxis-passes/src/rename/assign.rs index 77c12ce..4ac8086 100644 --- a/crates/noxis-passes/src/rename/assign.rs +++ b/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, -/// including bridge methods with covariant returns. -pub(super) fn rename_virtual_families( +/// Family = (name, params): the return type is ignored, so covariant +/// returns and bridge methods (`ACC_BRIDGE`) are captured together. +/// +/// 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, - cfg: &Config, - ctx: &mut Context, -) -> 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> = BTreeMap::new(); - let mut locked_families: BTreeSet<(String, String)> = BTreeSet::new(); +) -> (BTreeMap>, BTreeSet) { + let mut families: BTreeMap> = BTreeMap::new(); + let mut locked: BTreeSet = 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>, + locked_families: BTreeSet, + 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( } /// `()` → `()`, or `None` if the descriptor is malformed. -fn params_of(descriptor: &str) -> Option { +pub(super) fn params_of(descriptor: &str) -> Option { let close = descriptor.find(')')?; if !descriptor.starts_with('(') { return None; diff --git a/crates/noxis-passes/src/rename/mod.rs b/crates/noxis-passes/src/rename/mod.rs index ae6a081..c137694 100644 --- a/crates/noxis-passes/src/rename/mod.rs +++ b/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. diff --git a/crates/noxis-passes/tests/family.rs b/crates/noxis-passes/tests/family.rs new file mode 100644 index 0000000..a556004 --- /dev/null +++ b/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 { + 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:?}" + ); +}