Skip to content

Commit 9fe0dbf

Browse files
better utilize RecordIndexingVisitor in RecordComponentProposalService
RecordIndexingVisitor: only check bytecode if multiple methods are getter candidates; allows finding overrides in simple cases RecordIndexingVisitor: when there are multiple getter candidates, check name and descriptor in addition to bytecode; prevents some false-positives in getter finding (though false positives for non-getters are still possible)
1 parent f9bd3cf commit 9fe0dbf

File tree

2 files changed

+137
-92
lines changed

2 files changed

+137
-92
lines changed

enigma/src/main/java/org/quiltmc/enigma/impl/plugin/RecordComponentProposalService.java

Lines changed: 17 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -2,20 +2,17 @@
22

33
import org.jspecify.annotations.Nullable;
44
import org.quiltmc.enigma.api.Enigma;
5-
import org.quiltmc.enigma.api.analysis.index.jar.EntryIndex;
65
import org.quiltmc.enigma.api.analysis.index.jar.JarIndex;
76
import org.quiltmc.enigma.api.service.NameProposalService;
87
import org.quiltmc.enigma.api.source.TokenType;
98
import org.quiltmc.enigma.api.translation.mapping.EntryMapping;
109
import org.quiltmc.enigma.api.translation.mapping.EntryRemapper;
11-
import org.quiltmc.enigma.api.translation.representation.entry.ClassDefEntry;
12-
import org.quiltmc.enigma.api.translation.representation.entry.ClassEntry;
10+
import org.quiltmc.enigma.api.translation.mapping.tree.EntryTreeNode;
1311
import org.quiltmc.enigma.api.translation.representation.entry.Entry;
1412
import org.quiltmc.enigma.api.translation.representation.entry.FieldEntry;
1513
import org.quiltmc.enigma.api.translation.representation.entry.MethodEntry;
1614

1715
import java.util.HashMap;
18-
import java.util.List;
1916
import java.util.Map;
2017

2118
public record RecordComponentProposalService(RecordIndexingVisitor visitor) implements NameProposalService {
@@ -29,14 +26,18 @@ public Map<Entry<?>, EntryMapping> getProposedNames(Enigma enigma, JarIndex inde
2926

3027
@Nullable
3128
@Override
32-
public Map<Entry<?>, EntryMapping> getDynamicProposedNames(EntryRemapper remapper, @Nullable Entry<?> obfEntry, @Nullable EntryMapping oldMapping, @Nullable EntryMapping newMapping) {
29+
public Map<Entry<?>, EntryMapping> getDynamicProposedNames(
30+
EntryRemapper remapper, @Nullable Entry<?> obfEntry, @Nullable EntryMapping oldMapping,
31+
@Nullable EntryMapping newMapping
32+
) {
3333
if (obfEntry instanceof FieldEntry fieldEntry) {
34-
return this.mapRecordComponentGetter(remapper, fieldEntry.getContainingClass(), fieldEntry, newMapping);
34+
return this.mapRecordComponentGetter(fieldEntry, newMapping);
3535
} else if (obfEntry == null) {
36-
Map<Entry<?>, EntryMapping> mappings = new HashMap<>();
37-
for (var mapping : remapper.getMappings()) {
36+
final Map<Entry<?>, EntryMapping> mappings = new HashMap<>();
37+
for (final EntryTreeNode<EntryMapping> mapping : remapper.getMappings()) {
3838
if (mapping.getEntry() instanceof FieldEntry fieldEntry) {
39-
var getter = this.mapRecordComponentGetter(remapper, fieldEntry.getContainingClass(), fieldEntry, mapping.getValue());
39+
final Map<Entry<?>, EntryMapping> getter =
40+
this.mapRecordComponentGetter(fieldEntry, mapping.getValue());
4041
if (getter != null) {
4142
mappings.putAll(getter);
4243
}
@@ -50,34 +51,17 @@ public Map<Entry<?>, EntryMapping> getDynamicProposedNames(EntryRemapper remappe
5051
}
5152

5253
@Nullable
53-
private Map<Entry<?>, EntryMapping> mapRecordComponentGetter(EntryRemapper remapper, ClassEntry parent, FieldEntry obfFieldEntry, EntryMapping mapping) {
54-
EntryIndex entryIndex = remapper.getJarIndex().getIndex(EntryIndex.class);
55-
ClassDefEntry parentDef = entryIndex.getDefinition(parent);
56-
var def = entryIndex.getDefinition(obfFieldEntry);
57-
if ((parentDef != null && !parentDef.isRecord()) || (def != null && def.getAccess().isStatic())) {
58-
return null;
59-
}
60-
61-
List<MethodEntry> obfClassMethods = remapper.getJarIndex().getChildrenByClass().get(parentDef).stream()
62-
.filter(e -> e instanceof MethodEntry)
63-
.map(e -> (MethodEntry) e)
64-
.toList();
65-
66-
MethodEntry obfMethodEntry = null;
67-
for (MethodEntry method : obfClassMethods) {
68-
if (this.isGetter(obfFieldEntry, method)) {
69-
obfMethodEntry = method;
70-
break;
71-
}
72-
}
73-
74-
if (obfMethodEntry == null) {
54+
private Map<Entry<?>, EntryMapping> mapRecordComponentGetter(FieldEntry obfFieldEntry, EntryMapping mapping) {
55+
final MethodEntry obfGetter = this.visitor.getComponentGetter(obfFieldEntry);
56+
if (obfGetter == null) {
7557
return null;
7658
}
7759

7860
// remap method to match field
79-
EntryMapping newMapping = mapping.tokenType() == TokenType.OBFUSCATED ? new EntryMapping(null, null, TokenType.OBFUSCATED, null) : this.createMapping(mapping.targetName(), TokenType.DYNAMIC_PROPOSED);
80-
return Map.of(obfMethodEntry, newMapping);
61+
final EntryMapping getterMapping = mapping.tokenType() == TokenType.OBFUSCATED
62+
? EntryMapping.OBFUSCATED
63+
: this.createMapping(mapping.targetName(), TokenType.DYNAMIC_PROPOSED);
64+
return Map.of(obfGetter, getterMapping);
8165
}
8266

8367
@Override
@@ -89,11 +73,6 @@ public void validateProposedMapping(Entry<?> entry, EntryMapping mapping, boolea
8973
NameProposalService.super.validateProposedMapping(entry, mapping, dynamic);
9074
}
9175

92-
public boolean isGetter(FieldEntry obfFieldEntry, MethodEntry method) {
93-
final MethodEntry getter = this.visitor.getComponentGetter(obfFieldEntry);
94-
return getter != null && getter.equals(method);
95-
}
96-
9776
@Override
9877
public String getId() {
9978
return ID;

enigma/src/main/java/org/quiltmc/enigma/impl/plugin/RecordIndexingVisitor.java

Lines changed: 120 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
import com.google.common.collect.BiMap;
44
import com.google.common.collect.HashBiMap;
55
import com.google.common.collect.HashMultimap;
6+
import com.google.common.collect.ImmutableSet;
67
import com.google.common.collect.Multimap;
78
import org.jspecify.annotations.Nullable;
89
import org.objectweb.asm.ClassVisitor;
@@ -23,40 +24,74 @@
2324
import org.quiltmc.enigma.api.translation.representation.entry.MethodEntry;
2425

2526
import java.util.HashSet;
27+
import java.util.List;
2628
import java.util.Set;
2729
import java.util.stream.Stream;
2830

31+
// TODO add tests
32+
// TODO javadoc, including getter uncertainty
2933
final class RecordIndexingVisitor extends ClassVisitor {
34+
private static final int REQUIRED_GETTER_ACCESS = Opcodes.ACC_PUBLIC;
35+
private static final int ILLEGAL_GETTER_ACCESS = Opcodes.ACC_SYNTHETIC | Opcodes.ACC_BRIDGE | Opcodes.ACC_STATIC;
36+
37+
private static final ImmutableSet<String> ILLEGAL_GETTER_NAMES = ImmutableSet
38+
.of("clone", "finalize", "getClass", "hashCode", "notify", "notifyAll", "toString", "wait");
39+
40+
// visitation state fields; cleared in visitEnd()
3041
private ClassEntry clazz;
3142
private final Set<RecordComponentNode> recordComponents = new HashSet<>();
32-
private final Set<FieldNode> fields = new HashSet<>();
33-
private final Set<MethodNode> methods = new HashSet<>();
34-
35-
private final BiMap<FieldEntry, MethodEntry> gettersByField;
36-
private final Multimap<ClassEntry, FieldEntry> fieldsByClass = HashMultimap.create();
37-
private final Multimap<ClassEntry, MethodEntry> methodsByClass = HashMultimap.create();
43+
// this is a multimap because inner classes' fields go in the same map as their outer class's
44+
private final Multimap<String, FieldNode> fieldsByName = HashMultimap.create();
45+
private final Multimap<String, MethodNode> methodsByDescriptor = HashMultimap.create();
46+
47+
// index fields; contents publicly queryable
48+
private final Multimap<ClassEntry, FieldEntry> componentFieldsByClass = HashMultimap.create();
49+
// holds methods that are at least probably getters for their field keys; superset of definiteComponentGettersByField
50+
private final BiMap<FieldEntry, MethodEntry> componentGettersByField = HashBiMap.create();
51+
// holds methods that are definitely the getters for their field keys
52+
private final BiMap<FieldEntry, MethodEntry> definiteComponentGettersByField = HashBiMap.create();
53+
// holds methods that are at least probably getters; superset of definiteComponentGettersByClass
54+
private final Multimap<ClassEntry, MethodEntry> componentGettersByClass = HashMultimap.create();
55+
// holds methods that are definitely component getters
56+
private final Multimap<ClassEntry, MethodEntry> definiteComponentGettersByClass = HashMultimap.create();
3857

3958
RecordIndexingVisitor() {
4059
super(Enigma.ASM_VERSION);
41-
this.gettersByField = HashBiMap.create();
4260
}
4361

4462
@Nullable
4563
public MethodEntry getComponentGetter(FieldEntry componentField) {
46-
return this.gettersByField.get(componentField);
64+
return this.componentGettersByField.get(componentField);
4765
}
4866

4967
@Nullable
5068
public FieldEntry getComponentField(MethodEntry componentGetter) {
51-
return this.gettersByField.inverse().get(componentGetter);
69+
return this.componentGettersByField.inverse().get(componentGetter);
70+
}
71+
72+
// TODO javadoc, prevent directly naming method (always match field)
73+
@Nullable
74+
public MethodEntry getDefiniteComponentGetter(FieldEntry componentField) {
75+
return this.definiteComponentGettersByField.get(componentField);
76+
}
77+
78+
// TODO javadoc
79+
@Nullable
80+
public FieldEntry getDefiniteComponentField(MethodEntry componentGetter) {
81+
return this.definiteComponentGettersByField.inverse().get(componentGetter);
5282
}
5383

5484
public Stream<FieldEntry> streamComponentFields(ClassEntry recordEntry) {
55-
return this.fieldsByClass.get(recordEntry).stream();
85+
return this.componentFieldsByClass.get(recordEntry).stream();
5686
}
5787

5888
public Stream<MethodEntry> streamComponentMethods(ClassEntry recordEntry) {
59-
return this.methodsByClass.get(recordEntry).stream();
89+
return this.componentGettersByClass.get(recordEntry).stream();
90+
}
91+
92+
// TODO javadoc
93+
public Stream<MethodEntry> streamDefiniteComponentMethods(ClassEntry recordEntry) {
94+
return this.definiteComponentGettersByClass.get(recordEntry).stream();
6095
}
6196

6297
@Override
@@ -74,8 +109,8 @@ public RecordComponentVisitor visitRecordComponent(final String name, final Stri
74109
@Override
75110
public FieldVisitor visitField(final int access, final String name, final String descriptor, final String signature, final Object value) {
76111
if (this.clazz != null && ((access & Opcodes.ACC_PRIVATE) != 0) && this.recordComponents.stream().anyMatch(component -> component.name.equals(name))) {
77-
FieldNode node = new FieldNode(this.api, access, name, descriptor, signature, value);
78-
this.fields.add(node);
112+
final FieldNode node = new FieldNode(this.api, access, name, descriptor, signature, value);
113+
this.fieldsByName.put(node.name, node);
79114
return node;
80115
}
81116

@@ -85,8 +120,8 @@ public FieldVisitor visitField(final int access, final String name, final String
85120
@Override
86121
public MethodVisitor visitMethod(final int access, final String name, final String descriptor, final String signature, final String[] exceptions) {
87122
if (this.clazz != null && ((access & Opcodes.ACC_PUBLIC) != 0)) {
88-
MethodNode node = new MethodNode(this.api, access, name, descriptor, signature, exceptions);
89-
this.methods.add(node);
123+
final MethodNode node = new MethodNode(this.api, access, name, descriptor, signature, exceptions);
124+
this.methodsByDescriptor.put(node.desc, node);
90125
return node;
91126
}
92127

@@ -103,8 +138,8 @@ public void visitEnd() {
103138
} finally {
104139
this.clazz = null;
105140
this.recordComponents.clear();
106-
this.fields.clear();
107-
this.methods.clear();
141+
this.fieldsByName.clear();
142+
this.methodsByDescriptor.clear();
108143
}
109144
}
110145

@@ -113,43 +148,74 @@ private void collectResults() {
113148
return;
114149
}
115150

116-
for (RecordComponentNode component : this.recordComponents) {
117-
FieldNode field = null;
118-
for (FieldNode node : this.fields) {
119-
if (node.name.equals(component.name) && node.desc.equals(component.descriptor)) {
120-
field = node;
121-
break;
122-
}
123-
}
124-
125-
if (field == null) {
126-
throw new RuntimeException("Field not found for record component: " + component.name);
127-
}
128-
129-
for (MethodNode method : this.methods) {
130-
InsnList instructions = method.instructions;
131-
132-
// match bytecode to exact expected bytecode for a getter
133-
// only check important instructions (ignore new frame instructions, etc.)
134-
if (
135-
instructions.size() == 6
136-
&& instructions.get(2).getOpcode() == Opcodes.ALOAD
137-
&& instructions.get(3) instanceof FieldInsnNode fieldInsn
138-
&& fieldInsn.getOpcode() == Opcodes.GETFIELD
139-
&& fieldInsn.owner.equals(this.clazz.getFullName())
140-
&& fieldInsn.desc.equals(field.desc)
141-
&& fieldInsn.name.equals(field.name)
142-
&& instructions.get(4).getOpcode() >= Opcodes.IRETURN
143-
&& instructions.get(4).getOpcode() <= Opcodes.ARETURN
144-
) {
145-
final FieldEntry fieldEntry = new FieldEntry(this.clazz, field.name, new TypeDescriptor(field.desc));
146-
final MethodEntry methodEntry = new MethodEntry(this.clazz, method.name, new MethodDescriptor(method.desc));
147-
148-
this.gettersByField.put(fieldEntry, methodEntry);
149-
this.fieldsByClass.put(this.clazz, fieldEntry);
150-
this.methodsByClass.put(this.clazz, methodEntry);
151-
}
152-
}
151+
this.recordComponents.stream()
152+
.map(component -> this.fieldsByName.get(component.name).stream()
153+
.filter(field -> field.desc.equals(component.descriptor))
154+
.findAny()
155+
.orElseThrow(() -> new IllegalStateException(
156+
"Field not found for record component: " + component.name
157+
))
158+
)
159+
.forEach(field -> {
160+
final List<MethodNode> potentialGetters = this.methodsByDescriptor
161+
.get("()" + field.desc)
162+
.stream()
163+
.filter(method -> (method.access & REQUIRED_GETTER_ACCESS) == REQUIRED_GETTER_ACCESS)
164+
.filter(method -> (method.access & ILLEGAL_GETTER_ACCESS) == 0)
165+
.filter(method -> !ILLEGAL_GETTER_NAMES.contains(method.name))
166+
.toList();
167+
168+
if (potentialGetters.isEmpty()) {
169+
throw new IllegalStateException("No potential getters for field: " + field);
170+
} else {
171+
final FieldEntry fieldEntry =
172+
new FieldEntry(this.clazz, field.name, new TypeDescriptor(field.desc));
173+
// index the field even if a corresponding getter can't be found
174+
this.componentFieldsByClass.put(this.clazz, fieldEntry);
175+
176+
if (potentialGetters.size() == 1) {
177+
this.indexGetter(potentialGetters.get(0), fieldEntry, true);
178+
} else {
179+
// If there are multiple methods with the getter's descriptor and access, it's impossible to
180+
// tell which is the getter because obfuscation can mismatch getter/field names.
181+
// This matching produces as few false-positives as possible by matching name, descriptor,
182+
// and the bytecode of a default (non-overriden) getter method.
183+
// It can still give a false-positive if a non-getter method's obfuscated name matches the
184+
// field's, and that non-getter the has expected descriptor and bytecode of the getter.
185+
// It also has false-negatives for getter overrides with non-default bytecode.
186+
potentialGetters.stream()
187+
.filter(method -> method.name.equals(field.name))
188+
// match bytecode to exact expected bytecode for a getter
189+
// only check important instructions (ignore new frame instructions, etc.)
190+
.filter(method -> {
191+
final InsnList instructions = method.instructions;
192+
return instructions.size() == 6
193+
&& instructions.get(2).getOpcode() == Opcodes.ALOAD
194+
&& instructions.get(3) instanceof FieldInsnNode fieldInsn
195+
&& fieldInsn.getOpcode() == Opcodes.GETFIELD
196+
&& fieldInsn.owner.equals(this.clazz.getFullName())
197+
&& fieldInsn.desc.equals(field.desc)
198+
&& fieldInsn.name.equals(field.name)
199+
&& instructions.get(4).getOpcode() >= Opcodes.IRETURN
200+
&& instructions.get(4).getOpcode() <= Opcodes.ARETURN;
201+
})
202+
.findAny()
203+
.ifPresent(getter -> this.indexGetter(getter, fieldEntry, false));
204+
}
205+
}
206+
});
207+
}
208+
209+
private void indexGetter(MethodNode getterNode, FieldEntry fieldEntry, boolean definite) {
210+
final MethodEntry getterEntry =
211+
new MethodEntry(this.clazz, getterNode.name, new MethodDescriptor(getterNode.desc));
212+
213+
this.componentGettersByField.put(fieldEntry, getterEntry);
214+
this.componentGettersByClass.put(this.clazz, getterEntry);
215+
216+
if (definite) {
217+
this.definiteComponentGettersByField.put(fieldEntry, getterEntry);
218+
this.definiteComponentGettersByClass.put(this.clazz, getterEntry);
153219
}
154220
}
155221
}

0 commit comments

Comments
 (0)