[epilogue] Autogenerate nicer data names by default, not just raw element names (#7167)

eg "getFoo()" will now be logged as "Foo", or "m_leftMotor" as "Left Motor" It is now a compilation error to reuse the same logged name for multiple elements (since whatever is declared last would overwrite anything logged before it) Do not log record fields (just use the accessors). This also fixes an issue where records could never be logged due to identical member and accessor names Also skips toString, hashCode, and clone methods when generating loggers
2026-07-02 02:51:42 +00:00 · 2024-10-31 23:34:00 -04:00
parent aaf139320e
commit 0f313c672f
6 changed files with 496 additions and 47 deletions
--- a/epilogue-processor/src/main/java/edu/wpi/first/epilogue/processor/ElementHandler.java
+++ b/epilogue-processor/src/main/java/edu/wpi/first/epilogue/processor/ElementHandler.java
@@ -61,15 +61,46 @@ public abstract class ElementHandler {
   * @return the name specified in the {@link Logged @Logged} annotation on the element, if present;
   *     otherwise, the field or method's name with no modifications
   */
-  public String loggedName(Element element) {
-    var elementName = element.getSimpleName().toString();
-    var config = element.getAnnotation(Logged.class);
+  public static String loggedName(Element element) {
+    var elementConfig = element.getAnnotation(Logged.class);

-    if (config != null && !config.name().isBlank()) {
-      return config.name();
-    } else {
-      return elementName;
+    // Use the name provided on the logged element, if one is present
+    if (elementConfig != null && !elementConfig.name().isBlank()) {
+      return elementConfig.name();
    }
+
+    var config = elementConfig;
+
+    if (config == null) {
+      // Look up the parent class configuration
+      // We assume one is present, since logged elements should only be found if the enclosing class
+      // is @Logged itself
+      Logged parentConfig = null;
+      for (var parent = element.getEnclosingElement();
+          parent != null;
+          parent = parent.getEnclosingElement()) {
+        parentConfig = parent.getAnnotation(Logged.class);
+        if (parentConfig != null) {
+          break;
+        }
+      }
+
+      config = parentConfig;
+    }
+
+    if (config == null) {
+      // Uh oh
+      throw new IllegalStateException(
+          "Could not generate a name for element "
+              + element
+              + " without a @Logged annotation AND without being contained within a class with a @Logged annotation!\n\nOpen an issue at https://github.com/wpilibsuite/allwpilib/issues and include a copy of the file that caused this error.");
+    }
+
+    var elementName = element.getSimpleName().toString();
+    return switch (config.defaultNaming()) {
+      case USE_CODE_NAME -> elementName;
+      case USE_HUMAN_NAME -> StringUtils.toHumanName(elementName);
+    };
  }

  /**
--- 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
@@ -7,23 +7,36 @@ package edu.wpi.first.epilogue.processor;
 import static java.util.stream.Collectors.groupingBy;
 import static java.util.stream.Collectors.toList;

+import com.sun.source.tree.IdentifierTree;
+import com.sun.source.tree.ReturnTree;
+import com.sun.source.util.SimpleTreeVisitor;
+import com.sun.source.util.Trees;
 import edu.wpi.first.epilogue.Logged;
 import edu.wpi.first.epilogue.NotLogged;
 import java.io.IOException;
 import java.io.PrintWriter;
+import java.util.ArrayList;
 import java.util.EnumMap;
+import java.util.HashMap;
 import java.util.List;
+import java.util.Map;
+import java.util.Objects;
 import java.util.function.Predicate;
+import java.util.stream.Collectors;
 import java.util.stream.Stream;
 import javax.annotation.processing.ProcessingEnvironment;
 import javax.lang.model.element.Element;
 import javax.lang.model.element.ExecutableElement;
 import javax.lang.model.element.Modifier;
+import javax.lang.model.element.Name;
 import javax.lang.model.element.TypeElement;
 import javax.lang.model.element.VariableElement;
+import javax.tools.Diagnostic;

 /** Generates logger class files for {@link Logged @Logged}-annotated classes. */
 public class LoggerGenerator {
+  public static final Predicate<ExecutableElement> kIsBuiltInJavaMethod =
+      LoggerGenerator::isBuiltInJavaMethod;
  private final ProcessingEnvironment m_processingEnv;
  private final List<ElementHandler> m_handlers;

@@ -36,6 +49,19 @@ public class LoggerGenerator {
    return e.getAnnotation(NotLogged.class) == null;
  }

+  /**
+   * Checks if a method is a method declared in java.lang.Object that should not be logged.
+   *
+   * @param e the method to check
+   * @return true if the method is toString, hashCode, or clone; false otherwise
+   */
+  private static boolean isBuiltInJavaMethod(ExecutableElement e) {
+    Name name = e.getSimpleName();
+    return name.contentEquals("toString")
+        || name.contentEquals("hashCode")
+        || name.contentEquals("clone");
+  }
+
  /**
   * Generates the logger class used to handle data objects of the given type. The generated logger
   * class will subclass from {@link edu.wpi.first.epilogue.logging.ClassSpecificLogger} and
@@ -53,17 +79,23 @@ public class LoggerGenerator {
    Predicate<Element> optedIn =
        e -> !requireExplicitOptIn || e.getAnnotation(Logged.class) != null;

-    var fieldsToLog =
-        clazz.getEnclosedElements().stream()
-            .filter(e -> e instanceof VariableElement)
-            .map(e -> (VariableElement) e)
-            .filter(notSkipped)
-            .filter(optedIn)
-            .filter(e -> !e.getModifiers().contains(Modifier.STATIC))
-            .filter(this::isLoggable)
-            .toList();
+    List<VariableElement> fieldsToLog;
+    if (Objects.equals(clazz.getSuperclass().toString(), "java.lang.Record")) {
+      // Do not log record members - just use the accessor methods
+      fieldsToLog = List.of();
+    } else {
+      fieldsToLog =
+          clazz.getEnclosedElements().stream()
+              .filter(e -> e instanceof VariableElement)
+              .map(e -> (VariableElement) e)
+              .filter(notSkipped)
+              .filter(optedIn)
+              .filter(e -> !e.getModifiers().contains(Modifier.STATIC))
+              .filter(this::isLoggable)
+              .toList();
+    }

-    var methodsToLog =
+    List<ExecutableElement> methodsToLog =
        clazz.getEnclosedElements().stream()
            .filter(e -> e instanceof ExecutableElement)
            .map(e -> (ExecutableElement) e)
@@ -73,9 +105,51 @@ public class LoggerGenerator {
            .filter(e -> e.getModifiers().contains(Modifier.PUBLIC))
            .filter(e -> e.getParameters().isEmpty())
            .filter(e -> e.getReceiverType() != null)
+            .filter(kIsBuiltInJavaMethod.negate())
            .filter(this::isLoggable)
+            .filter(e -> !isSimpleGetterMethodForLoggedField(e, fieldsToLog))
            .toList();

+    // Validate no name collisions
+    Map<String, List<Element>> usedNames =
+        Stream.concat(fieldsToLog.stream(), methodsToLog.stream())
+            .reduce(
+                new HashMap<>(),
+                (map, element) -> {
+                  String name = ElementHandler.loggedName(element);
+                  map.computeIfAbsent(name, _k -> new ArrayList<>()).add(element);
+
+                  return map;
+                },
+                (left, right) -> {
+                  left.putAll(right);
+                  return left;
+                });
+
+    usedNames.forEach(
+        (name, elements) -> {
+          if (elements.size() > 1) {
+            // Collisions!
+            for (Element conflictingElement : elements) {
+              String conflicts =
+                  elements.stream()
+                      .filter(e -> !e.equals(conflictingElement))
+                      .map(e -> e.getEnclosingElement().getSimpleName() + "." + e)
+                      .collect(Collectors.joining(", "));
+
+              m_processingEnv
+                  .getMessager()
+                  .printMessage(
+                      Diagnostic.Kind.ERROR,
+                      "[EPILOGUE] Conflicting name detected: \""
+                          + name
+                          + "\" is also used by "
+                          + conflicts,
+                      conflictingElement);
+            }
+          }
+        });
+
    writeLoggerFile(clazz.getQualifiedName().toString(), config, fieldsToLog, methodsToLog);
  }

@@ -242,4 +316,55 @@ public class LoggerGenerator {
  private boolean isLoggable(Element element) {
    return m_handlers.stream().anyMatch(h -> h.isLoggable(element));
  }
+
+  /**
+   * Checks if a method is a simple "getter" method for a field in the given list. Here, we define
+   * "getter" as a method with a single return statement that references the name of a field, with
+   * no other expressions. `getX() { return x; }` would be considered a "getter" method, while
+   * `getX() { return x.clone(); }` would not be. Note that the method name is irrelevant; only the
+   * method body is checked.
+   *
+   * @param ex the method to check
+   * @param fieldsToLog the fields that will already be logged
+   * @return true if the method is a simple "getter" method, false otherwise
+   */
+  private boolean isSimpleGetterMethodForLoggedField(
+      ExecutableElement ex, List<VariableElement> fieldsToLog) {
+    var trees = Trees.instance(m_processingEnv);
+    var methodTree = trees.getTree(ex);
+
+    if (methodTree == null) {
+      // probably a record's synthetic reader method
+      return false;
+    }
+
+    if (methodTree.getBody() == null) {
+      // Abstract or native method, can't be determined to be a getter
+      return false;
+    }
+
+    var statements = methodTree.getBody().getStatements();
+    if (statements.size() != 1) {
+      // More complex than a simple `return m_field` statement
+      return false;
+    }
+
+    var statement = statements.get(0);
+    if (!(statement instanceof ReturnTree ret)) {
+      // Shouldn't get here, since we've already filtered for methods that return a value
+      // and with a single statement - that one statement should be the return
+      return false;
+    }
+
+    var returnExpression = ret.getExpression();
+    return returnExpression.accept(
+        new SimpleTreeVisitor<Boolean, Void>(false) {
+          @Override
+          public Boolean visitIdentifier(IdentifierTree identifier, Void unused) {
+            return fieldsToLog.stream()
+                .anyMatch(v -> v.getSimpleName().contentEquals(identifier.getName()));
+          }
+        },
+        null);
+  }
 }
--- 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,6 +5,9 @@
 package edu.wpi.first.epilogue.processor;

 import edu.wpi.first.epilogue.Logged;
+import java.util.Arrays;
+import java.util.List;
+import java.util.stream.Collectors;
 import javax.lang.model.element.TypeElement;

 public final class StringUtils {
@@ -59,6 +62,33 @@ public final class StringUtils {
    return builder.toString();
  }

+  /**
+   * Splits a camel-cased string like "fooBar" into individual words like ["foo", "Bar"].
+   *
+   * @param camelCasedString the camel-cased string to split
+   * @return the individual words in the input
+   */
+  public static List<String> splitToWords(CharSequence camelCasedString) {
+    // Implementation from https://stackoverflow.com/a/2560017, refactored for readability
+
+    // Uppercase letter not followed by the first letter of the next word
+    // This allows for splitting "IOLayer" into "IO" and "Layer"
+    String penultimateUppercaseLetter = "(?<=[A-Z])(?=[A-Z][a-z])";
+
+    // Any character that's NOT an uppercase letter, immediately followed by an uppercase letter
+    // This allows for splitting "fooBar" into "foo" and "Bar", or "123Bang" into "123" and "Bang"
+    String lastNonUppercaseLetter = "(?<=[^A-Z])(?=[A-Z])";
+
+    // The final letter in a sequence, followed by a non-alpha character like a number or underscore
+    // This allows for splitting "foo123" into "foo" and "123"
+    String finalLetter = "(?<=[A-Za-z])(?=[^A-Za-z])";
+
+    String regex =
+        String.format("%s|%s|%s", penultimateUppercaseLetter, lastNonUppercaseLetter, finalLetter);
+
+    return Arrays.asList(camelCasedString.toString().split(regex));
+  }
+
  /**
   * Gets the name of the field used to hold a logger for data of the given type.
   *
@@ -107,4 +137,31 @@ public final class StringUtils {

    return loggerClassName;
  }
+
+  /**
+   * Converts a camelCase element name to separate words, removing common field and method name
+   * prefixes like "m_" and "get".
+   *
+   * @param elementName the camelcased element name
+   * @return the name split into separate words and sanitized
+   */
+  public static String toHumanName(String elementName) {
+    // Delete common field prefixes (k_name, m_name, s_name)
+    var sanitizedName = elementName.replaceFirst("^[msk]_", "");
+
+    // Drop leading "k" prefix from fields
+    // (though normally these should be static, and thus not logged)
+    if (sanitizedName.matches("^k[A-Z].*$")) {
+      sanitizedName = sanitizedName.substring(1);
+    }
+
+    // Drop leading "get" from accessor methods
+    if (sanitizedName.matches("^get[A-Z].*$")) {
+      sanitizedName = sanitizedName.substring(3);
+    }
+
+    return splitToWords(sanitizedName).stream()
+        .map(StringUtils::capitalize)
+        .collect(Collectors.joining(" "));
+  }
 }