Skip to content

Commit

Permalink
Report for missing method parameter names (#25)
Browse files Browse the repository at this point in the history
  • Loading branch information
Machine-Maker authored Jan 16, 2024
1 parent 495823a commit 29c9add
Show file tree
Hide file tree
Showing 7 changed files with 261 additions and 8 deletions.
28 changes: 27 additions & 1 deletion codebook-lvt/src/main/java/io/papermc/codebook/lvt/LvtNamer.java
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@

package io.papermc.codebook.lvt;

import com.google.inject.Guice;
import com.google.inject.Injector;
import dev.denwav.hypo.asm.AsmClassData;
import dev.denwav.hypo.asm.AsmMethodData;
import dev.denwav.hypo.core.HypoContext;
Expand All @@ -34,7 +36,9 @@
import dev.denwav.hypo.model.data.MethodData;
import dev.denwav.hypo.model.data.types.JvmType;
import dev.denwav.hypo.model.data.types.PrimitiveType;
import io.papermc.codebook.report.ReportType;
import io.papermc.codebook.report.Reports;
import io.papermc.codebook.report.type.MissingMethodParam;
import java.io.IOException;
import java.util.ArrayList;
import java.util.Arrays;
Expand All @@ -57,12 +61,16 @@ public class LvtNamer {

private final MappingSet mappings;
private final LvtTypeSuggester lvtTypeSuggester;
private final Reports reports;
private final Injector reportsInjector;
private final RootLvtSuggester lvtAssignSuggester;

public LvtNamer(final HypoContext context, final MappingSet mappings, final Reports reports) throws IOException {
this.mappings = mappings;
this.lvtTypeSuggester = new LvtTypeSuggester(context);
this.lvtAssignSuggester = new RootLvtSuggester(context, this.lvtTypeSuggester, reports);
this.reports = reports;
this.reportsInjector = Guice.createInjector(reports);
this.lvtAssignSuggester = new RootLvtSuggester(context, this.lvtTypeSuggester, this.reportsInjector);
}

public void processClass(final AsmClassData classData) throws IOException {
Expand Down Expand Up @@ -96,6 +104,7 @@ private void fillNames0(final MethodData method) throws IOException {
// If it does, we need to keep track of the LVTs we inherit
@Nullable AsmMethodData outerMethod = null;
int @Nullable [] outerMethodParamLvtIndices = null;
@Nullable LambdaClosure lambdaClosure = null;
final @Nullable List<LambdaClosure> lambdaCalls = method.get(HypoHydration.LAMBDA_CALLS);
// Only track synthetic, non-synthetic means a method reference which does not behave as a closure (does not
// capture LVT)
Expand All @@ -111,6 +120,7 @@ private void fillNames0(final MethodData method) throws IOException {
continue;
}
outerMethodParamLvtIndices = lambdaCall.getParamLvtIndices();
lambdaClosure = lambdaCall;
// there can only be 1 outer method
break;
}
Expand Down Expand Up @@ -178,6 +188,22 @@ private void fillNames0(final MethodData method) throws IOException {
.getClassMapping(parentClass.name())
.flatMap(c -> c.getMethodMapping(method.name(), method.descriptorText()));

final @Nullable ClassData superClass = parentClass.superClass();

if (this.reports.shouldGenerate(ReportType.MISSING_METHOD_PARAM)) {
this.reportsInjector
.getInstance(MissingMethodParam.class)
.handleCheckingMappings(
method,
parentClass,
superClass,
lambdaCalls,
methodMapping.orElse(null),
outerMethodParamLvtIndices,
lambdaClosure,
localClassClosure);
}

// If there's no LVT table there's nothing for us to process
if (node.localVariables == null) {
// interface / abstract methods don't have LVT
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@
import static io.papermc.codebook.lvt.LvtUtil.toJvmType;

import com.google.inject.AbstractModule;
import com.google.inject.Guice;
import com.google.inject.Injector;
import dev.denwav.hypo.core.HypoContext;
import dev.denwav.hypo.model.data.ClassData;
Expand All @@ -52,7 +51,6 @@
import io.papermc.codebook.lvt.suggestion.context.method.MethodInsnContext;
import io.papermc.codebook.lvt.suggestion.numbers.MthRandomSuggester;
import io.papermc.codebook.lvt.suggestion.numbers.RandomSourceSuggester;
import io.papermc.codebook.report.Reports;
import io.papermc.codebook.report.type.MissingMethodLvtSuggestion;
import java.io.IOException;
import java.util.List;
Expand Down Expand Up @@ -92,10 +90,10 @@ public final class RootLvtSuggester extends AbstractModule implements LvtSuggest
private final List<? extends LvtSuggester> suggesters;

public RootLvtSuggester(
final HypoContext hypoContext, final LvtTypeSuggester lvtTypeSuggester, final Reports reports) {
final HypoContext hypoContext, final LvtTypeSuggester lvtTypeSuggester, final Injector reports) {
this.hypoContext = hypoContext;
this.lvtTypeSuggester = lvtTypeSuggester;
this.injector = Guice.createInjector(this, reports);
this.injector = reports.createChildInjector(this);
this.suggesters = SUGGESTERS.stream().map(this.injector::getInstance).toList();
}

Expand Down
1 change: 1 addition & 0 deletions codebook-reports/build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ dependencies {
implementation(platform(libs.hypo.platform))

api(libs.checker)
api(libs.lorenz)

implementation(libs.bundles.hypo.impl)
implementation(libs.bundles.asm)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,5 +24,5 @@

public enum ReportType {
MISSING_METHOD_LVT_SUGGESTION,
;
MISSING_METHOD_PARAM;
}
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@

import com.google.inject.AbstractModule;
import io.papermc.codebook.report.type.MissingMethodLvtSuggestion;
import io.papermc.codebook.report.type.MissingMethodParam;
import io.papermc.codebook.report.type.Report;
import java.io.IOException;
import java.nio.file.Files;
Expand Down Expand Up @@ -55,7 +56,9 @@ protected void configure() {
public Reports(final Path reportsDir, final Set<ReportType> typesToGenerate) {
this.reportsDir = reportsDir;
this.typesToGenerate = typesToGenerate;
this.reports = Map.of(ReportType.MISSING_METHOD_LVT_SUGGESTION, new MissingMethodLvtSuggestion());
this.reports = Map.of(
ReportType.MISSING_METHOD_LVT_SUGGESTION, new MissingMethodLvtSuggestion(),
ReportType.MISSING_METHOD_PARAM, new MissingMethodParam());
}

public void generateReports() throws IOException {
Expand All @@ -69,6 +72,10 @@ public void generateReports() throws IOException {
}
}

public boolean shouldGenerate(final ReportType reportType) {
return this.typesToGenerate.contains(reportType);
}

@Override
protected void configure() {
this.reports.values().forEach(this::bindReport);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,219 @@
/*
* codebook is a remapper utility for the PaperMC project.
*
* Copyright (c) 2023 Kyle Wood (DenWav)
* Contributors
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
* License as published by the Free Software Foundation;
* version 3 only, no later versions.
*
* This library is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
* Lesser General Public License for more details.
*
* You should have received a copy of the GNU Lesser General Public
* License along with this library; if not, write to the Free Software
* Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301
* USA
*/

package io.papermc.codebook.report.type;

import dev.denwav.hypo.hydrate.generic.HypoHydration;
import dev.denwav.hypo.hydrate.generic.LambdaClosure;
import dev.denwav.hypo.hydrate.generic.LocalClassClosure;
import dev.denwav.hypo.model.data.ClassData;
import dev.denwav.hypo.model.data.ClassKind;
import dev.denwav.hypo.model.data.MethodData;
import java.io.IOException;
import java.util.ArrayList;
import java.util.Comparator;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.concurrent.ConcurrentHashMap;
import java.util.function.IntUnaryOperator;
import java.util.regex.Pattern;
import org.cadixdev.lorenz.model.Mapping;
import org.cadixdev.lorenz.model.MethodMapping;
import org.checkerframework.checker.nullness.qual.Nullable;

public class MissingMethodParam implements Report {

private final Map<ClassData, List<String>> data = new ConcurrentHashMap<>();

private void checkMappings(
final MethodData method,
final @Nullable MethodMapping methodMapping,
final int descriptorParamOffset,
final IntUnaryOperator descriptorToMappingOffset) {
this.checkMappings(method, methodMapping, descriptorParamOffset, descriptorToMappingOffset, null);
}

private void checkMappings(
final MethodData method,
final @Nullable MethodMapping methodMapping,
final int descriptorParamOffset,
final IntUnaryOperator descriptorToMappingOffset,
final @Nullable LambdaClosure lambdaClosure) {
if (method.params().size() == descriptorParamOffset) {
return;
}
if (methodMapping == null
|| (method.params().size() - descriptorParamOffset
> methodMapping.getParameterMappings().size())) {
// != should have been sufficient here, but hypo's CopyMappingsDown for constructors incorrectly applies
// mappings to implicit constructor params
this.reportMissingParam(
method, methodMapping, descriptorParamOffset, descriptorToMappingOffset, lambdaClosure);
}
}

private static final Pattern ANONYMOUS_CLASS = Pattern.compile(".+\\$\\d+$");

private static boolean shouldSkipMapping(
final MethodData method,
final ClassData parentClass,
final @Nullable ClassData superClass,
final @Nullable List<LambdaClosure> lambdaCalls) {
final String name = method.name();
if (name.startsWith("access$") && method.isSynthetic()) {
// never in source
return true;
} else if (name.startsWith("lambda$")
&& method.isSynthetic()
&& (lambdaCalls == null || lambdaCalls.isEmpty())) {
// lambdas that had their use stripped by mojang
return true;
} else {
final String descriptorText = method.descriptorText();
if (superClass != null
&& superClass.name().equals("java/lang/Enum")
&& name.equals("valueOf")
&& descriptorText.startsWith("(Ljava/lang/String;)")) {
// created by the compiler
return true;
} else if (parentClass.is(ClassKind.RECORD)
&& name.equals("equals")
&& descriptorText.equals("(Ljava/lang/Object;)Z")) {
// created by the compiler
return true;
} else if (method.isSynthetic() && method.get(HypoHydration.SYNTHETIC_TARGET) != null) {
// don't trust isBridge, apparently it's not always accurate
return true;
} else {
return false;
}
}
}

private void handleConstructorMappings(
final MethodData method,
final ClassData parentClass,
final @Nullable MethodMapping methodMapping,
final @Nullable LocalClassClosure localClassClosure)
throws IOException {
if (parentClass.is(ClassKind.ENUM)) {
// enum constructors include name and ordinal
this.checkMappings(method, methodMapping, 2, i -> i + 1);
} else {
if (!ANONYMOUS_CLASS.matcher(parentClass.name()).matches()) {
// anonymous classes cannot have constructors in source
if (parentClass.outerClass() != null) {
final int descriptorParamOffset = parentClass.isStaticInnerClass() ? 0 : 1;
if (localClassClosure == null) {
this.checkMappings(method, methodMapping, descriptorParamOffset, i -> i + 1);
} else {
this.checkMappings(
method,
methodMapping,
descriptorParamOffset + localClassClosure.getParamLvtIndices().length,
i -> i + 1);
}
} else {
this.checkMappings(method, methodMapping, 0, i -> i + 1);
}
}
}
}

public void handleCheckingMappings(
final MethodData method,
final ClassData parentClass,
final @Nullable ClassData superClass,
final @Nullable List<LambdaClosure> lambdaCalls,
final @Nullable MethodMapping methodMapping,
final int @Nullable [] outerMethodParamLvtIndices,
final @Nullable LambdaClosure lambdaClosure,
final @Nullable LocalClassClosure localClassClosure)
throws IOException {
if (shouldSkipMapping(method, parentClass, superClass, lambdaCalls)) {
return;
}
if (method.isConstructor()) {
this.handleConstructorMappings(method, parentClass, methodMapping, localClassClosure);
} else {
if (outerMethodParamLvtIndices == null) {
this.checkMappings(method, methodMapping, 0, i -> i + (method.isStatic() ? 0 : 1));
} else {
final int descriptorOffset;
if (!method.isStatic() && outerMethodParamLvtIndices.length > 0 && outerMethodParamLvtIndices[0] == 0) {
descriptorOffset = outerMethodParamLvtIndices.length - 1;
} else {
descriptorOffset = outerMethodParamLvtIndices.length;
}
this.checkMappings(
method, methodMapping, descriptorOffset, i -> i + (method.isStatic() ? 0 : 1), lambdaClosure);
}
}
}

private void reportMissingParam(
final MethodData method,
final @Nullable MethodMapping methodMapping,
final int descriptorParamOffset,
final IntUnaryOperator descriptorToMappingOffset,
final @Nullable LambdaClosure lambdaClosure) {
final ClassData parentClass = method.parentClass();
final StringBuilder msg = new StringBuilder("\t#%s %s".formatted(method.name(), method.descriptorText()));
if (lambdaClosure != null) {
final MethodData containingMethod = lambdaClosure.getContainingMethod();
msg.append("%n\t\tLambda Source: %s#%s %s"
.formatted(
containingMethod.parentClass().equals(parentClass)
? ""
: containingMethod.parentClass().name(),
containingMethod.name(),
containingMethod.descriptorText()));
}
for (int i = descriptorParamOffset; i < method.params().size(); i++) {
final int paramIdx = i;
final int lastIdxOfDot = method.param(i).toString().lastIndexOf('.');
msg.append("%n\t\t%s\t%-50s\t%s"
.formatted(
i,
method.param(i).toString().substring(lastIdxOfDot + 1),
Optional.ofNullable(methodMapping)
.flatMap(m -> m.getParameterMapping(descriptorToMappingOffset.applyAsInt(paramIdx)))
.map(Mapping::getDeobfuscatedName)
.orElse("<<MISSING>>")));
}
this.data.computeIfAbsent(parentClass, ignored -> new ArrayList<>()).add(msg.toString());
}

@Override
public String generate() {
final StringBuilder output = new StringBuilder();
this.data.entrySet().stream()
.sorted(Comparator.comparing(entry -> entry.getKey().name()))
.forEach(entry -> {
output.append("Missing param mappings in %s, Method Count: %s, Param Count: TODO\n"
.formatted(entry.getKey().name(), entry.getValue().size()));
entry.getValue().forEach(msg -> output.append(msg).append("\n"));
});
return output.toString();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import static org.mockito.Mockito.when;
import static org.mockito.Mockito.withSettings;

import com.google.inject.Guice;
import dev.denwav.hypo.asm.AsmClassData;
import dev.denwav.hypo.asm.AsmMethodData;
import dev.denwav.hypo.core.HypoContext;
Expand Down Expand Up @@ -100,7 +101,8 @@ void setup() throws Exception {

when(this.randomSourceClass.name()).thenReturn(RANDOM_SOURCE_TYPE.asInternalName());

this.suggester = new RootLvtSuggester(context, new LvtTypeSuggester(context), this.reports);
this.suggester =
new RootLvtSuggester(context, new LvtTypeSuggester(context), Guice.createInjector(this.reports));
}

@ParameterizedTest
Expand Down

0 comments on commit 29c9add

Please sign in to comment.