Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improved opt-in logging support #7437

Merged
Merged
Show file tree
Hide file tree
Changes from 24 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
dc0d3d8
Testing opt-in logging without @Logged
Daniel1464 Nov 25, 2024
a14906f
Fixed problems...hopefully
Daniel1464 Nov 25, 2024
a62ee22
Fixed config logic
Daniel1464 Nov 25, 2024
e775a09
More attempted fixes
Daniel1464 Nov 26, 2024
15b5b98
Revert back to old isLoggable behavior
Daniel1464 Nov 26, 2024
844d498
Pretty much working now
Daniel1464 Nov 27, 2024
0bc9627
Reverted problematic changes
Daniel1464 Nov 27, 2024
ab66b81
Formatting fixes
github-actions[bot] Nov 27, 2024
2ad4fa7
Revert "Formatting fixes"
SamCarlberg Nov 28, 2024
53f47a2
Revert "Reverted problematic changes"
SamCarlberg Nov 28, 2024
2253477
Revert "Pretty much working now"
SamCarlberg Nov 28, 2024
b7d58a9
Broaden criteria for a loggable type
SamCarlberg Nov 28, 2024
2642d4a
Formatting fixes
github-actions[bot] Nov 28, 2024
ce6ffa6
Updated opt-in logging test
Daniel1464 Nov 28, 2024
49dcf06
Formatting fixes
github-actions[bot] Nov 28, 2024
b68cbb5
Java format
Daniel1464 Nov 28, 2024
bd71d20
Merge branch 'main' into opt-in-logging-without-logged-anno
Daniel1464 Nov 29, 2024
92cc005
Fixed inner class behavior
Daniel1464 Nov 29, 2024
a655354
Made logger order alphabetical,
Daniel1464 Nov 29, 2024
7a2a16a
Formatting fixes
github-actions[bot] Nov 29, 2024
ee5449e
Remove star-projected import
Daniel1464 Nov 30, 2024
99d77a4
Java format(again)
Daniel1464 Nov 30, 2024
bd2f01d
Merge branch 'wpilibsuite:main' into opt-in-logging-without-logged-anno
Daniel1464 Nov 30, 2024
92a760f
Renamed last uses of "dataLogger" with "backend"
Daniel1464 Dec 1, 2024
f28ede6
Separated opt-in field and method tests
Daniel1464 Dec 1, 2024
5412337
Java format
Daniel1464 Dec 1, 2024
8cbeee3
Added shouldNotLog test
Daniel1464 Dec 2, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,12 @@
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;
Expand Down Expand Up @@ -90,15 +93,15 @@ public boolean process(Set<? extends TypeElement> annotations, RoundEnvironment
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),
Expand All @@ -118,12 +121,39 @@ public boolean process(Set<? extends TypeElement> annotations, RoundEnvironment
.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<TypeElement> 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<? extends Element> annotatedElements) {
var fields =
annotatedElements.stream()
Expand Down Expand Up @@ -340,7 +370,8 @@ private Map<DeclaredType, DeclaredType> processCustomLoggers(
return customLoggers;
}

private void processEpilogue(RoundEnvironment roundEnv, TypeElement epilogueAnnotation) {
private void processEpilogue(
RoundEnvironment roundEnv, TypeElement epilogueAnnotation, Set<TypeElement> loggedTypes) {
var annotatedElements = roundEnv.getElementsAnnotatedWith(epilogueAnnotation);

List<String> loggerClassNames = new ArrayList<>();
Expand All @@ -358,12 +389,7 @@ private void processEpilogue(RoundEnvironment roundEnv, TypeElement epilogueAnno
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);
Expand Down Expand Up @@ -391,7 +417,7 @@ private void processEpilogue(RoundEnvironment roundEnv, TypeElement epilogueAnno

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;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
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;

Expand All @@ -35,10 +34,8 @@ protected LoggableHandler(

@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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
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;
Expand Down Expand Up @@ -42,6 +43,33 @@ public class LoggerGenerator {
LoggerGenerator::isBuiltInJavaMethod;
private final ProcessingEnvironment m_processingEnv;
private final List<ElementHandler> m_handlers;
private final Logged m_defaultConfig =
new Logged() {
@Override
public Class<? extends Annotation> 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<ElementHandler> handlers) {
this.m_processingEnv = processingEnv;
Expand Down Expand Up @@ -76,6 +104,9 @@ private static boolean isBuiltInJavaMethod(ExecutableElement e) {
*/
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<Element> notSkipped = LoggerGenerator::isNotSkipped;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ public static String loggerClassName(TypeElement clazz) {
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(" ", "");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,50 @@ public void update(EpilogueBackend backend, Example object) {
assertLoggerGenerates(source, expectedGeneratedSource);
}

@Test
void implicitOptInLogging() {
String source =
"""
package edu.wpi.first.epilogue;

class Example {
@Logged double x;
@Logged int y;

@Logged public double getValue() { return 2.0; }
@Logged public boolean isActive() { return false; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These would be better in a separate test, so we can check that just a logged field is enough to make a class logged, and that just a logged method is also enough. Having both in one test means it's ambiguous, and also means that if either field or method detection breaks (but not both), then it wouldn't be caught by the test cases.

@Test
void implicitLoggedFields() {
  String source =
    """
    package edu.wpi.first.epilogue;

    class Example {
      @Logged double x;
    }
    """;

  // ...
}

@Test
void implicitLoggedMethods() {
  String source =
    """
    package edu.wpi.first.epilogue;

    class Example {
      @Logged
      public double x() { return 42; }
    }
    """;

  // ...
}

Additionally, it'd be good to have another test to cover the null case of a class with no @Logged annotation and with no implicitly logged members to make sure we don't generate loggers for things that users didn't opt into

}
""";

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<Example> {
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);
backend.log("getValue", object.getValue());
backend.log("isActive", object.isActive());
}
}
}
""";

assertLoggerGenerates(source, expectedGeneratedSource);
}

@Test
void multiple() {
String source =
Expand Down Expand Up @@ -1416,6 +1460,47 @@ public void update(EpilogueBackend backend, Example object) {
assertLoggerGenerates(source, expectedRootLogger);
}

@Test
void nestedImplicit() {
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<Example> {
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 =
Expand Down
Loading