diff --git a/src/main/java/net/fabricmc/loom/util/function/CollectionUtil.java b/src/main/java/net/fabricmc/loom/util/function/CollectionUtil.java index 389dbbef..b7f37e33 100644 --- a/src/main/java/net/fabricmc/loom/util/function/CollectionUtil.java +++ b/src/main/java/net/fabricmc/loom/util/function/CollectionUtil.java @@ -1,7 +1,7 @@ /* * This file is part of fabric-loom, licensed under the MIT License (MIT). * - * Copyright (c) 2020-2021 FabricMC + * Copyright (c) 2020-2022 FabricMC * * Permission is hereby granted, free of charge, to any person obtaining a copy * of this software and associated documentation files (the "Software"), to deal @@ -25,6 +25,7 @@ package net.fabricmc.loom.util.function; import java.util.ArrayList; +import java.util.Collection; import java.util.List; import java.util.Optional; import java.util.function.Function; @@ -72,4 +73,18 @@ public final class CollectionUtil { return result; } + + /** + * Filters a list based on a predicate. + * + * @param collection the source collection + * @param filter the filtering predicate + * @param the element type + * @return a mutable list with the filtered entries + */ + public static List filter(Collection collection, Predicate filter) { + ArrayList result = new ArrayList<>(collection); + result.removeIf(filter.negate()); + return result; + } } diff --git a/src/main/java/net/fabricmc/loom/util/srg/SrgMerger.java b/src/main/java/net/fabricmc/loom/util/srg/SrgMerger.java index f3714b23..0db8bc80 100644 --- a/src/main/java/net/fabricmc/loom/util/srg/SrgMerger.java +++ b/src/main/java/net/fabricmc/loom/util/srg/SrgMerger.java @@ -28,10 +28,17 @@ import java.io.BufferedReader; import java.io.IOException; import java.nio.file.Files; import java.nio.file.Path; +import java.util.ArrayList; +import java.util.HashSet; +import java.util.List; import java.util.Map; +import java.util.Set; +import java.util.function.Consumer; import java.util.stream.Collectors; import java.util.stream.Stream; +import com.google.common.collect.ArrayListMultimap; +import com.google.common.collect.ListMultimap; import org.jetbrains.annotations.Nullable; import net.fabricmc.loom.api.mappings.layered.MappingsNamespace; @@ -50,17 +57,19 @@ import net.fabricmc.mappingio.tree.MappingTreeView; import net.fabricmc.mappingio.tree.MemoryMappingTree; /** - * Utilities for merging SRG mappings. + * Merges a Tiny file with an SRG file. * * @author Juuz */ public final class SrgMerger { + private static final List INPUT_NAMESPACES = List.of("official", "intermediary", "named"); private final MemoryMappingTree srg; private final MemoryMappingTree src; private final MemoryMappingTree output; private final FlatMappingVisitor flatOutput; private final boolean lenient; private final @Nullable MemoryMappingTree extra; + private final ListMultimap methodsBySrgName; private SrgMerger(Path srg, Path tiny, @Nullable ExtraMappings extraMappings, boolean lenient) throws IOException { this.srg = readSrg(srg); @@ -68,6 +77,7 @@ public final class SrgMerger { this.output = new MemoryMappingTree(); this.flatOutput = new RegularAsFlatMappingVisitor(output); this.lenient = lenient; + this.methodsBySrgName = ArrayListMultimap.create(); if (extraMappings != null) { this.extra = new MemoryMappingTree(); @@ -88,14 +98,20 @@ public final class SrgMerger { } MappingReader.read(tiny, this.src); - - if (!"official".equals(this.src.getSrcNamespace())) { - throw new MappingException("Mapping file " + tiny + " does not have the 'official' namespace as the default!"); - } + checkInputNamespaces(tiny); this.output.visitNamespaces(this.src.getSrcNamespace(), Stream.concat(Stream.of("srg"), this.src.getDstNamespaces().stream()).collect(Collectors.toList())); } + private void checkInputNamespaces(Path tiny) { + List inputNamespaces = new ArrayList<>(this.src.getDstNamespaces()); + inputNamespaces.add(0, this.src.getSrcNamespace()); + + if (!inputNamespaces.equals(INPUT_NAMESPACES)) { + throw new MappingException("Mapping file " + tiny + " does not have 'official, intermediary, named' as its namespaces! Found: " + inputNamespaces); + } + } + /** * Creates a destination name array where the first element * will be the srg name of the mapping element. @@ -149,6 +165,8 @@ public final class SrgMerger { } } + resolveConflicts(); + return output; } @@ -184,6 +202,7 @@ public final class SrgMerger { private void mergeMethod(MappingTree.ClassMapping srgClass, MappingTree.MethodMapping srgMethod, @Nullable MappingTree.ClassMapping tinyClass) throws IOException { String[] dstNames = createDstNameArray(srgMethod); MappingTree.MethodMapping tinyMethod = null; + String intermediaryName, namedName; if (tinyClass != null) { tinyMethod = tinyClass.getMethod(srgMethod.getSrcName(), srgMethod.getSrcDesc()); @@ -193,6 +212,8 @@ public final class SrgMerger { if (tinyMethod != null) { copyDstNames(dstNames, tinyMethod); + intermediaryName = tinyMethod.getName("intermediary"); + namedName = tinyMethod.getName("named"); } else { if (srgMethod.getSrcName().equals(srgMethod.getDstName(0))) { // These are only methods like or toString which have the same name in every NS. @@ -212,6 +233,7 @@ public final class SrgMerger { if (fillMethod != null) { fillMappings(dstNames, fillMethod); + intermediaryName = namedName = fillMethod.getSrcName(); } else { // Do not allow missing methods as these are typically subclass methods and cause issues where // class B extends A, and overrides a method from the superclass. Then the subclass method @@ -220,12 +242,97 @@ public final class SrgMerger { } } + if (!srgMethod.getSrcName().equals(dstNames[0])) { // ignore and the likes + methodsBySrgName.put( + new SrgMethodKey(dstNames[0], srgMethod.getSrcDesc()), + new MethodData(srgClass.getSrcName(), srgMethod.getSrcName(), srgMethod.getSrcDesc(), tinyMethod != null, intermediaryName, namedName) + ); + } + flatOutput.visitMethod(srgClass.getSrcName(), srgMethod.getSrcName(), srgMethod.getSrcDesc(), dstNames); } + /** + * Resolves conflicts where multiple methods map to an SRG method. + * We will prefer the ones with the Tiny mappings. + */ + private void resolveConflicts() { + List conflicts = new ArrayList<>(); + + for (SrgMethodKey srg : methodsBySrgName.keySet()) { + List methods = methodsBySrgName.get(srg); + if (methods.size() == 1) continue; + + // Determine whether the names conflict + Set foundNamedNames = new HashSet<>(); + + for (MethodData method : methods) { + foundNamedNames.add(method.namedName()); + } + + if (foundNamedNames.size() == 1) { + // No conflict, go on + continue; + } + + // Find preferred method + @Nullable MethodData preferred = findPreferredMethod(methods, conflicts::add); + if (preferred == null) continue; + + // Remove non-preferred methods + for (MethodData method : methods) { + if (method != preferred) { + MappingTree.ClassMapping clazz = output.getClass(method.obfOwner()); + clazz.getMethods().removeIf(m -> m.getSrcName().equals(method.obfName()) && m.getSrcDesc().equals(method.obfDesc())); + } + } + } + + if (!conflicts.isEmpty()) { + throw new MappingException("Unfixable conflicts:\n" + String.join("\n", conflicts)); + } + } + + private @Nullable MethodData findPreferredMethod(List methods, Consumer conflictReporter) { + List hasTiny = CollectionUtil.filter(methods, MethodData::hasTiny); + + // Record conflicts if needed + if (hasTiny.size() > 1) { // Multiple methods map to this SRG name + // Sometimes unrelated methods share an SRG name. Probably overrides from a past version? + Set intermediaryNames = new HashSet<>(); + + for (MethodData method : methods) { + intermediaryNames.add(method.intermediaryName()); + } + + // Only record a conflict if we map one intermediary name with multiple named names + if (intermediaryNames.size() == 1) { + StringBuilder message = new StringBuilder(); + message.append("- multiple preferred methods for ").append(srg).append(':'); + + for (MethodData preferred : hasTiny) { + message.append("\n\t> ").append(preferred); + } + + conflictReporter.accept(message.toString()); + } + + return null; + } else if (hasTiny.isEmpty()) { // No methods map to this SRG name + conflictReporter.accept("- no preferred methods found for " + srg + ", available: " + methods); + return null; + } + + return hasTiny.get(0); + } + /** * Merges SRG mappings with a tiny mappings tree through the obf names. * + *

The namespaces in the tiny file should be {@code official, intermediary, named}. + * The SRG names will add a new namespace called {@code srg} so that the final namespaces + * are {@code official, srg, intermediary, named}. + * *

This method does not care about method parameters, local variables or javadoc comments. * As such, it shouldn't be used for remapping the game jar. * @@ -239,7 +346,7 @@ public final class SrgMerger { * whether an unobfuscated name is needed in the result file * @param lenient whether lenient mode is enabled * @throws IOException if an IO error occurs while reading or writing the mappings - * @throws MappingException if the input tiny tree's default namespace is not 'official' + * @throws MappingException if the input tiny mappings' namespaces are incorrect * or if an element mentioned in the SRG file does not have tiny mappings in non-lenient mode */ public static void mergeSrg(Path srg, Path tiny, Path out, @Nullable ExtraMappings extraMappings, boolean lenient) @@ -276,4 +383,18 @@ public final class SrgMerger { return new ExtraMappings(path, MappingFormat.TSRG2, MappingsNamespace.OFFICIAL.toString(), MappingsNamespace.NAMED.toString()); } } + + private record SrgMethodKey(String name, String desc) { + @Override + public String toString() { + return name + desc; + } + } + + private record MethodData(String obfOwner, String obfName, String obfDesc, boolean hasTiny, String intermediaryName, String namedName) { + @Override + public String toString() { + return "%s.%s%s => %s/%s (%s)".formatted(obfOwner, obfName, obfDesc, intermediaryName, namedName, hasTiny ? "tiny" : "filled"); + } + } } diff --git a/src/test/resources/forge/testSrg/expectedOutput.tiny b/src/test/resources/forge/testSrg/expectedOutput.tiny index a44421aa..6fae24e8 100644 --- a/src/test/resources/forge/testSrg/expectedOutput.tiny +++ b/src/test/resources/forge/testSrg/expectedOutput.tiny @@ -2,6 +2,11 @@ tiny 2 0 official srg intermediary named c a test/FakeClass class_1 test/SomeClass f I a f_1_ field_1 myInt m ()I a m_1_ method_1 getMyInt + m ()V evilUnobfMethod m_3_ method_2 differentNamedName c b test/FakeSubclass class_2 test/SomeSubclass c test/DeObfClass test/DeObfClass test/DeObfClass test/DeObfClass m ()V iAmNotObfuscated m_2_ iAmNotObfuscated iAmNotObfuscated +c c test/FormerSiblingA class_3 test/RelatedA + m ()V a m_4_ method_3 fromClassA +c d test/FormerSiblingB class_4 test/RelatedB + m ()V a m_4_ method_4 fromClassB diff --git a/src/test/resources/forge/testSrg/extraInput.tsrg b/src/test/resources/forge/testSrg/extraInput.tsrg index 4cbffd52..cc395b84 100644 --- a/src/test/resources/forge/testSrg/extraInput.tsrg +++ b/src/test/resources/forge/testSrg/extraInput.tsrg @@ -3,8 +3,13 @@ a test/FakeClass ()V a I fakeField a ()I getFakeField + evilUnobfMethod ()V evilUnobfMethod b test/FakeSubclass ()V test/DeObfClass test/DeObfClass ()V iAmNotObfuscated ()V iAmNotObfuscated +c test/FormerSiblingA + a ()V something +d test/FormerSiblingB + a ()V something diff --git a/src/test/resources/forge/testSrg/proguard.txt b/src/test/resources/forge/testSrg/proguard.txt index 380352df..fa14cc07 100644 --- a/src/test/resources/forge/testSrg/proguard.txt +++ b/src/test/resources/forge/testSrg/proguard.txt @@ -2,8 +2,14 @@ test.FakeClass -> a: void () -> int fakeField -> a int getFakeField() -> a + void evilUnobfMethod() -> evilUnobfMethod test.FakeSubclass -> b: void () -> + void evilUnobfMethod() -> evilUnobfMethod test.DeObfClass -> test.DeObfClass: void () -> void iAmNotObfuscated() -> iAmNotObfuscated +test.FormerSiblingA -> c: + void something() -> a +test.FormerSiblingB -> d: + void something() -> a diff --git a/src/test/resources/forge/testSrg/srgInput.tsrg b/src/test/resources/forge/testSrg/srgInput.tsrg index 869f5ea7..8a370613 100644 --- a/src/test/resources/forge/testSrg/srgInput.tsrg +++ b/src/test/resources/forge/testSrg/srgInput.tsrg @@ -3,9 +3,15 @@ a test/FakeClass ()V a I f_1_ a ()I m_1_ + evilUnobfMethod ()V m_3_ b test/FakeSubclass ()V a ()I m_1_ + evilUnobfMethod ()V m_3_ test/DeObfClass test/DeObfClass ()V iAmNotObfuscated ()V m_2_ +c test/FormerSiblingA + a ()V m_4_ +d test/FormerSiblingB + a ()V m_4_ diff --git a/src/test/resources/forge/testSrg/tinyInput.tiny b/src/test/resources/forge/testSrg/tinyInput.tiny index 9159851d..91352e94 100644 --- a/src/test/resources/forge/testSrg/tinyInput.tiny +++ b/src/test/resources/forge/testSrg/tinyInput.tiny @@ -2,4 +2,9 @@ tiny 2 0 official intermediary named c a class_1 test/SomeClass f I a field_1 myInt m ()I a method_1 getMyInt + m ()V evilUnobfMethod method_2 differentNamedName c b class_2 test/SomeSubclass +c c class_3 test/RelatedA + m ()V a method_3 fromClassA +c d class_4 test/RelatedB + m ()V a method_4 fromClassB