From 49b8048db936956308d8e19589ff0ee16e2d973a Mon Sep 17 00:00:00 2001 From: Matyrobbrt Date: Sun, 6 Apr 2025 15:56:21 +0300 Subject: [PATCH] Catch conflicts when multiple parameters in the same scope get renamed to the same name --- .../parchment/GatherReplacementsVisitor.java | 27 +++++++++++++++++-- .../expected/com/example/Example.java | 11 ++++++++ tests/data/parchment/conflicts/mappings.tsrg | 9 +++++++ .../conflicts/source/com/example/Example.java | 11 ++++++++ .../net/neoforged/jst/tests/EmbeddedTest.java | 11 ++++++-- 5 files changed, 65 insertions(+), 4 deletions(-) create mode 100644 tests/data/parchment/conflicts/expected/com/example/Example.java create mode 100644 tests/data/parchment/conflicts/mappings.tsrg create mode 100644 tests/data/parchment/conflicts/source/com/example/Example.java 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 840e5d9..1d57043 100644 --- a/parchment/src/main/java/net/neoforged/jst/parchment/GatherReplacementsVisitor.java +++ b/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 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 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; diff --git a/tests/data/parchment/conflicts/expected/com/example/Example.java b/tests/data/parchment/conflicts/expected/com/example/Example.java new file mode 100644 index 0000000..0bb315d --- /dev/null +++ b/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) { + + } + } + } +} diff --git a/tests/data/parchment/conflicts/mappings.tsrg b/tests/data/parchment/conflicts/mappings.tsrg new file mode 100644 index 0000000..53ed1f7 --- /dev/null +++ b/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 diff --git a/tests/data/parchment/conflicts/source/com/example/Example.java b/tests/data/parchment/conflicts/source/com/example/Example.java new file mode 100644 index 0000000..4d4f9ff --- /dev/null +++ b/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) { + + } + } + } +} diff --git a/tests/src/test/java/net/neoforged/jst/tests/EmbeddedTest.java b/tests/src/test/java/net/neoforged/jst/tests/EmbeddedTest.java index 842fc84..365b3c0 100644 --- a/tests/src/test/java/net/neoforged/jst/tests/EmbeddedTest.java +++ b/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 consoleMapper, String... args) throws Exception {