Skip to content

Commit

Permalink
Fix incorrect handling of class repackaging in JOBF implementation (#118
Browse files Browse the repository at this point in the history 
)

Also refactors the test mapping generation to accommodate the added class repackaging renames.
  • Loading branch information
@NebelNidas
NebelNidas authored Dec 30, 2024
1 parent 7f7f7f4 commit 72e44db
Show file tree
Hide file tree
Showing 77 changed files with 754 additions and 566 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- Made merging with flipped namespaces actually work and handle both names and descriptors
- Fixed potentially incorrect descriptor computation by delaying until all classes are present and merged
- Fixed duplicate mapping definitions not being handled correctly in multiple readers
- Fixed incorrect handling of class repackaging in JOBF reader and writer
- Removed ASM dependency from core project

## [0.6.1] - 2024-04-15
Expand Down
66 changes: 53 additions & 13 deletions mapping-io-extras/src/test/java/net/fabricmc/mappingio/extras/MappingTreeRemapperTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,58 +27,98 @@

import net.fabricmc.mappingio.TestHelper;
import net.fabricmc.mappingio.tree.MappingTree;
import net.fabricmc.mappingio.tree.MappingTree.ClassMapping;
import net.fabricmc.mappingio.tree.MappingTree.FieldMapping;
import net.fabricmc.mappingio.tree.MappingTree.MethodMapping;
import net.fabricmc.mappingio.tree.MemoryMappingTree;

public class MappingTreeRemapperTest {
private static final String cls1SrcName = "class_1";
private static final String cls3SrcName = "class_3";
private static final String fld1SrcName = "field_1";
private static final String mth1SrcName = "method_1";
private static String srcNs;
private static String dstNs;
private static String cls1DstName;
private static String cls3DstName;
private static String fld1SrcDesc;
private static String fld1DstName;
private static String mth1SrcDesc;
private static String mth1DstName;
private static MappingTree mappingTree;
private static MappingTreeRemapper remapper;

@BeforeAll
public static void setup() throws IOException {
mappingTree = TestHelper.acceptTestMappings(new MemoryMappingTree());
remapper = new MappingTreeRemapper(mappingTree, "source", "target");
srcNs = mappingTree.getSrcNamespace();
dstNs = mappingTree.getDstNamespaces().get(0);

remapper = new MappingTreeRemapper(mappingTree, srcNs, dstNs);

ClassMapping cls1 = mappingTree.getClass(cls1SrcName);
ClassMapping cls3 = mappingTree.getClass(cls3SrcName);
cls1DstName = cls1.getDstName(0);
cls3DstName = cls3.getDstName(0);

FieldMapping fld1 = cls1.getField(fld1SrcName, null);
fld1SrcDesc = fld1.getSrcDesc();
fld1DstName = fld1.getDstName(0);

MethodMapping mth1 = cls1.getMethod(mth1SrcName, null);
mth1SrcDesc = mth1.getSrcDesc();
mth1DstName = mth1.getDstName(0);
}

@Test
public void testInvalidNamespaces() {
assertThrows(IllegalArgumentException.class, () -> new MappingTreeRemapper(mappingTree, "unknown", "target"),
assertThrows(IllegalArgumentException.class, () -> new MappingTreeRemapper(mappingTree, "unknown", dstNs),
"must throw on missing source namespace");
assertThrows(IllegalArgumentException.class, () -> new MappingTreeRemapper(mappingTree, "source", "unknown"),
assertThrows(IllegalArgumentException.class, () -> new MappingTreeRemapper(mappingTree, srcNs, "unknown"),
"must throw on missing target namespace");
}

@Test
public void testMapClass() {
assertEquals("class1Ns0Rename", remapper.map("class_1"));
assertEquals(cls1DstName, remapper.map(cls1SrcName));
}

@Test
public void testMapMethod() {
assertEquals("method1Ns0Rename", remapper.mapMethodName("class_1", "method_1", "()I"));
assertEquals(mth1DstName, remapper.mapMethodName(cls1SrcName, mth1SrcName, mth1SrcDesc));
}

@Test
public void testMapField() {
assertEquals("field1Ns0Rename", remapper.mapFieldName("class_1", "field_1", "I"));
assertEquals(fld1DstName, remapper.mapFieldName(cls1SrcName, fld1SrcName, fld1SrcDesc));
}

@Test
public void testMapRecordComponent() {
// Record components are remapped as fields.
assertEquals("field1Ns0Rename", remapper.mapRecordComponentName("class_1", "field_1", "I"));
assertEquals(fld1DstName, remapper.mapRecordComponentName(cls1SrcName, fld1SrcName, fld1SrcDesc));
}

@Test
public void testMapDesc() {
assertEquals("Lclass3Ns0Rename;", remapper.mapDesc("Lclass_3;"));
assertEquals("()Lclass1Ns0Rename;", remapper.mapMethodDesc("()Lclass_1;"));
assertEquals(clsDesc(cls1DstName), remapper.mapDesc(clsDesc(cls1SrcName)));
assertEquals(mthDesc(cls3DstName), remapper.mapMethodDesc(mthDesc(cls3SrcName)));
}

@Test
public void testMapType() {
Type fieldType = Type.getType("Lclass_3;");
Type methodType = Type.getMethodType("()Lclass_1;");
assertEquals(Type.getType("Lclass3Ns0Rename;"), remapper.mapValue(fieldType));
assertEquals(Type.getMethodType("()Lclass1Ns0Rename;"), remapper.mapValue(methodType));
Type fieldType = Type.getType(clsDesc(cls3SrcName));
Type methodType = Type.getMethodType(mthDesc(cls1SrcName));

assertEquals(Type.getType(clsDesc(cls3DstName)), remapper.mapValue(fieldType));
assertEquals(Type.getMethodType(mthDesc(cls1DstName)), remapper.mapValue(methodType));
}

private String clsDesc(String name) {
return "L" + name + ";";
}

private String mthDesc(String name) {
return "()L" + name + ";";
}
}
6 changes: 5 additions & 1 deletion src/main/java/net/fabricmc/mappingio/format/FeatureSet.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ public interface FeatureSet {
MetadataSupport fileMetadata();
MetadataSupport elementMetadata();
NameSupport packages();
NameSupport classes();
ClassSupport classes();
MemberSupport fields();
MemberSupport methods();
LocalSupport args();
Expand Down Expand Up @@ -82,6 +82,10 @@ interface DescSupport {
FeaturePresence dstDescs();
}

interface ClassSupport extends NameSupport {
boolean hasRepackaging();
}

interface MemberSupport extends NameSupport, DescSupport {
}

Expand Down
53 changes: 48 additions & 5 deletions src/main/java/net/fabricmc/mappingio/format/FeatureSetBuilder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,15 @@

import org.jetbrains.annotations.ApiStatus;

import net.fabricmc.mappingio.format.FeatureSet.ClassSupport;
import net.fabricmc.mappingio.format.FeatureSet.DescSupport;
import net.fabricmc.mappingio.format.FeatureSet.ElementCommentSupport;
import net.fabricmc.mappingio.format.FeatureSet.FeaturePresence;
import net.fabricmc.mappingio.format.FeatureSet.LocalSupport;
import net.fabricmc.mappingio.format.FeatureSet.MemberSupport;
import net.fabricmc.mappingio.format.FeatureSet.MetadataSupport;
import net.fabricmc.mappingio.format.FeatureSet.NameSupport;
import net.fabricmc.mappingio.format.FeatureSetImpl.ClassSupportImpl;
import net.fabricmc.mappingio.format.FeatureSetImpl.DescSupportImpl;
import net.fabricmc.mappingio.format.FeatureSetImpl.LocalSupportImpl;
import net.fabricmc.mappingio.format.FeatureSetImpl.MemberSupportImpl;
Expand All @@ -44,7 +46,7 @@ public static FeatureSetBuilder createFrom(FeatureSet featureSet) {
featureSet.fileMetadata(),
featureSet.elementMetadata(),
new NameFeatureBuilder(featureSet.packages()),
new NameFeatureBuilder(featureSet.classes()),
new ClassSupportBuilder(featureSet.classes()),
new MemberSupportBuilder(featureSet.fields()),
new MemberSupportBuilder(featureSet.methods()),
new LocalSupportBuilder(featureSet.args()),
Expand All @@ -58,7 +60,7 @@ public static FeatureSetBuilder createFrom(FeatureSet featureSet) {
initWithFullSupport ? MetadataSupport.ARBITRARY : MetadataSupport.NONE,
initWithFullSupport ? MetadataSupport.ARBITRARY : MetadataSupport.NONE,
new NameFeatureBuilder(initWithFullSupport),
new NameFeatureBuilder(initWithFullSupport),
new ClassSupportBuilder(initWithFullSupport),
new MemberSupportBuilder(initWithFullSupport),
new MemberSupportBuilder(initWithFullSupport),
new LocalSupportBuilder(initWithFullSupport),
Expand All @@ -67,7 +69,7 @@ public static FeatureSetBuilder createFrom(FeatureSet featureSet) {
initWithFullSupport);
}

FeatureSetBuilder(boolean hasNamespaces, MetadataSupport fileMetadata, MetadataSupport elementMetadata, NameFeatureBuilder packages, NameFeatureBuilder classes, MemberSupportBuilder fields, MemberSupportBuilder methods, LocalSupportBuilder args, LocalSupportBuilder vars, ElementCommentSupport elementComments, boolean hasFileComments) {
FeatureSetBuilder(boolean hasNamespaces, MetadataSupport fileMetadata, MetadataSupport elementMetadata, NameFeatureBuilder packages, ClassSupportBuilder classes, MemberSupportBuilder fields, MemberSupportBuilder methods, LocalSupportBuilder args, LocalSupportBuilder vars, ElementCommentSupport elementComments, boolean hasFileComments) {
this.hasNamespaces = hasNamespaces;
this.fileMetadata = fileMetadata;
this.elementMetadata = elementMetadata;
Expand Down Expand Up @@ -101,7 +103,7 @@ public FeatureSetBuilder withPackages(Consumer<NameFeatureBuilder> featureApplie
return this;
}

public FeatureSetBuilder withClasses(Consumer<NameFeatureBuilder> featureApplier) {
public FeatureSetBuilder withClasses(Consumer<ClassSupportBuilder> featureApplier) {
featureApplier.accept(classes);
return this;
}
Expand Down Expand Up @@ -155,14 +157,55 @@ public FeatureSet build() {
private MetadataSupport fileMetadata;
private MetadataSupport elementMetadata;
private NameFeatureBuilder packages;
private NameFeatureBuilder classes;
private ClassSupportBuilder classes;
private MemberSupportBuilder fields;
private MemberSupportBuilder methods;
private LocalSupportBuilder args;
private LocalSupportBuilder vars;
private ElementCommentSupport elementComments;
private boolean hasFileComments;

public static class ClassSupportBuilder {
ClassSupportBuilder() {
this(false);
}

ClassSupportBuilder(boolean initWithFullSupport) {
this(new NameFeatureBuilder(initWithFullSupport), initWithFullSupport);
}

ClassSupportBuilder(ClassSupport classFeature) {
this(new NameFeatureBuilder(classFeature), classFeature.hasRepackaging());
}

private ClassSupportBuilder(NameFeatureBuilder names, boolean hasRepackaging) {
this.names = names;
this.hasRepackaging = hasRepackaging;
}

public ClassSupportBuilder withSrcNames(FeaturePresence featurePresence) {
names.withSrcNames(featurePresence);
return this;
}

public ClassSupportBuilder withDstNames(FeaturePresence featurePresence) {
names.withDstNames(featurePresence);
return this;
}

public ClassSupportBuilder withRepackaging(boolean value) {
hasRepackaging = value;
return this;
}

public ClassSupport build() {
return new ClassSupportImpl(names.build(), hasRepackaging);
}

private NameFeatureBuilder names;
private boolean hasRepackaging;
}

public static class MemberSupportBuilder {
MemberSupportBuilder() {
this(false);
Expand Down
44 changes: 18 additions & 26 deletions src/main/java/net/fabricmc/mappingio/format/FeatureSetImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
package net.fabricmc.mappingio.format;

class FeatureSetImpl implements FeatureSet {
FeatureSetImpl(boolean hasNamespaces, MetadataSupport fileMetadata, MetadataSupport elementMetadata, NameSupport packages, NameSupport classes, MemberSupport fields, MemberSupport methods, LocalSupport args, LocalSupport vars, ElementCommentSupport elementComments, boolean hasFileComments) {
FeatureSetImpl(boolean hasNamespaces, MetadataSupport fileMetadata, MetadataSupport elementMetadata, NameSupport packages, ClassSupport classes, MemberSupport fields, MemberSupport methods, LocalSupport args, LocalSupport vars, ElementCommentSupport elementComments, boolean hasFileComments) {
this.hasNamespaces = hasNamespaces;
this.fileMetadata = fileMetadata;
this.elementMetadata = elementMetadata;
Expand Down Expand Up @@ -52,7 +52,7 @@ public NameSupport packages() {
}

@Override
public NameSupport classes() {
public ClassSupport classes() {
return classes;
}

Expand Down Expand Up @@ -90,28 +90,32 @@ public boolean hasFileComments() {
private final MetadataSupport fileMetadata;
private final MetadataSupport elementMetadata;
private final NameSupport packages;
private final NameSupport classes;
private final ClassSupport classes;
private final MemberSupport fields;
private final MemberSupport methods;
private final LocalSupport args;
private final LocalSupport vars;
private final ElementCommentSupport elementComments;
private final boolean hasFileComments;

static class MemberSupportImpl implements MemberSupport {
MemberSupportImpl(NameSupport names, DescSupport descriptors) {
this.names = names;
this.descriptors = descriptors;
static class ClassSupportImpl extends NameSupportImpl implements ClassSupport {
ClassSupportImpl(NameSupport names, boolean hasRepackaging) {
super(names.srcNames(), names.dstNames());
this.hasRepackaging = hasRepackaging;
}

@Override
public FeaturePresence srcNames() {
return names.srcNames();
public boolean hasRepackaging() {
return hasRepackaging;
}

@Override
public FeaturePresence dstNames() {
return names.dstNames();
private final boolean hasRepackaging;
}

static class MemberSupportImpl extends NameSupportImpl implements MemberSupport {
MemberSupportImpl(NameSupport names, DescSupport descriptors) {
super(names.srcNames(), names.dstNames());
this.descriptors = descriptors;
}

@Override
Expand All @@ -124,18 +128,17 @@ public FeaturePresence dstDescs() {
return descriptors.dstDescs();
}

private final NameSupport names;
private final DescSupport descriptors;
}

static class LocalSupportImpl implements LocalSupport {
static class LocalSupportImpl extends NameSupportImpl implements LocalSupport {
LocalSupportImpl(FeaturePresence positions, FeaturePresence lvIndices, FeaturePresence lvtRowIndices, FeaturePresence startOpIndices, FeaturePresence endOpIndices, NameSupport names, DescSupport descriptors) {
super(names.srcNames(), names.dstNames());
this.positions = positions;
this.lvIndices = lvIndices;
this.lvtRowIndices = lvtRowIndices;
this.startOpIndices = startOpIndices;
this.endOpIndices = endOpIndices;
this.names = names;
this.descriptors = descriptors;
}

Expand Down Expand Up @@ -164,16 +167,6 @@ public FeaturePresence endOpIndices() {
return endOpIndices;
}

@Override
public FeaturePresence srcNames() {
return names.srcNames();
}

@Override
public FeaturePresence dstNames() {
return names.dstNames();
}

@Override
public FeaturePresence srcDescs() {
return descriptors.srcDescs();
Expand All @@ -189,7 +182,6 @@ public FeaturePresence dstDescs() {
private final FeaturePresence lvtRowIndices;
private final FeaturePresence startOpIndices;
private final FeaturePresence endOpIndices;
private final NameSupport names;
private final DescSupport descriptors;
}

Expand Down
Loading

0 comments on commit 72e44db

Please sign in to comment.