-
Notifications
You must be signed in to change notification settings - Fork 665
[epilogue] Detailed trigger dependencies #8253
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
Open
Raptorly1
wants to merge
15
commits into
wpilibsuite:main
Choose a base branch
from
Raptorly1:feature/detailed-trigger-dependencies
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+1,125
−26
Open
Changes from all commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
7dfabed
Add @DependsOn annotation for dependency metadata
Raptorly1 f99b7d8
Update annotation processor to recognize @DependsOn
Raptorly1 d4f1099
Implement dependency logging in LoggerGenerator
Raptorly1 429eddf
Add test case for @DependsOn annotation processing
Raptorly1 2b13f61
Add LogMetadata class for dependency tracking
Raptorly1 e4ae20d
Extend EpilogueBackend with metadata-aware log methods
Raptorly1 815480f
Implement dependency metadata generation in LoggerGenerator
Raptorly1 98f0f6f
Update test expectations for dependency metadata feature
Raptorly1 eb28d17
Generate LogMetadata fields for elements with @DependsOn annotations …
Raptorly1 39e05ea
Refactor generateLogMetadata method to accept fields and methods list…
Raptorly1 37a2e52
Implement metadata-aware logging methods in EpilogueBackend and NTEpi…
Raptorly1 3ad142f
Add List import and update LogMetadata
Raptorly1 539a41d
Refactor LoggerGenerator to use static LogMetadata fields and update …
Raptorly1 79b132c
change FQN to import
Raptorly1 bcc1eeb
Update LoggerGenerator to validate @DependsOn dependencies and update…
Raptorly1 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,13 +11,15 @@ | |
| import com.sun.source.tree.ReturnTree; | ||
| import com.sun.source.util.SimpleTreeVisitor; | ||
| import com.sun.source.util.Trees; | ||
| import edu.wpi.first.epilogue.DependsOn; | ||
| 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.regex.Matcher; | ||
| import java.util.Comparator; | ||
| import java.util.Deque; | ||
| import java.util.EnumMap; | ||
|
|
@@ -26,6 +28,7 @@ | |
| import java.util.List; | ||
| import java.util.Map; | ||
| import java.util.Objects; | ||
| import java.util.Set; | ||
| import java.util.function.Predicate; | ||
| import java.util.stream.Collectors; | ||
| import java.util.stream.Stream; | ||
|
|
@@ -204,6 +207,15 @@ private void writeLoggerFile( | |
| .toList(); | ||
| boolean requiresVarHandles = !varHandleFields.isEmpty(); | ||
|
|
||
| // Check if any element has valid @DependsOn dependencies to determine if LogMetadata import is needed | ||
| boolean needsLogMetadataImport = | ||
| Stream.concat(loggableFields.stream(), loggableMethods.stream()) | ||
| .anyMatch( | ||
| element -> { | ||
| List<String> validDeps = collectLogDependencies(element, loggableFields, loggableMethods); | ||
| return validDeps != null && !validDeps.isEmpty(); | ||
| }); | ||
|
|
||
| try (var out = new PrintWriter(loggerFile.openWriter())) { | ||
| if (packageName != null) { | ||
| // package com.example; | ||
|
|
@@ -215,6 +227,10 @@ private void writeLoggerFile( | |
| out.println("import edu.wpi.first.epilogue.Epilogue;"); | ||
| out.println("import edu.wpi.first.epilogue.logging.ClassSpecificLogger;"); | ||
| out.println("import edu.wpi.first.epilogue.logging.EpilogueBackend;"); | ||
| if (needsLogMetadataImport) { | ||
| out.println("import edu.wpi.first.epilogue.logging.LogMetadata;"); | ||
| out.println("import java.util.List;"); | ||
| } | ||
| if (requiresVarHandles) { | ||
| out.println("import java.lang.invoke.MethodHandles;"); | ||
| out.println("import java.lang.invoke.VarHandle;"); | ||
|
|
@@ -241,12 +257,39 @@ private void writeLoggerFile( | |
| out.println(); | ||
| } | ||
|
|
||
| // Static initializer block to load VarHandles and reflection fields | ||
| // Generate instance LogMetadata fields for elements with @DependsOn annotations | ||
| // that have at least one valid dependency | ||
| List<Element> logMetadataElements = | ||
| Stream.concat(loggableFields.stream(), loggableMethods.stream()) | ||
| .filter( | ||
| element -> { | ||
| // Only include elements that have valid dependencies | ||
| List<String> validDeps = collectLogDependencies(element, loggableFields, loggableMethods); | ||
| return validDeps != null && !validDeps.isEmpty(); | ||
| }) | ||
| .sorted(Comparator.comparing(e -> e.getSimpleName().toString())) | ||
| .collect(Collectors.toList()); | ||
|
|
||
| if (!logMetadataElements.isEmpty()) { | ||
| for (var element : logMetadataElements) { | ||
| // Generate static LogMetadata field with inline initialization | ||
| String metadataInitialization = | ||
| generateLogMetadataConstruction(element, loggableFields, loggableMethods); | ||
| out.printf( | ||
| " // Cached LogMetadata for element %s with @DependsOn annotations%n", | ||
| element.getSimpleName()); | ||
| out.printf( | ||
| " private static final LogMetadata %s = %s;%n", | ||
| logMetadataFieldName(element), | ||
| metadataInitialization); | ||
| } | ||
| out.println(); | ||
| } | ||
|
|
||
| // Static initializer block to load VarHandles only | ||
| if (requiresVarHandles) { | ||
| out.println(" static {"); | ||
|
|
||
| out.println(" try {"); | ||
|
|
||
| out.println(" var rootLookup = MethodHandles.lookup();"); | ||
|
|
||
| // Group private fields by class, then generate a private lookup for each class | ||
|
|
@@ -344,7 +387,17 @@ private void writeLoggerFile( | |
| // unloggable commands. | ||
| var logInvocation = h.logInvocation(loggableElement, clazz); | ||
| if (logInvocation != null) { | ||
| out.println(logInvocation.indent(6).stripTrailing() + ";"); | ||
| // Generate log invocation for the main element | ||
| var metadata = | ||
| generateLogMetadataFieldReference(loggableElement, loggableFields, loggableMethods); | ||
| if (metadata != null) { | ||
| var modifiedInvocation = | ||
| logInvocation.replaceFirst( | ||
| "\\)$", Matcher.quoteReplacement(", " + metadata + ")")); | ||
| out.println(modifiedInvocation.indent(6).stripTrailing() + ";"); | ||
| } else { | ||
| out.println(logInvocation.indent(6).stripTrailing() + ";"); | ||
| } | ||
| } | ||
| }); | ||
| } | ||
|
|
@@ -369,6 +422,22 @@ public static String varHandleName(VariableElement field) { | |
| .formatted(field.getEnclosingElement().toString().replace(".", "_"), field.getSimpleName()); | ||
| } | ||
|
|
||
| /** | ||
| * Generates the name of a static final LogMetadata field for the given element. The field name is | ||
| * guaranteed to be unique, with disambiguation between fields and methods. | ||
| * | ||
| * @param element The element to generate a LogMetadata field for | ||
| * @return The name of the generated LogMetadata field | ||
| */ | ||
| public static String logMetadataFieldName(Element element) { | ||
| String suffix = element instanceof VariableElement ? "_field" : "_method"; | ||
| return "$metadata_%s_%s%s" | ||
| .formatted( | ||
| element.getEnclosingElement().toString().replace(".", "_"), | ||
| element.getSimpleName(), | ||
| suffix); | ||
| } | ||
|
|
||
| private void collectLoggables( | ||
| TypeElement clazz, List<VariableElement> fields, List<ExecutableElement> methods) { | ||
| var config = clazz.getAnnotation(Logged.class); | ||
|
|
@@ -474,4 +543,99 @@ public Boolean visitIdentifier(IdentifierTree identifier, Void unused) { | |
| }, | ||
| null); | ||
| } | ||
|
|
||
| /** | ||
| * Collects the valid dependency names for an element marked with @DependsOn annotations. | ||
| * | ||
| * @param element the element that has @DependsOn annotations | ||
| * @param fieldsToLog the list of fields that will be logged | ||
| * @param methodsToLog the list of methods that will be logged | ||
| * @return List of valid dependency names, or null if no valid dependencies | ||
| */ | ||
| private static List<String> collectLogDependencies( | ||
| Element element, List<VariableElement> fieldsToLog, List<ExecutableElement> methodsToLog) { | ||
| // Check for @DependsOn annotations (both single and multiple via @DependsOn.Container) | ||
| List<DependsOn> dependencies = new ArrayList<>(); | ||
|
|
||
| // Get single @DependsOn annotation | ||
| DependsOn singleDependency = element.getAnnotation(DependsOn.class); | ||
| if (singleDependency != null) { | ||
| dependencies.add(singleDependency); | ||
| } | ||
|
|
||
| // Get multiple @DependsOn annotations via container | ||
| DependsOn.Container containerDependency = element.getAnnotation(DependsOn.Container.class); | ||
| if (containerDependency != null) { | ||
| dependencies.addAll(List.of(containerDependency.value())); | ||
SamCarlberg marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| if (dependencies.isEmpty()) { | ||
| return null; | ||
| } | ||
|
|
||
| // Precompute set of logged names for efficient lookup | ||
| Set<String> loggedNames = | ||
| Stream.concat(fieldsToLog.stream(), methodsToLog.stream()) | ||
| .map(ElementHandler::loggedName) | ||
| .collect(Collectors.toSet()); | ||
|
|
||
| // Collect valid dependency names by checking against logged elements | ||
| List<String> validDependencyNames = new ArrayList<>(); | ||
| for (DependsOn dependency : dependencies) { | ||
| String dependencyName = dependency.value(); | ||
| if (loggedNames.contains(dependencyName)) { | ||
| validDependencyNames.add(dependencyName); | ||
| } | ||
|
Comment on lines
+586
to
+588
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is silently skipping dependency names that don't match any logged names the best behavior? Should we add tests for this? |
||
| } | ||
|
|
||
| if (validDependencyNames.isEmpty()) { | ||
| return null; | ||
| } | ||
|
|
||
| return validDependencyNames; | ||
| } | ||
|
|
||
| /** | ||
| * Generates LogMetadata construction code for static field initialization. | ||
| * | ||
| * @param element the element that has @DependsOn annotations | ||
| * @param fieldsToLog the list of fields that will be logged | ||
| * @param methodsToLog the list of methods that will be logged | ||
| * @return LogMetadata construction code, or null if no valid dependencies | ||
| */ | ||
| private static String generateLogMetadataConstruction( | ||
| Element element, List<VariableElement> fieldsToLog, List<ExecutableElement> methodsToLog) { | ||
| List<String> validDependencyNames = collectLogDependencies(element, fieldsToLog, methodsToLog); | ||
|
|
||
| if (validDependencyNames == null) { | ||
| return null; | ||
| } | ||
|
|
||
| // Create LogMetadata construction code for static field initialization | ||
| String dependencyList = | ||
| validDependencyNames.stream() | ||
| .map(name -> "\"" + name + "\"") | ||
| .collect(Collectors.joining(", ")); | ||
| return "new LogMetadata(List.of(" + dependencyList + "))"; | ||
| } | ||
|
|
||
| /** | ||
| * Generates a reference to the static LogMetadata field for an element. | ||
| * | ||
| * @param element the element that has @DependsOn annotations | ||
| * @param fieldsToLog the list of fields that will be logged | ||
| * @param methodsToLog the list of methods that will be logged | ||
| * @return LogMetadata field reference, or null if no valid dependencies | ||
| */ | ||
| private static String generateLogMetadataFieldReference( | ||
| Element element, List<VariableElement> fieldsToLog, List<ExecutableElement> methodsToLog) { | ||
| List<String> validDependencyNames = collectLogDependencies(element, fieldsToLog, methodsToLog); | ||
|
|
||
| if (validDependencyNames == null) { | ||
| return null; | ||
| } | ||
|
|
||
| // Return reference to static final field | ||
| return logMetadataFieldName(element); | ||
| } | ||
| } | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did anything change so that this no longer generates reflection fields (which were mentioned in the original version of the comment)?