From b4c3dcf20d40eabab61a018e538d2603979e3ec6 Mon Sep 17 00:00:00 2001 From: Bjarne Koll Date: Mon, 16 Dec 2024 14:23:21 +0100 Subject: [PATCH 1/2] Flag to inherit access transformers on methods Useful for modifying methods exposed by types with a lot of children to avoid specifying them all. --- README.md | 3 ++ .../AccessTransformersTransformer.java | 14 ++++++++- .../accesstransformers/ApplyATsVisitor.java | 31 ++++++++++++++----- .../methods_inheritance/accesstransformer.cfg | 1 + .../methods_inheritance/expected/Child.java | 6 ++++ .../expected/Grandchild.java | 6 ++++ .../methods_inheritance/expected/Parent.java | 5 +++ .../methods_inheritance/source/Child.java | 6 ++++ .../source/Grandchild.java | 6 ++++ .../methods_inheritance/source/Parent.java | 5 +++ .../accesstransformer.cfg | 1 + .../expected/Child.java | 6 ++++ .../expected/Grandchild.java | 6 ++++ .../expected/Parent.java | 5 +++ .../methods_no_inheritance/source/Child.java | 6 ++++ .../source/Grandchild.java | 6 ++++ .../methods_no_inheritance/source/Parent.java | 5 +++ .../net/neoforged/jst/tests/EmbeddedTest.java | 20 ++++++++++-- 18 files changed, 128 insertions(+), 10 deletions(-) create mode 100644 tests/data/accesstransformer/methods_inheritance/accesstransformer.cfg create mode 100644 tests/data/accesstransformer/methods_inheritance/expected/Child.java create mode 100644 tests/data/accesstransformer/methods_inheritance/expected/Grandchild.java create mode 100644 tests/data/accesstransformer/methods_inheritance/expected/Parent.java create mode 100644 tests/data/accesstransformer/methods_inheritance/source/Child.java create mode 100644 tests/data/accesstransformer/methods_inheritance/source/Grandchild.java create mode 100644 tests/data/accesstransformer/methods_inheritance/source/Parent.java create mode 100644 tests/data/accesstransformer/methods_no_inheritance/accesstransformer.cfg create mode 100644 tests/data/accesstransformer/methods_no_inheritance/expected/Child.java create mode 100644 tests/data/accesstransformer/methods_no_inheritance/expected/Grandchild.java create mode 100644 tests/data/accesstransformer/methods_no_inheritance/expected/Parent.java create mode 100644 tests/data/accesstransformer/methods_no_inheritance/source/Child.java create mode 100644 tests/data/accesstransformer/methods_no_inheritance/source/Grandchild.java create mode 100644 tests/data/accesstransformer/methods_no_inheritance/source/Parent.java diff --git a/README.md b/README.md index 7550448..c6ff7ba 100644 --- a/README.md +++ b/README.md @@ -102,6 +102,9 @@ Plugin - parchment Plugin - accesstransformers --access-transformer= + --access-transformer-inherit-method + Whether or not access transformers on methods should be inherited from + parent types --access-transformer-validation= The level of validation to use for ats --enable-accesstransformers diff --git a/accesstransformers/src/main/java/net/neoforged/jst/accesstransformers/AccessTransformersTransformer.java b/accesstransformers/src/main/java/net/neoforged/jst/accesstransformers/AccessTransformersTransformer.java index c70f4fb..76452b7 100644 --- a/accesstransformers/src/main/java/net/neoforged/jst/accesstransformers/AccessTransformersTransformer.java +++ b/accesstransformers/src/main/java/net/neoforged/jst/accesstransformers/AccessTransformersTransformer.java @@ -25,6 +25,12 @@ public class AccessTransformersTransformer implements SourceTransformer { @CommandLine.Option(names = "--access-transformer-validation", description = "The level of validation to use for ats") public AccessTransformerValidation validation = AccessTransformerValidation.LOG; + @CommandLine.Option( + names = "--access-transformer-inherit-method", + description = "Whether or not access transformers on methods should be inherited from parent types" + ) + public boolean inheritMethodAccessTransformers = false; + private AccessTransformerFiles ats; private Map pendingATs; private Logger logger; @@ -64,7 +70,13 @@ public boolean afterRun(TransformContext context) { @Override public void visitFile(PsiFile psiFile, Replacements replacements) { - var visitor = new ApplyATsVisitor(ats, replacements, pendingATs, logger); + var visitor = new ApplyATsVisitor( + ats, + replacements, + pendingATs, + this.inheritMethodAccessTransformers, + logger + ); visitor.visitFile(psiFile); if (visitor.errored) { errored = true; diff --git a/accesstransformers/src/main/java/net/neoforged/jst/accesstransformers/ApplyATsVisitor.java b/accesstransformers/src/main/java/net/neoforged/jst/accesstransformers/ApplyATsVisitor.java index 2cfce18..a4bb286 100644 --- a/accesstransformers/src/main/java/net/neoforged/jst/accesstransformers/ApplyATsVisitor.java +++ b/accesstransformers/src/main/java/net/neoforged/jst/accesstransformers/ApplyATsVisitor.java @@ -14,7 +14,6 @@ import com.intellij.psi.PsiRecursiveElementVisitor; import com.intellij.psi.PsiWhiteSpace; import com.intellij.psi.util.ClassUtil; -import com.intellij.psi.util.PsiClassUtil; import net.neoforged.accesstransformer.parser.AccessTransformerFiles; import net.neoforged.accesstransformer.parser.Target; import net.neoforged.accesstransformer.parser.Transformation; @@ -46,12 +45,20 @@ class ApplyATsVisitor extends PsiRecursiveElementVisitor { private final AccessTransformerFiles ats; private final Replacements replacements; private final Map pendingATs; + private final boolean inheritMethodATs; private final Logger logger; boolean errored = false; - public ApplyATsVisitor(AccessTransformerFiles ats, Replacements replacements, Map pendingATs, Logger logger) { + public ApplyATsVisitor( + AccessTransformerFiles ats, + Replacements replacements, + Map pendingATs, + boolean inheritMethodATs, + Logger logger + ) { this.ats = ats; this.replacements = replacements; + this.inheritMethodATs = inheritMethodATs; this.logger = logger; this.pendingATs = pendingATs; } @@ -61,7 +68,7 @@ public void visitElement(@NotNull PsiElement element) { if (element instanceof PsiClass psiClass) { if (psiClass.getQualifiedName() != null) { String className = ClassUtil.getJVMClassName(psiClass); - if (!ats.containsClassTarget(className)) { + if (!inheritMethodATs && !ats.containsClassTarget(className)) { // Skip this class and its children, but not the inner classes for (PsiClass innerClass : psiClass.getInnerClasses()) { visitElement(innerClass); @@ -106,10 +113,20 @@ public void visitElement(@NotNull PsiElement element) { apply(pendingATs.remove(new Target.FieldTarget(className, field.getName())), field, cls); } } else if (element instanceof PsiMethod method) { - final var cls = method.getContainingClass(); - if (cls != null && cls.getQualifiedName() != null) { - String className = ClassUtil.getJVMClassName(cls); - apply(pendingATs.remove(method(className, method)), method, cls); + final var owningType = method.getContainingClass(); + if (owningType != null) { + // Locate transformation by searching owning type and its parents. + // Remove if this is AT is defined for its immediate owning type. + Transformation foundTransformation = this.pendingATs.remove(method(ClassUtil.getJVMClassName(owningType), method)); + PsiClass definedForSuperType = owningType; + for (; + this.inheritMethodATs && foundTransformation == null && definedForSuperType != null && definedForSuperType.getQualifiedName() != null; + definedForSuperType = definedForSuperType.getSuperClass() + ) { + foundTransformation = this.ats.getAccessTransformers().get(method(ClassUtil.getJVMClassName(definedForSuperType), method)); + } + + apply(foundTransformation, method, owningType); } } diff --git a/tests/data/accesstransformer/methods_inheritance/accesstransformer.cfg b/tests/data/accesstransformer/methods_inheritance/accesstransformer.cfg new file mode 100644 index 0000000..a6a2ef9 --- /dev/null +++ b/tests/data/accesstransformer/methods_inheritance/accesstransformer.cfg @@ -0,0 +1 @@ +public Parent flag()Z diff --git a/tests/data/accesstransformer/methods_inheritance/expected/Child.java b/tests/data/accesstransformer/methods_inheritance/expected/Child.java new file mode 100644 index 0000000..8057da2 --- /dev/null +++ b/tests/data/accesstransformer/methods_inheritance/expected/Child.java @@ -0,0 +1,6 @@ +class Child extends Parent { + @Override + public boolean flag() { + return true; + } +} \ No newline at end of file diff --git a/tests/data/accesstransformer/methods_inheritance/expected/Grandchild.java b/tests/data/accesstransformer/methods_inheritance/expected/Grandchild.java new file mode 100644 index 0000000..9c42ae0 --- /dev/null +++ b/tests/data/accesstransformer/methods_inheritance/expected/Grandchild.java @@ -0,0 +1,6 @@ +class Grandchild extends Child { + @Override + public boolean flag() { + return false; + } +} \ No newline at end of file diff --git a/tests/data/accesstransformer/methods_inheritance/expected/Parent.java b/tests/data/accesstransformer/methods_inheritance/expected/Parent.java new file mode 100644 index 0000000..ed8b47f --- /dev/null +++ b/tests/data/accesstransformer/methods_inheritance/expected/Parent.java @@ -0,0 +1,5 @@ +class Parent { + public boolean flag() { + return false; + } +} \ No newline at end of file diff --git a/tests/data/accesstransformer/methods_inheritance/source/Child.java b/tests/data/accesstransformer/methods_inheritance/source/Child.java new file mode 100644 index 0000000..66a5567 --- /dev/null +++ b/tests/data/accesstransformer/methods_inheritance/source/Child.java @@ -0,0 +1,6 @@ +class Child extends Parent { + @Override + protected boolean flag() { + return true; + } +} \ No newline at end of file diff --git a/tests/data/accesstransformer/methods_inheritance/source/Grandchild.java b/tests/data/accesstransformer/methods_inheritance/source/Grandchild.java new file mode 100644 index 0000000..f55e09b --- /dev/null +++ b/tests/data/accesstransformer/methods_inheritance/source/Grandchild.java @@ -0,0 +1,6 @@ +class Grandchild extends Child { + @Override + protected boolean flag() { + return false; + } +} \ No newline at end of file diff --git a/tests/data/accesstransformer/methods_inheritance/source/Parent.java b/tests/data/accesstransformer/methods_inheritance/source/Parent.java new file mode 100644 index 0000000..7d8caad --- /dev/null +++ b/tests/data/accesstransformer/methods_inheritance/source/Parent.java @@ -0,0 +1,5 @@ +class Parent { + protected boolean flag() { + return false; + } +} \ No newline at end of file diff --git a/tests/data/accesstransformer/methods_no_inheritance/accesstransformer.cfg b/tests/data/accesstransformer/methods_no_inheritance/accesstransformer.cfg new file mode 100644 index 0000000..a6a2ef9 --- /dev/null +++ b/tests/data/accesstransformer/methods_no_inheritance/accesstransformer.cfg @@ -0,0 +1 @@ +public Parent flag()Z diff --git a/tests/data/accesstransformer/methods_no_inheritance/expected/Child.java b/tests/data/accesstransformer/methods_no_inheritance/expected/Child.java new file mode 100644 index 0000000..66a5567 --- /dev/null +++ b/tests/data/accesstransformer/methods_no_inheritance/expected/Child.java @@ -0,0 +1,6 @@ +class Child extends Parent { + @Override + protected boolean flag() { + return true; + } +} \ No newline at end of file diff --git a/tests/data/accesstransformer/methods_no_inheritance/expected/Grandchild.java b/tests/data/accesstransformer/methods_no_inheritance/expected/Grandchild.java new file mode 100644 index 0000000..f55e09b --- /dev/null +++ b/tests/data/accesstransformer/methods_no_inheritance/expected/Grandchild.java @@ -0,0 +1,6 @@ +class Grandchild extends Child { + @Override + protected boolean flag() { + return false; + } +} \ No newline at end of file diff --git a/tests/data/accesstransformer/methods_no_inheritance/expected/Parent.java b/tests/data/accesstransformer/methods_no_inheritance/expected/Parent.java new file mode 100644 index 0000000..ed8b47f --- /dev/null +++ b/tests/data/accesstransformer/methods_no_inheritance/expected/Parent.java @@ -0,0 +1,5 @@ +class Parent { + public boolean flag() { + return false; + } +} \ No newline at end of file diff --git a/tests/data/accesstransformer/methods_no_inheritance/source/Child.java b/tests/data/accesstransformer/methods_no_inheritance/source/Child.java new file mode 100644 index 0000000..66a5567 --- /dev/null +++ b/tests/data/accesstransformer/methods_no_inheritance/source/Child.java @@ -0,0 +1,6 @@ +class Child extends Parent { + @Override + protected boolean flag() { + return true; + } +} \ No newline at end of file diff --git a/tests/data/accesstransformer/methods_no_inheritance/source/Grandchild.java b/tests/data/accesstransformer/methods_no_inheritance/source/Grandchild.java new file mode 100644 index 0000000..f55e09b --- /dev/null +++ b/tests/data/accesstransformer/methods_no_inheritance/source/Grandchild.java @@ -0,0 +1,6 @@ +class Grandchild extends Child { + @Override + protected boolean flag() { + return false; + } +} \ No newline at end of file diff --git a/tests/data/accesstransformer/methods_no_inheritance/source/Parent.java b/tests/data/accesstransformer/methods_no_inheritance/source/Parent.java new file mode 100644 index 0000000..7d8caad --- /dev/null +++ b/tests/data/accesstransformer/methods_no_inheritance/source/Parent.java @@ -0,0 +1,5 @@ +class Parent { + protected boolean flag() { + return false; + } +} \ No newline at end of file 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 082c293..0beb39d 100644 --- a/tests/src/test/java/net/neoforged/jst/tests/EmbeddedTest.java +++ b/tests/src/test/java/net/neoforged/jst/tests/EmbeddedTest.java @@ -1,5 +1,6 @@ package net.neoforged.jst.tests; +import com.intellij.util.ArrayUtil; import net.neoforged.jst.cli.Main; import org.assertj.core.util.CanIgnoreReturnValue; import org.junit.jupiter.api.BeforeEach; @@ -286,6 +287,16 @@ void testImplicitConstructors() throws Exception { void testIllegal() throws Exception { runATTest("illegal"); } + + @Test + void testMethodsNoInheritance() throws Exception { + runATTest("methods_no_inheritance"); + } + + @Test + void testMethodsInheritance() throws Exception { + runATTest("methods_inheritance", "--access-transformer-inherit-method"); + } } @Nested @@ -350,10 +361,15 @@ protected final void runInterfaceInjectionTest(String testDirName, Path tempDir, } } - protected final void runATTest(String testDirName) throws Exception { + protected final void runATTest(String testDirName, final String... extraArgs) throws Exception { testDirName = "accesstransformer/" + testDirName; var atPath = testDataRoot.resolve(testDirName).resolve("accesstransformer.cfg"); - runTest(testDirName, txt -> txt.replace(atPath.toAbsolutePath().toString(), "{atpath}"), "--enable-accesstransformers", "--access-transformer", atPath.toString()); + runTest(testDirName, txt -> txt.replace(atPath.toAbsolutePath().toString(), "{atpath}"), ArrayUtil.mergeArrays( + new String[]{ + "--enable-accesstransformers", "--access-transformer", atPath.toString() + }, + extraArgs + )); } protected final void runParchmentTest(String testDirName, String mappingsFilename) throws Exception { From 64a53bbb79e3419989c83e70ac3e05f26d1a3773 Mon Sep 17 00:00:00 2001 From: Bjarne Koll Date: Mon, 16 Dec 2024 15:46:33 +0100 Subject: [PATCH 2/2] Only visit types with parent ATs when inheriting When inheriting method ATs, classes may need to be processed that are not found in the source AT set as they inherit their ATs from their parent. The previous iteration would accomplish this by simply visiting the entire source root. The commit introduces a simplistic heuristic, filtering out any types from processing that do not have a parent with an AT defined. While this is not exact, e.g. a class will still be processed if its parent type has a field changed via AT, the accesstransformer library used does not expose the types of ATs defined on a class, making this a best effort approximation. --- .../accesstransformers/ApplyATsVisitor.java | 20 ++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/accesstransformers/src/main/java/net/neoforged/jst/accesstransformers/ApplyATsVisitor.java b/accesstransformers/src/main/java/net/neoforged/jst/accesstransformers/ApplyATsVisitor.java index a4bb286..923247a 100644 --- a/accesstransformers/src/main/java/net/neoforged/jst/accesstransformers/ApplyATsVisitor.java +++ b/accesstransformers/src/main/java/net/neoforged/jst/accesstransformers/ApplyATsVisitor.java @@ -68,7 +68,25 @@ public void visitElement(@NotNull PsiElement element) { if (element instanceof PsiClass psiClass) { if (psiClass.getQualifiedName() != null) { String className = ClassUtil.getJVMClassName(psiClass); - if (!inheritMethodATs && !ats.containsClassTarget(className)) { + boolean shouldBeSkipped = !ats.containsClassTarget(className); + + // Classes may need changing that are not in the source at set. + // Check if any of this classe's parents are in the source at set first to potentially inherit their ATs. + if (shouldBeSkipped && this.inheritMethodATs) { + for ( + PsiClass parentType = psiClass.getSuperClass(); + parentType != null; + parentType = parentType.getSuperClass() + ) { + if (this.ats.containsClassTarget(ClassUtil.getJVMClassName(parentType))) { + // Process class as we inherit method ATs and we found *some* AT. + // Heuristic isn't perfect, but the alternative would be scanning *all* method targets for the parent owning type. + shouldBeSkipped = false; + break; + } + } + } + if (shouldBeSkipped) { // Skip this class and its children, but not the inner classes for (PsiClass innerClass : psiClass.getInnerClasses()) { visitElement(innerClass);