Add Problem Reporting API and provide Access Transformer Errors in a Machine Readable Format (#50)

Author: shartte

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

@@ -8,17 +8,30 @@
 import net.neoforged.jst.api.Replacements;
 import net.neoforged.jst.api.SourceTransformer;
 import net.neoforged.jst.api.TransformContext;
+import net.neoforged.problems.Problem;
+import net.neoforged.problems.ProblemGroup;
+import net.neoforged.problems.ProblemId;
+import net.neoforged.problems.ProblemLocation;
+import net.neoforged.problems.ProblemReporter;
+import net.neoforged.problems.ProblemSeverity;
 import picocli.CommandLine;
 
-import java.io.IOException;
-import java.io.UncheckedIOException;
 import java.nio.file.Path;
+import java.nio.file.Paths;
 import java.util.List;
 import java.util.Map;
 import java.util.concurrent.ConcurrentHashMap;
+import java.util.regex.Pattern;
 
 public class AccessTransformersTransformer implements SourceTransformer {
 
+    private static final ProblemGroup PROBLEM_GROUP = ProblemGroup.create("access-transformer", "Access Transformers");
+    static final ProblemId INVALID_AT = ProblemId.create("invalid-at", "Invalid", PROBLEM_GROUP);
+    private static final ProblemId MISSING_TARGET = ProblemId.create("missing-target", "Missing Target", PROBLEM_GROUP);
+
+    private static final Pattern LINE_PATTERN = Pattern.compile("\\bline\\s+(\\d+)");
+    private static final Pattern ORIGIN_PATTERN = Pattern.compile("(.*):(\\d+)$");
+
     @CommandLine.Option(names = "--access-transformer", required = true)
     public List<Path> atFiles;
 
@@ -28,19 +41,36 @@ public class AccessTransformersTransformer implements SourceTransformer {
     private AccessTransformerFiles ats;
     private Map<Target, Transformation> pendingATs;
     private Logger logger;
+    private ProblemReporter problemReporter;
     private volatile boolean errored;
 
     @Override
     public void beforeRun(TransformContext context) {
         ats = new AccessTransformerFiles();
         logger = context.logger();
+        problemReporter = context.problemReporter();
 
         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 (Exception e) {
+                logger.error("Failed to parse access transformer file %s: %s", path, e.getMessage());
+
+                if (e.getMessage() != null) {
+                    var m = LINE_PATTERN.matcher(e.getMessage());
+                    if (m.matches()) {
+                        // The AT parser internally uses 0-based line numbering, but the problem reporter uses 1-based
+                        int line = 1 + Integer.parseUnsignedInt(m.group(1));
+                        problemReporter.report(INVALID_AT, ProblemSeverity.ERROR, ProblemLocation.ofLocationInFile(path, line), e.getMessage());
+                    } else {
+                        problemReporter.report(INVALID_AT, ProblemSeverity.ERROR, ProblemLocation.ofFile(path), e.getMessage());
+                    }
+                }
+
+                if (e instanceof RuntimeException re) {
+                    throw re;
+                }
+                throw new RuntimeException(e);
             }
         }
 
@@ -55,16 +85,39 @@ public boolean afterRun(TransformContext context) {
                 // so we don't log the ClassTarget as that will cause duplication
                 if (target instanceof Target.ClassTarget && target.className().contains("$")) return;
                 logger.error("Access transformer %s, targeting %s did not apply as its target doesn't exist", transformation, target);
+
+                var problem = Problem.builder(MISSING_TARGET)
+                        .severity(ProblemSeverity.ERROR)
+                        .contextualLabel("The target " + target + " does not exist.")
+                        .build();
+                reportProblem(problemReporter, transformation, problem);
             });
             errored = true;
         }
 
         return !(errored && validation == AccessTransformerValidation.ERROR);
     }
 
+    static void reportProblem(ProblemReporter problemReporter, Transformation transformation, Problem problem) {
+        // Report a problem for each origin of the transform
+        for (String origin : transformation.origins()) {
+            var m = ORIGIN_PATTERN.matcher(origin);
+            ProblemLocation problemLocation;
+            if (!m.matches()) {
+                problemLocation = ProblemLocation.ofFile(Paths.get(origin));
+            } else {
+                var file = Path.of(m.group(1));
+                var line = Integer.parseUnsignedInt(m.group(2));
+                problemLocation = ProblemLocation.ofLocationInFile(file, line);
+            }
+
+            problemReporter.report(Problem.builder(problem).location(problemLocation).build());
+        }
+    }
+
     @Override
     public void visitFile(PsiFile psiFile, Replacements replacements) {
-        var visitor = new ApplyATsVisitor(ats, replacements, pendingATs, logger);
+        var visitor = new ApplyATsVisitor(ats, replacements, pendingATs, logger, problemReporter);
         visitor.visitFile(psiFile);
         if (visitor.errored) {
             errored = true;

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

@@ -14,17 +14,20 @@
 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;
 import net.neoforged.jst.api.Logger;
 import net.neoforged.jst.api.PsiHelper;
 import net.neoforged.jst.api.Replacements;
+import net.neoforged.problems.Problem;
+import net.neoforged.problems.ProblemReporter;
 import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
 
+import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.Collections;
 import java.util.EnumMap;
 import java.util.Locale;
 import java.util.Map;
@@ -47,13 +50,15 @@ class ApplyATsVisitor extends PsiRecursiveElementVisitor {
     private final Replacements replacements;
     private final Map<Target, Transformation> pendingATs;
     private final Logger logger;
+    private final ProblemReporter problemReporter;
     boolean errored = false;
 
-    public ApplyATsVisitor(AccessTransformerFiles ats, Replacements replacements, Map<Target, Transformation> pendingATs, Logger logger) {
+    public ApplyATsVisitor(AccessTransformerFiles ats, Replacements replacements, Map<Target, Transformation> pendingATs, Logger logger, ProblemReporter problemReporter) {
         this.ats = ats;
         this.replacements = replacements;
         this.logger = logger;
         this.pendingATs = pendingATs;
+        this.problemReporter = problemReporter;
     }
 
     @Override
@@ -119,7 +124,7 @@ public void visitElement(@NotNull PsiElement element) {
     private void apply(@Nullable Transformation at, PsiModifierListOwner owner, PsiClass containingClass) {
         if (at == null) return;
         if (!at.isValid()) {
-            error("Found invalid access transformer: %s. Final state: conflicting", at);
+            error(at, "Found invalid access transformer. Final state: conflicting");
             return;
         }
 
@@ -147,7 +152,7 @@ public String toString() {
         // 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) {
-                error("Access transformer %s targeting %s attempted to make a non-static interface method %s. They can only be made public.", at, targetInfo, targetAcc);
+                error(at, "Access transformer targeting %s attempted to make a non-static interface method %s. They can only be made public.", targetInfo, targetAcc);
             } else {
                 for (var kw : modifiers.getChildren()) {
                     if (kw instanceof PsiKeyword && kw.getText().equals(PsiKeyword.PRIVATE)) { // Strip private, replace it with default
@@ -158,7 +163,7 @@ public String toString() {
             }
         } else if (containingClass.isEnum() && owner instanceof PsiMethod mtd && mtd.isConstructor() && at.modifier().ordinal() < Transformation.Modifier.DEFAULT.ordinal()) {
             // Enum constructors can at best be package-private, any other attempt must be prevented
-            error("Access transformer %s targeting %s attempted to make an enum constructor %s", at, targetInfo, at.modifier());
+            error(at, "Access transformer targeting %s attempted to make an enum constructor %s", targetInfo, at.modifier());
         } else if (targetAcc.ordinal() < detectModifier(modifiers, null).ordinal()) { // PUBLIC (0) < PROTECTED (1) < DEFAULT (2) < PRIVATE (3)
             modify(targetAcc, modifiers, Arrays.stream(modifiers.getChildren())
                     .filter(el -> el instanceof PsiKeyword)
@@ -176,7 +181,7 @@ public String toString() {
                     .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);
+            error(at, "Access transformer attempted to make %s final. Was non-final", targetInfo);
         }
     }
 
@@ -250,11 +255,11 @@ private void checkImplicitConstructor(PsiClass psiClass, String className, @Null
 
             var implicitAT = pendingATs.remove(new Target.MethodTarget(className, "<init>", desc));
             if (implicitAT != null && implicitAT.modifier() != detectModifier(psiClass.getModifierList(), classAt)) {
-                error("Access transformer %s targeting the implicit constructor of %s is not valid, as a record's constructor must have the same access level as the record class. Please AT the record too: \"%s\"", implicitAT, className,
+                error(implicitAT, "Access transformer targeting the implicit constructor of %s is not valid, as a record's constructor must have the same access level as the record class. Please AT the record too: \"%s\"", className,
                         implicitAT.modifier().toString().toLowerCase(Locale.ROOT) + " " + className);
                 pendingATs.remove(new Target.MethodTarget(className, "<init>", desc));
             } else if (classAt != null && detectModifier(psiClass.getModifierList(), null).ordinal() > classAt.modifier().ordinal() && implicitAT == null) {
-                error("Access transformer %s targeting record class %s attempts to widen its access without widening the constructor's access. You must AT the constructor too: \"%s\"", classAt, className,
+                error(classAt, "Access transformer targeting record class %s attempts to widen its access without widening the constructor's access. You must AT the constructor too: \"%s\"", className,
                         classAt.modifier().toString().toLowerCase(Locale.ROOT) + " " + className + " <init>" + desc);
                 pendingATs.remove(new Target.MethodTarget(className, "<init>", desc));
             }
@@ -294,8 +299,16 @@ private void injectConstructor(PsiClass psiClass, String className, Transformati
                 " ".repeat(indent) + modifierString + psiClass.getName() + "() {}");
     }
 
-    private void error(String message, Object... args) {
-        logger.error(message, args);
+    private void error(Transformation transformation, String message, Object... args) {
+        var problem = Problem.builder(AccessTransformersTransformer.INVALID_AT)
+                .contextualLabel(String.format(Locale.ROOT, message, args))
+                .build();
+        AccessTransformersTransformer.reportProblem(problemReporter, transformation, problem);
+
+        var formatArgs = new ArrayList<>();
+        Collections.addAll(formatArgs, args);
+        formatArgs.add(transformation);
+        logger.error(message + " at %s", formatArgs.toArray());
         errored = true;
     }
 

api/build.gradle

@@ -19,6 +19,7 @@ java {
 dependencies {
     api "com.jetbrains.intellij.java:java-psi-impl:$intellij_version"
     api "info.picocli:picocli:$picocli_version"
+    api "net.neoforged.installertools:problems-api:$problems_api_version"
     compileOnly "org.jetbrains:annotations:$jetbrains_annotations_version"
 }
 

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

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

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

@@ -5,13 +5,17 @@
 import net.neoforged.jst.api.SourceTransformerPlugin;
 import net.neoforged.jst.cli.io.FileSinks;
 import net.neoforged.jst.cli.io.FileSources;
+import net.neoforged.problems.FileProblemReporter;
+import net.neoforged.problems.ProblemReporter;
+import org.jetbrains.annotations.Nullable;
 import org.jetbrains.annotations.VisibleForTesting;
 import picocli.CommandLine;
 
 import java.nio.file.Path;
 import java.util.ArrayList;
 import java.util.HashSet;
 import java.util.List;
+import java.util.Objects;
 import java.util.ServiceLoader;
 import java.util.concurrent.Callable;
 
@@ -41,9 +45,12 @@ 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")
+    @CommandLine.Option(names = "--debug", description = "Print additional debugging information")
     boolean debug = false;
 
+    @CommandLine.Option(names = "--problems-report", description = "Write problems to this report file.")
+    Path problemsReport;
+
     private final HashSet<SourceTransformer> enabledTransformers = new HashSet<>();
 
     public static void main(String[] args) {
@@ -68,7 +75,8 @@ public static int innerMain(String... args) {
     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(logger)) {
+             var problemReporter = createProblemReporter(problemsReport);
+             var processor = new SourceFileProcessor(logger, Objects.requireNonNullElse(problemReporter, ProblemReporter.NOOP))) {
 
             if (librariesList != null) {
                 processor.addLibrariesList(librariesList);
@@ -96,6 +104,15 @@ public Integer call() throws Exception {
         return 0;
     }
 
+    @Nullable
+    private FileProblemReporter createProblemReporter(Path problemsReport) {
+        if (problemsReport == null) {
+            return null;
+        } else {
+            return new FileProblemReporter(problemsReport);
+        }
+    }
+
     private void setupPluginCliOptions(List<SourceTransformerPlugin> plugins, CommandLine.Model.CommandSpec spec) {
         for (var plugin : plugins) {
             var transformer = plugin.createTransformer();

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

@@ -13,6 +13,7 @@
 import net.neoforged.jst.api.TransformContext;
 import net.neoforged.jst.cli.intellij.ClasspathSetup;
 import net.neoforged.jst.cli.intellij.IntelliJEnvironmentImpl;
+import net.neoforged.problems.ProblemReporter;
 
 import java.io.IOException;
 import java.io.UncheckedIOException;
@@ -33,11 +34,13 @@ class SourceFileProcessor implements AutoCloseable {
     private final IntelliJEnvironmentImpl ijEnv;
     private int maxQueueDepth = 50;
     private final Logger logger;
+    private final ProblemReporter problemReporter;
 
     private final List<String> ignoredPrefixes = new ArrayList<>();
 
-    public SourceFileProcessor(Logger logger) throws IOException {
+    public SourceFileProcessor(Logger logger, ProblemReporter problemReporter) throws IOException {
         this.logger = logger;
+        this.problemReporter = problemReporter;
         ijEnv = new IntelliJEnvironmentImpl(logger);
         ijEnv.addCurrentJdkToClassPath();
     }
@@ -47,7 +50,7 @@ public boolean process(FileSource source, FileSink sink, List<SourceTransformer>
             throw new IllegalStateException("Cannot have an input with possibly more than one file when the output is a single file.");
         }
 
-        var context = new TransformContext(ijEnv, source, sink, logger);
+        var context = new TransformContext(ijEnv, source, sink, logger, problemReporter);
 
         var sourceRoot = source.createSourceRoot(VirtualFileManager.getInstance());
         ijEnv.addSourceRoot(sourceRoot);

gradle.properties

@@ -5,3 +5,4 @@ picocli_version=4.7.6
 junit_version=5.10.3
 assertj_version=3.26.0
 gson_version=2.10.1
+problems_api_version=3.0.3

settings.gradle

@@ -1,3 +1,5 @@
+import org.gradle.api.initialization.resolve.RepositoriesMode
+
 pluginManagement {
     repositories {
         gradlePluginPortal()
@@ -13,6 +15,7 @@ plugins {
 }
 
 dependencyResolutionManagement {
+    repositoriesMode = RepositoriesMode.FAIL_ON_PROJECT_REPOS
     repositories {
         mavenCentral()
         maven {

tests/data/accesstransformer/classes/expected_report.json

@@ -0,0 +1 @@
+[]

tests/data/accesstransformer/fields/expected_report.json

@@ -0,0 +1 @@
+[]