Fix epilogue generating incorrect packages

wpilibsuite · Nov 28, 2024 · 3443b39 · 3443b39
1 parent b6de7ac
commit 3443b39
Show file tree

Hide file tree

Showing 3 changed files with 161 additions and 46 deletions.
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
@@ -15,7 +15,9 @@
 import edu.wpi.first.epilogue.NotLogged;
 import java.io.IOException;
 import java.io.PrintWriter;
+import java.util.ArrayDeque;
 import java.util.ArrayList;
+import java.util.Deque;
 import java.util.EnumMap;
 import java.util.HashMap;
 import java.util.List;
@@ -29,6 +31,7 @@
 import javax.lang.model.element.ExecutableElement;
 import javax.lang.model.element.Modifier;
 import javax.lang.model.element.Name;
+import javax.lang.model.element.PackageElement;
 import javax.lang.model.element.TypeElement;
 import javax.lang.model.element.VariableElement;
 import javax.tools.Diagnostic;
@@ -150,36 +153,28 @@ public void writeLoggerFile(TypeElement clazz) throws IOException {
           }
         });
 
-    writeLoggerFile(clazz.getQualifiedName().toString(), config, fieldsToLog, methodsToLog);
+    writeLoggerFile(clazz, config, fieldsToLog, methodsToLog);
   }
 
   private void writeLoggerFile(
-      String className,
+      TypeElement clazz,
       Logged classConfig,
       List<VariableElement> loggableFields,
       List<ExecutableElement> loggableMethods)
       throws IOException {
-    String packageName = null;
-    int lastDot = className.lastIndexOf('.');
-    if (lastDot > 0) {
-      packageName = className.substring(0, lastDot);
+    // Walk nesting levels, to support inner classes
+    Deque<String> nesting = new ArrayDeque<>();
+    Element enclosing = clazz.getEnclosingElement();
+    while (!(enclosing instanceof PackageElement p)) {
+      nesting.addFirst(enclosing.getSimpleName().toString());
+      enclosing = enclosing.getEnclosingElement();
     }
+    String packageName = p.getQualifiedName().toString();
+    nesting.addLast(clazz.getSimpleName().toString());
+    String simpleClassName = String.join(".", nesting);
 
-    String simpleClassName = StringUtils.simpleName(className);
-    String loggerClassName = className + "Logger";
-    String loggerSimpleClassName = loggerClassName.substring(lastDot + 1);
-
-    // Use the name on the class config to set the generated logger names
-    // This helps to avoid naming conflicts
-    if (!classConfig.name().isBlank()) {
-      loggerSimpleClassName =
-          StringUtils.capitalize(classConfig.name().replaceAll(" ", "")) + "Logger";
-      if (lastDot > 0) {
-        loggerClassName = packageName + "." + loggerSimpleClassName;
-      } else {
-        loggerClassName = loggerSimpleClassName;
-      }
-    }
+    String loggerClassName = StringUtils.loggerClassName(clazz);
+    String loggerSimpleClassName = StringUtils.simpleName(loggerClassName);
 
     var loggerFile = m_processingEnv.getFiler().createSourceFile(loggerClassName);
 
@@ -220,13 +215,13 @@ private void writeLoggerFile(
         }
         out.println();
 
-        var clazz = simpleClassName + ".class";
+        var classReference = simpleClassName + ".class";
 
         out.println("  static {");
         out.println("    try {");
         out.println(
             "      var lookup = MethodHandles.privateLookupIn("
-                + clazz
+                + classReference
                 + ", MethodHandles.lookup());");
 
         for (var privateField : privateFields) {
@@ -235,7 +230,7 @@ private void writeLoggerFile(
               "      $"
                   + fieldName
                   + " = lookup.findVarHandle("
-                  + clazz
+                  + classReference
                   + ", \""
                   + fieldName
                   + "\", "

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
@@ -5,9 +5,13 @@
 package edu.wpi.first.epilogue.processor;
 
 import edu.wpi.first.epilogue.Logged;
+import java.util.ArrayDeque;
 import java.util.Arrays;
+import java.util.Deque;
 import java.util.List;
 import java.util.stream.Collectors;
+import javax.lang.model.element.Element;
+import javax.lang.model.element.PackageElement;
 import javax.lang.model.element.TypeElement;
 
 public final class StringUtils {
@@ -108,34 +112,24 @@ public static String loggerFieldName(TypeElement clazz) {
    */
   public static String loggerClassName(TypeElement clazz) {
     var config = clazz.getAnnotation(Logged.class);
-    var className = clazz.getQualifiedName().toString();
 
-    String packageName;
-    int lastDot = className.lastIndexOf('.');
-    if (lastDot <= 0) {
-      packageName = null;
-    } else {
-      packageName = className.substring(0, lastDot);
+    Deque<String> nesting = new ArrayDeque<>();
+    Element enclosing = clazz.getEnclosingElement();
+    while (!(enclosing instanceof PackageElement p)) {
+      nesting.addFirst(enclosing.getSimpleName().toString());
+      enclosing = enclosing.getEnclosingElement();
     }
+    nesting.addLast(clazz.getSimpleName().toString());
+    String packageName = p.getQualifiedName().toString();
 
-    String loggerClassName;
-
-    // Use the name on the class config to set the generated logger names
-    // This helps to avoid naming conflicts
-    if (config.name().isBlank()) {
-      loggerClassName = className + "Logger";
+    String className;
+    if (config.name().isEmpty()) {
+      className = String.join("$", nesting);
     } else {
-      String cleaned = config.name().replaceAll(" ", "");
-
-      var loggerSimpleClassName = StringUtils.capitalize(cleaned) + "Logger";
-      if (packageName != null) {
-        loggerClassName = packageName + "." + loggerSimpleClassName;
-      } else {
-        loggerClassName = loggerSimpleClassName;
-      }
+      className = config.name();
     }
 
-    return loggerClassName;
+    return packageName + "." + className + "Logger";
   }
 
   /**

diff --git a/...gue-processor/src/test/java/edu/wpi/first/epilogue/processor/AnnotationProcessorTest.java b/...gue-processor/src/test/java/edu/wpi/first/epilogue/processor/AnnotationProcessorTest.java
@@ -1166,6 +1166,132 @@ public void update(DataLogger dataLogger, Example object) {
     assertLoggerGenerates(source, expectedRootLogger);
   }
 
+  @Test
+  void innerClasses() {
+    String source =
+        """
+        package edu.wpi.first.epilogue;
+
+        class Outer {
+          @Logged
+          class Example { // Deliberately nonstatic
+            double x;
+          }
+        }
+        """;
+
+    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.DataLogger;
+
+        public class Outer$ExampleLogger extends ClassSpecificLogger<Outer.Example> {
+          public Outer$ExampleLogger() {
+            super(Outer.Example.class);
+          }
+
+          @Override
+          public void update(DataLogger dataLogger, Outer.Example object) {
+            if (Epilogue.shouldLog(Logged.Importance.DEBUG)) {
+              dataLogger.log("x", object.x);
+            }
+          }
+        }
+        """;
+
+    assertLoggerGenerates(source, expectedRootLogger);
+  }
+
+  @Test
+  void highlyNestedInnerClasses() {
+    String source =
+        """
+        package edu.wpi.first.epilogue;
+
+        class A {
+          class B {
+            class C {
+              class D {
+                @Logged
+                class Example {
+                  double x;
+                }
+              }
+            }
+          }
+        }
+        """;
+
+    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.DataLogger;
+
+        public class A$B$C$D$ExampleLogger extends ClassSpecificLogger<A.B.C.D.Example> {
+          public A$B$C$D$ExampleLogger() {
+            super(A.B.C.D.Example.class);
+          }
+
+          @Override
+          public void update(DataLogger dataLogger, A.B.C.D.Example object) {
+            if (Epilogue.shouldLog(Logged.Importance.DEBUG)) {
+              dataLogger.log("x", object.x);
+            }
+          }
+        }
+        """;
+
+    assertLoggerGenerates(source, expectedRootLogger);
+  }
+
+  @Test
+  void renamedInnerClass() {
+    String source =
+        """
+        package edu.wpi.first.epilogue;
+
+        class Outer {
+          @Logged(name = "CustomExample") // For the sake of testing, needs "Example" somewhere in the name
+          class Example {
+            double x;
+          }
+        }
+        """;
+
+    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.DataLogger;
+
+        public class CustomExampleLogger extends ClassSpecificLogger<Outer.Example> {
+          public CustomExampleLogger() {
+            super(Outer.Example.class);
+          }
+
+          @Override
+          public void update(DataLogger dataLogger, Outer.Example object) {
+            if (Epilogue.shouldLog(Logged.Importance.DEBUG)) {
+              dataLogger.log("x", object.x);
+            }
+          }
+        }
+        """;
+
+    assertLoggerGenerates(source, expectedRootLogger);
+  }
+
   @Test
   void diamondInheritance() {
     String source =