From 60198c0becffddabd43064aa971069d704007fbc Mon Sep 17 00:00:00 2001 From: Daniel Chen <108989218+Daniel1464@users.noreply.github.com> Date: Tue, 3 Dec 2024 23:30:55 -0500 Subject: [PATCH] [epilogue] Improved opt-in logging support (#7437) --- .../processor/AnnotationProcessor.java | 50 ++++-- .../epilogue/processor/LoggableHandler.java | 7 +- .../epilogue/processor/LoggerGenerator.java | 31 ++++ .../first/epilogue/processor/StringUtils.java | 2 +- .../processor/AnnotationProcessorTest.java | 143 ++++++++++++++++++ 5 files changed, 215 insertions(+), 18 deletions(-) diff --git a/epilogue-processor/src/main/java/edu/wpi/first/epilogue/processor/AnnotationProcessor.java b/epilogue-processor/src/main/java/edu/wpi/first/epilogue/processor/AnnotationProcessor.java index 22f0e89459..2781c055c8 100644 --- a/epilogue-processor/src/main/java/edu/wpi/first/epilogue/processor/AnnotationProcessor.java +++ b/epilogue-processor/src/main/java/edu/wpi/first/epilogue/processor/AnnotationProcessor.java @@ -11,9 +11,12 @@ import java.io.IOException; import java.util.ArrayList; import java.util.Comparator; import java.util.HashMap; +import java.util.LinkedHashSet; import java.util.List; import java.util.Map; import java.util.Set; +import java.util.stream.Collectors; +import java.util.stream.Stream; import javax.annotation.processing.AbstractProcessor; import javax.annotation.processing.RoundEnvironment; import javax.annotation.processing.SupportedAnnotationTypes; @@ -90,15 +93,15 @@ public class AnnotationProcessor extends AbstractProcessor { e); }); + var loggedTypes = getLoggedTypes(roundEnv); + // Handlers are declared in order of priority. If an element could be logged in more than one // way (eg a class implements both Sendable and StructSerializable), the order of the handlers // in this list will determine how it gets logged. m_handlers = List.of( new LoggableHandler( - processingEnv, - roundEnv.getElementsAnnotatedWith( - Logged.class)), // prioritize epilogue logging over Sendable + processingEnv, loggedTypes), // prioritize epilogue logging over Sendable new ConfiguredLoggerHandler( processingEnv, customLoggers), // then customized logging configs new ArrayHandler(processingEnv), @@ -118,12 +121,39 @@ public class AnnotationProcessor extends AbstractProcessor { .findAny() .ifPresent( epilogue -> { - processEpilogue(roundEnv, epilogue); + processEpilogue(roundEnv, epilogue, loggedTypes); }); return false; } + /** + * Gets the set of all loggable types in the compilation unit. A type is considered loggable if it + * is directly annotated with {@code @Logged} or contains a field or method with a {@code @Logged} + * annotation. + * + * @param roundEnv the compilation round environment + * @return the set of all loggable types + */ + private Set getLoggedTypes(RoundEnvironment roundEnv) { + // Fetches everything annotated with @Logged; classes, methods, values, etc. + var annotatedElements = roundEnv.getElementsAnnotatedWith(Logged.class); + return Stream.concat( + // 1. All type elements (classes, interfaces, or enums) with the @Logged annotation + annotatedElements.stream() + .filter(e -> e instanceof TypeElement) + .map(e -> (TypeElement) e), + // 2. All type elements containing a field or method with the @Logged annotation + annotatedElements.stream() + .filter(e -> e instanceof VariableElement || e instanceof ExecutableElement) + .map(Element::getEnclosingElement) + .filter(e -> e instanceof TypeElement) + .map(e -> (TypeElement) e)) + .sorted(Comparator.comparing(e -> e.getSimpleName().toString())) + .collect( + Collectors.toCollection(LinkedHashSet::new)); // Collect to a set to avoid duplicates + } + private boolean validateFields(Set annotatedElements) { var fields = annotatedElements.stream() @@ -340,7 +370,8 @@ public class AnnotationProcessor extends AbstractProcessor { return customLoggers; } - private void processEpilogue(RoundEnvironment roundEnv, TypeElement epilogueAnnotation) { + private void processEpilogue( + RoundEnvironment roundEnv, TypeElement epilogueAnnotation, Set loggedTypes) { var annotatedElements = roundEnv.getElementsAnnotatedWith(epilogueAnnotation); List loggerClassNames = new ArrayList<>(); @@ -358,12 +389,7 @@ public class AnnotationProcessor extends AbstractProcessor { return; } - var classes = - annotatedElements.stream() - .filter(e -> e instanceof TypeElement) - .map(e -> (TypeElement) e) - .toList(); - for (TypeElement clazz : classes) { + for (TypeElement clazz : loggedTypes) { try { warnOfNonLoggableElements(clazz); m_loggerGenerator.writeLoggerFile(clazz); @@ -391,7 +417,7 @@ public class AnnotationProcessor extends AbstractProcessor { private void warnOfNonLoggableElements(TypeElement clazz) { var config = clazz.getAnnotation(Logged.class); - if (config.strategy() == Logged.Strategy.OPT_IN) { + if (config == null || config.strategy() == Logged.Strategy.OPT_IN) { // field and method validations will have already checked everything return; } diff --git a/epilogue-processor/src/main/java/edu/wpi/first/epilogue/processor/LoggableHandler.java b/epilogue-processor/src/main/java/edu/wpi/first/epilogue/processor/LoggableHandler.java index 8dea822c4a..ba14022a2c 100644 --- a/epilogue-processor/src/main/java/edu/wpi/first/epilogue/processor/LoggableHandler.java +++ b/epilogue-processor/src/main/java/edu/wpi/first/epilogue/processor/LoggableHandler.java @@ -15,7 +15,6 @@ import javax.lang.model.element.ElementKind; import javax.lang.model.element.ExecutableElement; import javax.lang.model.element.TypeElement; import javax.lang.model.element.VariableElement; -import javax.lang.model.type.DeclaredType; import javax.lang.model.type.TypeKind; import javax.lang.model.type.TypeMirror; @@ -35,10 +34,8 @@ public class LoggableHandler extends ElementHandler { @Override public boolean isLoggable(Element element) { - var dataType = dataType(element); - return dataType.getAnnotation(Logged.class) != null - || dataType instanceof DeclaredType decl - && decl.asElement().getAnnotation(Logged.class) != null; + return m_processingEnv.getTypeUtils().asElement(dataType(element)) instanceof TypeElement t + && m_loggedTypes.contains(t); } @Override diff --git a/epilogue-processor/src/main/java/edu/wpi/first/epilogue/processor/LoggerGenerator.java b/epilogue-processor/src/main/java/edu/wpi/first/epilogue/processor/LoggerGenerator.java index 15fa2104dc..499d9912a7 100644 --- a/epilogue-processor/src/main/java/edu/wpi/first/epilogue/processor/LoggerGenerator.java +++ b/epilogue-processor/src/main/java/edu/wpi/first/epilogue/processor/LoggerGenerator.java @@ -15,6 +15,7 @@ import edu.wpi.first.epilogue.Logged; import edu.wpi.first.epilogue.NotLogged; import java.io.IOException; import java.io.PrintWriter; +import java.lang.annotation.Annotation; import java.util.ArrayDeque; import java.util.ArrayList; import java.util.Deque; @@ -42,6 +43,33 @@ public class LoggerGenerator { LoggerGenerator::isBuiltInJavaMethod; private final ProcessingEnvironment m_processingEnv; private final List m_handlers; + private final Logged m_defaultConfig = + new Logged() { + @Override + public Class annotationType() { + return Logged.class; + } + + @Override + public String name() { + return ""; + } + + @Override + public Strategy strategy() { + return Strategy.OPT_IN; + } + + @Override + public Importance importance() { + return Importance.DEBUG; + } + + @Override + public Naming defaultNaming() { + return Naming.USE_CODE_NAME; + } + }; public LoggerGenerator(ProcessingEnvironment processingEnv, List handlers) { this.m_processingEnv = processingEnv; @@ -76,6 +104,9 @@ public class LoggerGenerator { */ public void writeLoggerFile(TypeElement clazz) throws IOException { var config = clazz.getAnnotation(Logged.class); + if (config == null) { + config = m_defaultConfig; + } boolean requireExplicitOptIn = config.strategy() == Logged.Strategy.OPT_IN; Predicate notSkipped = LoggerGenerator::isNotSkipped; diff --git a/epilogue-processor/src/main/java/edu/wpi/first/epilogue/processor/StringUtils.java b/epilogue-processor/src/main/java/edu/wpi/first/epilogue/processor/StringUtils.java index 23fa2964ba..44063eed65 100644 --- a/epilogue-processor/src/main/java/edu/wpi/first/epilogue/processor/StringUtils.java +++ b/epilogue-processor/src/main/java/edu/wpi/first/epilogue/processor/StringUtils.java @@ -123,7 +123,7 @@ public final class StringUtils { String packageName = p.getQualifiedName().toString(); String className; - if (config.name().isEmpty()) { + if (config == null || config.name().isEmpty()) { className = String.join("$", nesting); } else { className = capitalize(config.name()).replaceAll(" ", ""); diff --git a/epilogue-processor/src/test/java/edu/wpi/first/epilogue/processor/AnnotationProcessorTest.java b/epilogue-processor/src/test/java/edu/wpi/first/epilogue/processor/AnnotationProcessorTest.java index cdfb99e082..f211999bfe 100644 --- a/epilogue-processor/src/test/java/edu/wpi/first/epilogue/processor/AnnotationProcessorTest.java +++ b/epilogue-processor/src/test/java/edu/wpi/first/epilogue/processor/AnnotationProcessorTest.java @@ -9,6 +9,7 @@ import static com.google.testing.compile.Compiler.javac; import static edu.wpi.first.epilogue.processor.CompileTestOptions.kJavaVersionOptions; import static org.junit.jupiter.api.Assertions.assertAll; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; import com.google.testing.compile.Compilation; import com.google.testing.compile.JavaFileObjects; @@ -59,6 +60,107 @@ class AnnotationProcessorTest { assertLoggerGenerates(source, expectedGeneratedSource); } + @Test + void optInFields() { + String source = + """ + package edu.wpi.first.epilogue; + + class Example { + @Logged double x; + @Logged int y; + } + """; + + String expectedGeneratedSource = + """ + package edu.wpi.first.epilogue; + + import edu.wpi.first.epilogue.Logged; + import edu.wpi.first.epilogue.Epilogue; + import edu.wpi.first.epilogue.logging.ClassSpecificLogger; + import edu.wpi.first.epilogue.logging.EpilogueBackend; + + public class ExampleLogger extends ClassSpecificLogger { + public ExampleLogger() { + super(Example.class); + } + + @Override + public void update(EpilogueBackend backend, Example object) { + if (Epilogue.shouldLog(Logged.Importance.DEBUG)) { + backend.log("x", object.x); + backend.log("y", object.y); + } + } + } + """; + + assertLoggerGenerates(source, expectedGeneratedSource); + } + + @Test + void optInMethods() { + String source = + """ + package edu.wpi.first.epilogue; + + class Example { + @Logged public double getValue() { return 2.0; } + @Logged public String getName() { return "Example"; } + } + """; + + String expectedGeneratedSource = + """ + package edu.wpi.first.epilogue; + + import edu.wpi.first.epilogue.Logged; + import edu.wpi.first.epilogue.Epilogue; + import edu.wpi.first.epilogue.logging.ClassSpecificLogger; + import edu.wpi.first.epilogue.logging.EpilogueBackend; + + public class ExampleLogger extends ClassSpecificLogger { + public ExampleLogger() { + super(Example.class); + } + + @Override + public void update(EpilogueBackend backend, Example object) { + if (Epilogue.shouldLog(Logged.Importance.DEBUG)) { + backend.log("getValue", object.getValue()); + backend.log("getName", object.getName()); + } + } + } + """; + + assertLoggerGenerates(source, expectedGeneratedSource); + } + + @Test + void shouldNotLog() { + String source = + """ + class Example { + public double getValue() { return 2.0; } + public String getName() { return "Example"; } + } + """; + + Compilation compilation = + javac() + .withOptions(kJavaVersionOptions) + .withProcessors(new AnnotationProcessor()) + .compile(JavaFileObjects.forSourceString("edu.wpi.first.epilogue.Example", source)); + + assertThat(compilation).succeeded(); + // nothing is annotated with @Logged; so, no logger file should be generated + assertTrue( + compilation.generatedSourceFiles().stream() + .noneMatch(jfo -> jfo.getName().contains("Example"))); + } + @Test void multiple() { String source = @@ -1416,6 +1518,47 @@ class AnnotationProcessorTest { assertLoggerGenerates(source, expectedRootLogger); } + @Test + void nestedOptIn() { + String source = + """ + package edu.wpi.first.epilogue; + + class Implicit { + @Logged double x; + } + + class Example { + @Logged Implicit i; + } + """; + + String expectedRootLogger = + """ + package edu.wpi.first.epilogue; + + import edu.wpi.first.epilogue.Logged; + import edu.wpi.first.epilogue.Epilogue; + import edu.wpi.first.epilogue.logging.ClassSpecificLogger; + import edu.wpi.first.epilogue.logging.EpilogueBackend; + + public class ExampleLogger extends ClassSpecificLogger { + public ExampleLogger() { + super(Example.class); + } + + @Override + public void update(EpilogueBackend backend, Example object) { + if (Epilogue.shouldLog(Logged.Importance.DEBUG)) { + Epilogue.implicitLogger.tryUpdate(backend.getNested("i"), object.i, Epilogue.getConfig().errorHandler); + } + } + } + """; + + assertLoggerGenerates(source, expectedRootLogger); + } + @Test void customLogger() { String source =