Tests and fixes

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=<inputFormat>] [--libraries-list=<librariesList>]
            [--max-queue-depth=<maxQueueDepth>] [--out-format=<outputFormat>]
            [--classpath=<addToClasspath>]... [--enable-parchment
-           --parchment-mappings=<mappingsPath> [--parchment-javadoc]] [--enable-accesstransformers
+           --parchment-mappings=<mappingsPath> [--[no-]parchment-javadoc]
+           [--parchment-conflict-prefix=<conflictPrefix>]] [--enable-accesstransformers
            --access-transformer=<atFiles> [--access-transformer=<atFiles>]...
            [--access-transformer-validation=<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=<addToClasspath>
-                            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=<inputFormat>
-                            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=<librariesList>
-                            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=<maxQueueDepth>
-                            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=<outputFormat>
-                            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=<conflictPrefix>
+                           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=<mappingsPath>
-
+                           The location of the Parchment mappings file
 Plugin - accesstransformers
       --access-transformer=<atFiles>
 
       --access-transformer-validation=<validation>
-                            The level of validation to use for ats
+                           The level of validation to use for ats
       --enable-accesstransformers
-                            Enable accesstransformers
+                           Enable accesstransformers
 ```
 
 ## Licenses

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 {

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<String> 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<String> 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<String, String> parameterJavadoc = new HashMap<>();
                 Map<String, String> renamedParameters = new HashMap<>();
-                final UnaryOperator<String> remapper;
-                if (psiMethod.getBody() == null) {
-                    remapper = p -> "p" + uppercase(p);
+
+                final UnaryOperator<String> namer;
+                if (conflictResolver == null) {
+                    namer = UnaryOperator.identity();
                 } else {
-                    final Set<String> 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<String> 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<String> 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);

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<String> 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);
+    }
 }

parchment/src/main/java/net/neoforged/jst/parchment/PsiParchmentHelper.java

@@ -2,19 +2,23 @@
 
 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;
 import net.neoforged.jst.parchment.namesanddocs.NamesAndDocsForClass;
 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<Optional<NamesAndDocsForClass>> CLASS_DATA_KEY = Key.create("names_and_docs_for_class");
     private static final Key<Optional<NamesAndDocsForMethod>> METHOD_DATA_KEY = Key.create("names_and_docs_for_method");
+    private static final Key<Set<String>> ALL_FIELD_NAMES = Key.create("all_field_names");
 
     @SuppressWarnings("OptionalAssignedToNull")
     @Nullable
@@ -63,4 +67,14 @@ public static NamesAndDocsForMethod getMethodData(NamesAndDocsDatabase namesAndD
         }
     }
 
+    public static Set<String> 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;
+    }
 }

parchment/src/main/java/net/neoforged/jst/parchment/LocalReferenceCollector.java → 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<String> references;
-    public LocalReferenceCollector(Set<String> references) {
-        this.references = references;
+public class ReservedNamesCollector extends PsiRecursiveElementVisitor {
+    public final Set<String> names;
+    public ReservedNamesCollector(Set<String> 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);