diff --git a/README.md b/README.md index 4874439..40f3e6c 100644 --- a/README.md +++ b/README.md @@ -30,42 +30,47 @@ It can be invoked as a standalone executable Jar-File. Java 17 is required. Usage: jst [-hV] [--in-format=] [--libraries-list=] [--max-queue-depth=] [--out-format=] [--classpath=]... [--enable-parchment - --parchment-mappings= [--parchment-javadoc]] [--enable-accesstransformers + --parchment-mappings= [--[no-]parchment-javadoc] + [--parchment-conflict-prefix=]] [--enable-accesstransformers --access-transformer= [--access-transformer=]... [--access-transformer-validation=]] INPUT OUTPUT - INPUT Path to a single Java-file, a source-archive or a folder containing the - source to transform. - OUTPUT Path to where the resulting source should be placed. + INPUT Path to a single Java-file, a source-archive or a folder containing the + source to transform. + OUTPUT Path to where the resulting source should be placed. --classpath= - Additional classpath entries to use. Is combined with --libraries-list. - -h, --help Show this help message and exit. + Additional classpath entries to use. Is combined with --libraries-list. + -h, --help Show this help message and exit. --in-format= - Specify the format of INPUT explicitly. AUTO (the default) performs - auto-detection. Other options are SINGLE_FILE for Java files, ARCHIVE - for source jars or zips, and FOLDER for folders containing Java code. + Specify the format of INPUT explicitly. AUTO (the default) performs + auto-detection. Other options are SINGLE_FILE for Java files, ARCHIVE + for source jars or zips, and FOLDER for folders containing Java code. --libraries-list= - Specifies a file that contains a path to an archive or directory to add - to the classpath on each line. + Specifies a file that contains a path to an archive or directory to add + to the classpath on each line. --max-queue-depth= - When both input and output support ordering (archives), the transformer - will try to maintain that order. To still process items in parallel, - a queue is used. Larger queue depths lead to higher memory usage. + When both input and output support ordering (archives), the transformer + will try to maintain that order. To still process items in parallel, a + queue is used. Larger queue depths lead to higher memory usage. --out-format= - Specify the format of OUTPUT explicitly. Allows the same options as - --in-format. - -V, --version Print version information and exit. + Specify the format of OUTPUT explicitly. Allows the same options as + --in-format. + -V, --version Print version information and exit. Plugin - parchment - --enable-parchment Enable parchment - --parchment-javadoc + --enable-parchment Enable parchment + --parchment-conflict-prefix= + Apply the prefix specified if a Parchment parameter name conflicts with + existing variable names + --[no-]parchment-javadoc + Whether Parchment javadocs should be applied --parchment-mappings= - + The location of the Parchment mappings file Plugin - accesstransformers --access-transformer= --access-transformer-validation= - The level of validation to use for ats + The level of validation to use for ats --enable-accesstransformers - Enable accesstransformers + Enable accesstransformers ``` ## Licenses diff --git a/parchment/build.gradle b/parchment/build.gradle index 8b365c0..c2d56ff 100644 --- a/parchment/build.gradle +++ b/parchment/build.gradle @@ -9,6 +9,8 @@ dependencies { testImplementation platform("org.junit:junit-bom:$junit_version") testImplementation 'org.junit.jupiter:junit-jupiter' + testImplementation(project(':cli')) + testImplementation("org.assertj:assertj-core:$assertj_version") } test { diff --git a/parchment/src/main/java/net/neoforged/jst/parchment/GatherReplacementsVisitor.java b/parchment/src/main/java/net/neoforged/jst/parchment/GatherReplacementsVisitor.java index 5084e04..714fe27 100644 --- a/parchment/src/main/java/net/neoforged/jst/parchment/GatherReplacementsVisitor.java +++ b/parchment/src/main/java/net/neoforged/jst/parchment/GatherReplacementsVisitor.java @@ -1,5 +1,6 @@ package net.neoforged.jst.parchment; +import com.intellij.lang.jvm.JvmParameter; import com.intellij.psi.JavaPsiFacade; import com.intellij.psi.PsiClass; import com.intellij.psi.PsiElement; @@ -13,8 +14,8 @@ import net.neoforged.jst.api.PsiHelper; import net.neoforged.jst.api.Replacements; import net.neoforged.jst.parchment.namesanddocs.NamesAndDocsDatabase; -import net.neoforged.jst.parchment.namesanddocs.NamesAndDocsForParameter; import org.jetbrains.annotations.NotNull; +import org.jetbrains.annotations.Nullable; import java.util.ArrayList; import java.util.HashMap; @@ -29,6 +30,8 @@ class GatherReplacementsVisitor extends PsiRecursiveElementVisitor { private final NamesAndDocsDatabase namesAndDocs; private final boolean enableJavadoc; + @Nullable + private final UnaryOperator conflictResolver; private final Replacements replacements; /** * Renamed parameters of the combined outer scopes we are currently visiting. @@ -39,9 +42,10 @@ class GatherReplacementsVisitor extends PsiRecursiveElementVisitor { public GatherReplacementsVisitor(NamesAndDocsDatabase namesAndDocs, boolean enableJavadoc, - Replacements replacements) { + @Nullable UnaryOperator conflictResolver, Replacements replacements) { this.namesAndDocs = namesAndDocs; this.enableJavadoc = enableJavadoc; + this.conflictResolver = conflictResolver; this.replacements = replacements; } @@ -79,14 +83,24 @@ public void visitElement(@NotNull PsiElement element) { Map parameterJavadoc = new HashMap<>(); Map renamedParameters = new HashMap<>(); - final UnaryOperator remapper; - if (psiMethod.getBody() == null) { - remapper = p -> "p" + uppercase(p); + + final UnaryOperator namer; + if (conflictResolver == null) { + namer = UnaryOperator.identity(); } else { - final Set localRefs = new HashSet<>(); - new LocalReferenceCollector(localRefs).visitElement(psiMethod.getBody()); - remapper = p -> localRefs.contains(p) ? "p" + uppercase(p) : p; + if (psiMethod.getBody() == null) { + namer = conflictResolver; + } else { + final Set localRefs = new HashSet<>(); + // Existing parameter names are considered reserved to avoid patched-in parameters to conflict with Parchment names + for (JvmParameter parameter : psiMethod.getParameters()) { + localRefs.add(parameter.getName()); + } + new ReservedNamesCollector(localRefs).visitElement(psiMethod.getBody()); + namer = p -> localRefs.contains(p) ? conflictResolver.apply(p) : p; + } } + List parameterOrder = new ArrayList<>(); var parameters = psiMethod.getParameterList().getParameters(); @@ -107,7 +121,7 @@ public void visitElement(@NotNull PsiElement element) { // Optionally replace the parameter name, but skip record constructors, since those could have // implications for the field names. if (paramData != null && paramData.getName() != null && !PsiHelper.isRecordConstructor(psiMethod)) { - var paramName = remapper.apply(paramData.getName()); + var paramName = namer.apply(paramData.getName()); // Replace parameters within the method body activeParameters.put(psiParameter, paramName); diff --git a/parchment/src/main/java/net/neoforged/jst/parchment/ParchmentTransformer.java b/parchment/src/main/java/net/neoforged/jst/parchment/ParchmentTransformer.java index faa5204..75ddf64 100644 --- a/parchment/src/main/java/net/neoforged/jst/parchment/ParchmentTransformer.java +++ b/parchment/src/main/java/net/neoforged/jst/parchment/ParchmentTransformer.java @@ -11,18 +11,35 @@ import java.io.IOException; import java.io.UncheckedIOException; import java.nio.file.Path; +import java.util.function.UnaryOperator; public class ParchmentTransformer implements SourceTransformer { - @CommandLine.Option(names = "--parchment-mappings", required = true) + @CommandLine.Option(names = "--parchment-mappings", required = true, description = "The location of the Parchment mappings file") public Path mappingsPath; - @CommandLine.Option(names = "--parchment-javadoc") + @CommandLine.Option(names = "--parchment-javadoc", description = "Whether Parchment javadocs should be applied", negatable = true) public boolean enableJavadoc = true; + @CommandLine.Option(names = "--parchment-conflict-prefix", description = "Apply the prefix specified if a Parchment parameter name conflicts with existing variable names") + public String conflictPrefix; + private NamesAndDocsDatabase namesAndDocs; + private UnaryOperator conflictResolver; @Override public void beforeRun(TransformContext context) { + if (conflictPrefix != null) { + if (conflictPrefix.isBlank()) { + throw new IllegalArgumentException("Parchment conflict prefix cannot be blank"); + } + + if (Character.isLetterOrDigit(conflictPrefix.charAt(conflictPrefix.length() - 1))) { + conflictResolver = p -> conflictPrefix + toUpperCase(p); + } else { + conflictResolver = p -> conflictPrefix + p; + } + } + System.out.println("Loading mapping file " + mappingsPath); try { namesAndDocs = NameAndDocSourceLoader.load(mappingsPath); @@ -33,8 +50,14 @@ public void beforeRun(TransformContext context) { @Override public void visitFile(PsiFile psiFile, Replacements replacements) { - var visitor = new GatherReplacementsVisitor(namesAndDocs, enableJavadoc, replacements); + var visitor = new GatherReplacementsVisitor(namesAndDocs, enableJavadoc, conflictResolver, replacements); visitor.visitElement(psiFile); } + private static String toUpperCase(String str) { + if (str.length() == 1) { + return String.valueOf(Character.toUpperCase(str.charAt(0))); + } + return Character.toUpperCase(str.charAt(0)) + str.substring(1); + } } diff --git a/parchment/src/main/java/net/neoforged/jst/parchment/PsiParchmentHelper.java b/parchment/src/main/java/net/neoforged/jst/parchment/PsiParchmentHelper.java index 36c5239..7620692 100644 --- a/parchment/src/main/java/net/neoforged/jst/parchment/PsiParchmentHelper.java +++ b/parchment/src/main/java/net/neoforged/jst/parchment/PsiParchmentHelper.java @@ -2,6 +2,7 @@ import com.intellij.openapi.util.Key; import com.intellij.psi.PsiClass; +import com.intellij.psi.PsiField; import com.intellij.psi.PsiMethod; import net.neoforged.jst.api.PsiHelper; import net.neoforged.jst.parchment.namesanddocs.NamesAndDocsDatabase; @@ -9,12 +10,15 @@ import net.neoforged.jst.parchment.namesanddocs.NamesAndDocsForMethod; import org.jetbrains.annotations.Nullable; +import java.util.HashSet; import java.util.Optional; +import java.util.Set; public class PsiParchmentHelper { // Keys for attaching mapping data to a PsiClass. Used to prevent multiple lookups for the same class/method. private static final Key> CLASS_DATA_KEY = Key.create("names_and_docs_for_class"); private static final Key> METHOD_DATA_KEY = Key.create("names_and_docs_for_method"); + private static final Key> ALL_FIELD_NAMES = Key.create("all_field_names"); @SuppressWarnings("OptionalAssignedToNull") @Nullable @@ -63,4 +67,14 @@ public static NamesAndDocsForMethod getMethodData(NamesAndDocsDatabase namesAndD } } + public static Set getAllFieldNames(PsiClass clazz) { + var existing = clazz.getUserData(ALL_FIELD_NAMES); + if (existing != null) return existing; + existing = new HashSet<>(); + for (PsiField field : clazz.getAllFields()) { + existing.add(field.getName()); + } + clazz.putUserData(ALL_FIELD_NAMES, existing); + return existing; + } } diff --git a/parchment/src/main/java/net/neoforged/jst/parchment/LocalReferenceCollector.java b/parchment/src/main/java/net/neoforged/jst/parchment/ReservedNamesCollector.java similarity index 58% rename from parchment/src/main/java/net/neoforged/jst/parchment/LocalReferenceCollector.java rename to parchment/src/main/java/net/neoforged/jst/parchment/ReservedNamesCollector.java index 1a3c254..a03b45e 100644 --- a/parchment/src/main/java/net/neoforged/jst/parchment/LocalReferenceCollector.java +++ b/parchment/src/main/java/net/neoforged/jst/parchment/ReservedNamesCollector.java @@ -1,5 +1,6 @@ package net.neoforged.jst.parchment; +import com.intellij.psi.PsiClass; import com.intellij.psi.PsiElement; import com.intellij.psi.PsiJavaToken; import com.intellij.psi.PsiLocalVariable; @@ -11,16 +12,16 @@ import java.util.Set; -public class LocalReferenceCollector extends PsiRecursiveElementVisitor { - public final Set references; - public LocalReferenceCollector(Set references) { - this.references = references; +public class ReservedNamesCollector extends PsiRecursiveElementVisitor { + public final Set names; + public ReservedNamesCollector(Set names) { + this.names = names; } @Override public void visitElement(@NotNull PsiElement element) { if (element instanceof PsiParameter parameter) { - references.add(parameter.getName()); + names.add(parameter.getName()); } else if (element instanceof PsiReferenceExpression reference && !(element instanceof PsiMethodReferenceExpression)) { boolean qualified = false; // Unqualified references are to be considered local variables @@ -32,10 +33,15 @@ public void visitElement(@NotNull PsiElement element) { } if (!qualified) { - references.add(reference.getLastChild().getText()); + names.add(reference.getLastChild().getText()); } } else if (element instanceof PsiLocalVariable variable) { - references.add(variable.getName()); + names.add(variable.getName()); + } else if (element instanceof PsiClass cls) { + // To catch cases where inherited protected fields of local classes take precedence over the parameters + // when we encounter a class local to a method we will consider all its field names reserved + names.addAll(PsiParchmentHelper.getAllFieldNames(cls)); + return; // But we don't need to process further as references in the methods declared in the local class are out of scope for this check } element.acceptChildren(this); diff --git a/parchment/src/test/java/net/neoforged/jst/parchment/namesanddocs/parchment/ReservedNamesTest.java b/parchment/src/test/java/net/neoforged/jst/parchment/namesanddocs/parchment/ReservedNamesTest.java new file mode 100644 index 0000000..68a1607 --- /dev/null +++ b/parchment/src/test/java/net/neoforged/jst/parchment/namesanddocs/parchment/ReservedNamesTest.java @@ -0,0 +1,186 @@ +package net.neoforged.jst.parchment.namesanddocs.parchment; + +import com.intellij.psi.PsiClass; +import com.intellij.psi.PsiElement; +import com.intellij.psi.PsiFile; +import com.intellij.psi.PsiJavaFile; +import com.intellij.psi.PsiMethod; +import com.intellij.psi.util.PsiTreeUtil; +import net.neoforged.jst.api.Logger; +import net.neoforged.jst.cli.intellij.IntelliJEnvironmentImpl; +import net.neoforged.jst.parchment.ReservedNamesCollector; +import org.intellij.lang.annotations.Language; +import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Test; + +import java.io.IOException; +import java.util.HashSet; +import java.util.Set; + +import static org.assertj.core.api.Assertions.assertThat; + +public class ReservedNamesTest { + static IntelliJEnvironmentImpl ijEnv; + + @BeforeAll + static void setUp() throws IOException { + ijEnv = new IntelliJEnvironmentImpl(new Logger(null, null)); + } + + @AfterAll + static void tearDown() throws IOException { + ijEnv.close(); + } + + @Test + void testSimpleDeclarations() { + var method = parseSingleMethod(""" + class Main { + void run() { + String declared = "a"; + int declaredInt = 1; + } + }"""); + assertThat(collectReferences(method)) + .containsExactlyInAnyOrder("declared", "declaredInt"); + } + + @Test + void testLocalReferences() { + var method = parseSingleMethod(""" + class Main { + int qualified, nonqualified; + + void run() { + this.qualified = 1; + nonqualified = 2; + } + }"""); + assertThat(collectReferences(method)) + .containsExactlyInAnyOrder("nonqualified"); + } + + @Test + void testMethodReferences() { + var method = parseSingleMethod(""" + class Main { + void run() { + Runnable runnable = this::run; + } + }"""); + + // We test that the run method reference isn't considered a reserved name + assertThat(collectReferences(method)) + .containsExactlyInAnyOrder("runnable"); + } + + @Test + void testLambdaParameters() { + var method = parseSingleMethod(""" + import java.util.function.Function; + class Main { + void run() { + Function func = (p1) -> { + }; + } + }"""); + + assertThat(collectReferences(method)) + .containsExactlyInAnyOrder("func", "p1"); + } + + @Test + void testLambdaLocals() { + var method = parseSingleMethod(""" + import java.util.function.Function; + class Main { + void run() { + Runnable run = () -> { + String lambdaLocal = "abc"; + }; + } + }"""); + + assertThat(collectReferences(method)) + .containsExactlyInAnyOrder("run", "lambdaLocal"); + } + + @Test + void testAnonymousFields() { + // Test that fields of anonymous classes are considered reserved names as they take priority over + // captured locals of the parent method + var file = parse(""" + import java.util.function.Function; + class Main { + void run() { + new SubClass() { + int declaredAnon; + }; + } + + static class SubClass extends SubClassParent { + private String directField; + } + + static class SubClassParent { + protected int inheritedField; + } + }"""); + + assertThat(collectReferences(file.getClasses()[0].getMethods()[0])) + .containsExactlyInAnyOrder("directField", "inheritedField", "declaredAnon"); + } + + @Test + void testLocalClassFields() { + // Test that fields of local classes are considered reserved names as they take priority over + // captured locals of the parent method + var file = parse(""" + import java.util.function.Function; + class Main { + void run() { + class LocalClass extends SubClass { + int declaredAnon; + } + } + + static class SubClass extends SubClassParent { + private String directField; + } + + static class SubClassParent { + protected int inheritedField; + } + }"""); + + assertThat(collectReferences(file.getClasses()[0].getMethods()[0])) + .containsExactlyInAnyOrder("directField", "inheritedField", "declaredAnon"); + } + + private Set collectReferences(PsiElement element) { + var set = new HashSet(); + new ReservedNamesCollector(set).visitElement(element); + return set; + } + + private PsiMethod parseSingleMethod(@Language("JAVA") String javaCode) { + return parseSingleElement(javaCode, PsiMethod.class); + } + + private PsiClass parseSingleClass(@Language("JAVA") String javaCode) { + return parseSingleElement(javaCode, PsiClass.class); + } + + private T parseSingleElement(@Language("JAVA") String javaCode, Class type) { + var file = parse(javaCode); + + var elements = PsiTreeUtil.collectElementsOfType(file, type); + assertThat(1).describedAs("parsed elements").isEqualTo(elements.size()); + return elements.iterator().next(); + } + + private PsiJavaFile parse(@Language("JAVA") String javaCode) { + return (PsiJavaFile) ijEnv.parseFileFromMemory("Test.java", javaCode); + } +}