Skip to content

Conversation

Ornamus
Copy link
Contributor

@Ornamus Ornamus commented May 31, 2025

A sort of patch/continuation to #7993. @SamCarlberg and I overlooked that VarHandler does not allow you to violate some Java access permissions, such as accessing a private variable of a superclass or accessing a protected or package-private variable of a superclass if you're not in that package. As a result, some of the superclass-logging features of #7993 didn't work as intended:

  • A java.lang.IllegalAccessException would be raised on startup if a logged class' superclass had a @Logged private field
  • A java.lang.IllegalAccessException would be raised on startup if a logged class' superclass had a @Logged protected or package-private field and the child class wasn't in the same package

This PR fixes this by switching to using reflection to handle non-public superclass fields. This lets me revert the change from 7993 that made all non-public fields get accessed with VarHandler's - this can go back to being used for private-non-superclass fields only like it was before, which simplifies a lot of loggers and cleans up a lot of the unit test outputs :)

I pulled this set of changes into the robot project that helped me catch the issues and all seems to be working well - cross-package superclass fields are logging as intended.

@Ornamus Ornamus requested a review from a team as a code owner May 31, 2025 17:11
@github-actions github-actions bot added the component: epilogue Annotation-based logging library label May 31, 2025
@Ornamus Ornamus changed the title [epilogue] Use reflection to access superclass private & package-private fields [epilogue] Use reflection to access non-public superclass fields May 31, 2025
Comment on lines 262 to 264
" var lookup = MethodHandles.privateLookupIn("
+ classReference
+ ", \""
+ fieldName
+ "\", "
+ m_processingEnv.getTypeUtils().erasure(varHandleField.asType())
+ ".class);");
+ ", MethodHandles.lookup());");
Copy link
Member

Choose a reason for hiding this comment

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

We can use a separate private lookup for each type and generate something like this:

// given
@Logged class Superclass {
  private double superclassVar;
}

@Logged class Subclass extends Superclass {
  private int subclassVar;
}

// generate
var rootLookup = MethodHandles.lookup();
var subclassLookup = MethodHandles.privateLookupIn(Subclass.class, rootLookup);
var superclassLookup = MethodHandles.privateLookupIn(Superclass.class, rootLookup);

$subclassVar = subclassLookup.findVarHandle(Subclass.class, "subclassVar", int.class);
$superclassVar = superclassLookup.findVarHandle(Superclass.class, "superclassVar", double.class);

Then there's no need for runtime reflection

Fields from superclasses /could/ be read directly if they're declared public, or protected or package-private in the same package as the logged class. However, checking package equality is fragile so we only emit direct field accesses for public fields
In case a superclass has a private field with the same name as the logged class, we want to avoid generating the same name for both varhandles
@PeterJohnson PeterJohnson merged commit 45db0fd into wpilibsuite:main Aug 31, 2025
44 checks passed
wesleymg pushed a commit to wesleymg/allwpilib that referenced this pull request Sep 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: epilogue Annotation-based logging library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants