Catch conflicts when multiple parameters in the same scope get renamed to the same name (#48)

Author: Matyrobbrt

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

@@ -39,6 +39,11 @@ class GatherReplacementsVisitor extends PsiRecursiveElementVisitor {
      * this may contain the parameters of multiple scopes simultaneously.
      */
     private final Map<PsiParameter, String> activeParameters = new IdentityHashMap<>();
+    /**
+     * The parameter names that have already been used by a method in the outer scope and
+     * therefore cannot be used in nested methods.
+     */
+    private final Set<String> activeNames = new HashSet<>();
 
     public GatherReplacementsVisitor(NamesAndDocsDatabase namesAndDocs,
                                      boolean enableJavadoc,
@@ -134,8 +139,23 @@ else if (PsiHelper.isNonStaticInnerClass(psiMethod.getContainingClass())) {
                     if (paramData != null && paramData.getName() != null && !PsiHelper.isRecordConstructor(psiMethod)) {
                         var paramName = namer.apply(paramData.getName());
 
+                        // We cannot rename a parameter to name that was already taken in this scope
+                        if (activeNames.contains(paramName)) {
+                            // If we have no conflict resolver then we simply don't try to rename this parameter
+                            if (conflictResolver == null) {
+                                parameterOrder.add(psiParameter.getName());
+                                continue;
+                            }
+
+                            // Keep applying the conflict resolver until the name is no longer used
+                            while (activeNames.contains(paramName)) {
+                                paramName = conflictResolver.apply(paramName);
+                            }
+                        }
+
                         // Replace parameters within the method body
                         activeParameters.put(psiParameter, paramName);
+                        activeNames.add(paramName);
 
                         // Find and replace the parameter identifier
                         replacements.replace(psiParameter.getNameIdentifier(), paramName);
@@ -171,14 +191,17 @@ else if (PsiHelper.isNonStaticInnerClass(psiMethod.getContainingClass())) {
                     );
                 }
 
-                // When replacements were made and activeParamets were added, we visit the method children here ourselves
+                // When replacements were made and activeParameters were added, we visit the method children here ourselves
                 // and clean up active parameters afterward
                 if (hadReplacements) {
                     try {
                         element.acceptChildren(this);
                     } finally {
                         for (var parameter : parameters) {
-                            activeParameters.remove(parameter);
+                            var nm = activeParameters.remove(parameter);
+                            if (nm != null) {
+                                activeNames.remove(nm);
+                            }
                         }
                     }
                     return;

tests/data/parchment/conflicts/expected/com/example/Example.java

@@ -0,0 +1,11 @@
+package com.example;
+
+public class Example {
+    public void m(String in, String p_in) {
+        class Nested {
+            public void m(String p_p_in, String out) {
+
+            }
+        }
+    }
+}

tests/data/parchment/conflicts/mappings.tsrg

@@ -0,0 +1,9 @@
+tsrg2 left right
+obfuscated_c com/example/Example
+	obfuscated_m (Ljava/lang/String;)V m
+		0 o in
+		1 o in
+obfuscated_c$nested com/example/Example$1Nested
+	obfuscated_m (Ljava/lang/String;)V m
+		0 o in
+		1 o out

tests/data/parchment/conflicts/source/com/example/Example.java

@@ -0,0 +1,11 @@
+package com.example;
+
+public class Example {
+    public void m(String a1, String ac2) {
+        class Nested {
+            public void m(String ac3, String a4) {
+
+            }
+        }
+    }
+}

tests/src/test/java/net/neoforged/jst/tests/EmbeddedTest.java

@@ -224,6 +224,11 @@ void testTsrgMappings() throws Exception {
         void testAnonymousClasses() throws Exception {
             runParchmentTest("anonymous_classes", "mappings.tsrg");
         }
+
+        @Test
+        void testConflicts() throws Exception {
+            runParchmentTest("conflicts", "mappings.tsrg", "--parchment-conflict-prefix=p_");
+        }
     }
 
     @Nested
@@ -367,9 +372,11 @@ protected final void runATTest(String testDirName, final String... extraArgs) th
         ));
     }
 
-    protected final void runParchmentTest(String testDirName, String mappingsFilename) throws Exception {
+    protected final void runParchmentTest(String testDirName, String mappingsFilename, String... extraArgs) throws Exception {
         testDirName = "parchment/" + testDirName;
-        runTest(testDirName, UnaryOperator.identity(), "--enable-parchment", "--parchment-mappings", testDataRoot.resolve(testDirName).resolve(mappingsFilename).toString());
+        var args = new ArrayList<>(Arrays.asList("--enable-parchment", "--parchment-mappings", testDataRoot.resolve(testDirName).resolve(mappingsFilename).toString()));
+        args.addAll(Arrays.asList(extraArgs));
+        runTest(testDirName, UnaryOperator.identity(), args.toArray(String[]::new));
     }
 
     protected final void runTest(String testDirName, UnaryOperator<String> consoleMapper, String... args) throws Exception {