Add logging (#17)

README.md

@@ -28,11 +28,16 @@ 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>] [--enable-parchment
-           --parchment-mappings=<mappingsPath> [--parchment-javadoc]] INPUT OUTPUT
+           [--max-queue-depth=<maxQueueDepth>] [--out-format=<outputFormat>]
+           [--classpath=<addToClasspath>]... [--enable-parchment
+           --parchment-mappings=<mappingsPath> [--parchment-javadoc]] [--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.
+      --classpath=<addToClasspath>
+                            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
@@ -53,6 +58,14 @@ Plugin - parchment
       --enable-parchment    Enable parchment
       --parchment-javadoc
       --parchment-mappings=<mappingsPath>
+
+Plugin - accesstransformers
+      --access-transformer=<atFiles>
+
+      --access-transformer-validation=<validation>
+                            The level of validation to use for ats
+      --enable-accesstransformers
+                            Enable accesstransformers
 ```
 
 ## Licenses

accesstransformers/build.gradle

@@ -4,7 +4,7 @@ plugins {
 
 dependencies {
     implementation project(":api")
-    implementation 'net.neoforged.accesstransformers:at-parser:11.0.0'
+    implementation 'net.neoforged.accesstransformers:at-parser:11.0.4'
 
     testImplementation platform("org.junit:junit-bom:$junit_version")
     testImplementation 'org.junit.jupiter:junit-jupiter'

accesstransformers/src/main/java/net/neoforged/jst/accesstransformers/AccessTransformersTransformer.java

@@ -2,6 +2,9 @@
 
 import com.intellij.psi.PsiFile;
 import net.neoforged.accesstransformer.parser.AccessTransformerFiles;
+import net.neoforged.accesstransformer.parser.Target;
+import net.neoforged.accesstransformer.parser.Transformation;
+import net.neoforged.jst.api.Logger;
 import net.neoforged.jst.api.Replacements;
 import net.neoforged.jst.api.SourceTransformer;
 import net.neoforged.jst.api.TransformContext;
@@ -11,27 +14,60 @@
 import java.io.UncheckedIOException;
 import java.nio.file.Path;
 import java.util.List;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
 
 public class AccessTransformersTransformer implements SourceTransformer {
 
     @CommandLine.Option(names = "--access-transformer", required = true)
     public List<Path> atFiles;
 
+    @CommandLine.Option(names = "--access-transformer-validation", description = "The level of validation to use for ats")
+    public AccessTransformerValidation validation = AccessTransformerValidation.LOG;
+
     private AccessTransformerFiles ats;
+    private Map<Target, Transformation> pendingATs;
+    private Logger logger;
+    private volatile boolean errored;
+
     @Override
     public void beforeRun(TransformContext context) {
         ats = new AccessTransformerFiles();
-        try {
-            for (Path path : atFiles) {
+        logger = context.logger();
+
+        for (Path path : atFiles) {
+            try {
                 ats.loadFromPath(path);
+            } catch (IOException ex) {
+                logger.error("Failed to parse access transformer file %s: %s", path, ex.getMessage());
+                throw new UncheckedIOException(ex);
             }
-        } catch (IOException ex) {
-            throw new UncheckedIOException(ex);
         }
+
+        pendingATs = new ConcurrentHashMap<>(ats.getAccessTransformers());
+    }
+
+    @Override
+    public boolean afterRun(TransformContext context) {
+        if (!pendingATs.isEmpty()) {
+            pendingATs.forEach((target, transformation) -> logger.error("Access transformer %s, targeting %s did not apply as its target doesn't exist", transformation, target));
+            errored = true;
+        }
+
+        return !(errored && validation == AccessTransformerValidation.ERROR);
     }
 
     @Override
     public void visitFile(PsiFile psiFile, Replacements replacements) {
-        new ApplyATsVisitor(ats, replacements).visitFile(psiFile);
+        var visitor = new ApplyATsVisitor(ats, replacements, pendingATs, logger);
+        visitor.visitFile(psiFile);
+        if (visitor.errored) {
+            errored = true;
+        }
+    }
+
+    public enum AccessTransformerValidation {
+        LOG,
+        ERROR
     }
 }

accesstransformers/src/main/java/net/neoforged/jst/accesstransformers/ApplyATsVisitor.java

@@ -1,5 +1,6 @@
 package net.neoforged.jst.accesstransformers;
 
+import com.intellij.navigation.NavigationItem;
 import com.intellij.psi.PsiClass;
 import com.intellij.psi.PsiElement;
 import com.intellij.psi.PsiField;
@@ -13,14 +14,14 @@
 import net.neoforged.accesstransformer.parser.AccessTransformerFiles;
 import net.neoforged.accesstransformer.parser.Target;
 import net.neoforged.accesstransformer.parser.Transformation;
+import net.neoforged.jst.api.Logger;
 import net.neoforged.jst.api.PsiHelper;
 import net.neoforged.jst.api.Replacements;
 import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
 
 import java.util.Arrays;
 import java.util.EnumMap;
-import java.util.HashMap;
 import java.util.Map;
 import java.util.Optional;
 import java.util.Set;
@@ -35,12 +36,15 @@ class ApplyATsVisitor extends PsiRecursiveElementVisitor {
 
     private final AccessTransformerFiles ats;
     private final Replacements replacements;
-    private final Map<Target, Transformation> atCopy;
+    private final Map<Target, Transformation> pendingATs;
+    private final Logger logger;
+    boolean errored = false;
 
-    public ApplyATsVisitor(AccessTransformerFiles ats, Replacements replacements) {
+    public ApplyATsVisitor(AccessTransformerFiles ats, Replacements replacements, Map<Target, Transformation> pendingATs, Logger logger) {
         this.ats = ats;
         this.replacements = replacements;
-        atCopy = new HashMap<>(ats.getAccessTransformers());
+        this.logger = logger;
+        this.pendingATs = pendingATs;
     }
 
     @Override
@@ -56,51 +60,71 @@ public void visitElement(@NotNull PsiElement element) {
                     return;
                 }
 
-                apply(atCopy.get(new Target.ClassTarget(className)), psiClass, psiClass);
-                var fieldWildcard = atCopy.get(new Target.WildcardFieldTarget(className));
+                apply(pendingATs.remove(new Target.ClassTarget(className)), psiClass, psiClass);
+
+                var fieldWildcard = pendingATs.remove(new Target.WildcardFieldTarget(className));
                 if (fieldWildcard != null) {
                     for (PsiField field : psiClass.getFields()) {
                         // Apply a merged state if an explicit AT for the field already exists
-                        apply(merge(fieldWildcard, atCopy.remove(new Target.FieldTarget(className, field.getName()))), field, psiClass);
+                        var newState = merge(fieldWildcard, pendingATs.remove(new Target.FieldTarget(className, field.getName())));
+                        logger.debug("Applying field wildcard AT %s to %s in %s", newState, field.getName(), className);
+                        apply(newState, field, psiClass);
                     }
                 }
 
-                var methodWildcard = atCopy.get(new Target.WildcardMethodTarget(className));
+                var methodWildcard = pendingATs.remove(new Target.WildcardMethodTarget(className));
                 if (methodWildcard != null) {
                     for (PsiMethod method : psiClass.getMethods()) {
                         // Apply a merged state if an explicit AT for the method already exists
-                        apply(merge(methodWildcard, atCopy.remove(method(className, method))), method, psiClass);
+                        var newState = merge(methodWildcard, pendingATs.remove(method(className, method)));
+                        logger.debug("Applying method wildcard AT %s to %s in %s", newState, method.getName(), className);
+                        apply(newState, method, psiClass);
                     }
                 }
             }
         } else if (element instanceof PsiField field) {
             final var cls = field.getContainingClass();
             if (cls != null && cls.getQualifiedName() != null) {
                 String className = ClassUtil.getJVMClassName(cls);
-                apply(atCopy.get(new Target.FieldTarget(className, field.getName())), field, cls);
+                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(atCopy.get(method(className, method)), method, cls);
+                apply(pendingATs.remove(method(className, method)), method, cls);
             }
         }
 
         element.acceptChildren(this);
     }
 
-    // TODO - proper logging when an AT can't be applied
     private void apply(@Nullable Transformation at, PsiModifierListOwner owner, PsiClass containingClass) {
-        if (at == null || !at.isValid()) return;
+        if (at == null) return;
+        if (!at.isValid()) {
+            error("Found invalid access transformer: %s. Final state: conflicting", at);
+            return;
+        }
+
+        var targetInfo = new Object() {
+            @Override
+            public String toString() {
+                if (owner instanceof PsiClass cls) {
+                    return ClassUtil.getJVMClassName(cls);
+                }
+                return ((NavigationItem) owner).getName() + " of " + ClassUtil.getJVMClassName(containingClass);
+            }
+        };
+        logger.debug("Applying AT %s to %s", at, targetInfo);
+
         var modifiers = owner.getModifierList();
 
         var targetAcc = at.modifier();
 
         // If we're modifying a non-static interface method we can only make it public, meaning it must be defined as default
         if (containingClass.isInterface() && owner instanceof PsiMethod && !modifiers.hasModifierProperty(PsiModifier.STATIC)) {
             if (targetAcc != Transformation.Modifier.PUBLIC) {
-                System.err.println("Non-static interface methods can only be made public");
+                error("Access transformer %s targeting %s attempted to make a non-static interface method %s. They can only be made public.", at, targetInfo, targetAcc);
             } else {
                 for (var kw : modifiers.getChildren()) {
                     if (kw instanceof PsiKeyword && kw.getText().equals(PsiKeyword.PRIVATE)) { // Strip private, replace it with default
@@ -125,6 +149,8 @@ private void apply(@Nullable Transformation at, PsiModifierListOwner owner, PsiC
                     .filter(kw -> kw.getText().equals(PsiModifier.FINAL))
                     .findFirst()
                     .ifPresent(replacements::remove);
+        } else if (finalState == Transformation.FinalState.MAKEFINAL && !modifiers.hasModifierProperty(PsiModifier.FINAL)) {
+            error("Access transformer %s attempted to make %s final. Was non-final", at, targetInfo);
         }
     }
 
@@ -180,6 +206,11 @@ private void modify(Transformation.Modifier targetAcc, PsiModifierList modifiers
         }
     }
 
+    private void error(String message, Object... args) {
+        logger.error(message, args);
+        errored = true;
+    }
+
     private static String detectKind(PsiClass cls) {
         if (cls.isRecord()) {
             return "record";

api/src/main/java/net/neoforged/jst/api/Logger.java

@@ -0,0 +1,28 @@
+package net.neoforged.jst.api;
+
+import org.jetbrains.annotations.Nullable;
+
+import java.io.PrintStream;
+import java.util.Locale;
+
+public class Logger {
+    private final PrintStream debugOut;
+    private final PrintStream errorOut;
+
+    public Logger(@Nullable PrintStream debugOut, @Nullable PrintStream errorOut) {
+        this.debugOut = debugOut;
+        this.errorOut = errorOut;
+    }
+
+    public void error(String message, Object... args) {
+        if (errorOut != null) {
+            errorOut.printf(Locale.ROOT, message + "\n", args);
+        }
+    }
+
+    public void debug(String message, Object... args) {
+        if (debugOut != null) {
+            debugOut.printf(Locale.ROOT, message + "\n", args);
+        }
+    }
+}

api/src/main/java/net/neoforged/jst/api/SourceTransformer.java

@@ -6,7 +6,8 @@ public interface SourceTransformer {
     default void beforeRun(TransformContext context) {
     }
 
-    default void afterRun(TransformContext context) {
+    default boolean afterRun(TransformContext context) {
+        return true;
     }
 
     void visitFile(PsiFile psiFile, Replacements replacements);

api/src/main/java/net/neoforged/jst/api/TransformContext.java

@@ -1,4 +1,4 @@
 package net.neoforged.jst.api;
 
-public record TransformContext(IntelliJEnvironment environment, FileSource source, FileSink sink) {
+public record TransformContext(IntelliJEnvironment environment, FileSource source, FileSink sink, Logger logger) {
 }

cli/build.gradle

@@ -20,6 +20,7 @@ dependencies {
     implementation "info.picocli:picocli:$picocli_version"
     implementation project(":parchment")
     implementation project(":accesstransformers")
+    implementation 'org.slf4j:slf4j-simple:2.0.13'
 
     testImplementation platform("org.junit:junit-bom:$junit_version")
     testImplementation 'org.junit.jupiter:junit-jupiter'

cli/src/main/java/net/neoforged/jst/cli/Main.java

@@ -1,5 +1,6 @@
 package net.neoforged.jst.cli;
 
+import net.neoforged.jst.api.Logger;
 import net.neoforged.jst.api.SourceTransformer;
 import net.neoforged.jst.api.SourceTransformerPlugin;
 import net.neoforged.jst.cli.io.FileSinks;
@@ -37,6 +38,9 @@ public class Main implements Callable<Integer> {
     @CommandLine.Option(names = "--max-queue-depth", description = "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.")
     int maxQueueDepth = 100;
 
+    @CommandLine.Command(name = "--debug", description = "Print additional debugging information")
+    boolean debug = false;
+
     private final HashSet<SourceTransformer> enabledTransformers = new HashSet<>();
 
     public static void main(String[] args) {
@@ -59,9 +63,9 @@ public static int innerMain(String... args) {
 
     @Override
     public Integer call() throws Exception {
-
+        var logger = debug ? new Logger(System.out, System.err) : new Logger(null, System.err);
         try (var source = FileSources.create(inputPath, inputFormat);
-             var processor = new SourceFileProcessor()) {
+             var processor = new SourceFileProcessor(logger)) {
 
             if (librariesList != null) {
                 processor.addLibrariesList(librariesList);
@@ -75,7 +79,10 @@ public Integer call() throws Exception {
             var orderedTransformers = new ArrayList<>(enabledTransformers);
 
             try (var sink = FileSinks.create(outputPath, outputFormat, source)) {
-                processor.process(source, sink, orderedTransformers);
+                if (!processor.process(source, sink, orderedTransformers)) {
+                    logger.error("Transformation failed");
+                    return 1;
+                }
             }
 
         }