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

Fix build-time Refaster template loading #121

Merged
merged 1 commit into from
Jun 30, 2022

Conversation

Stephan202
Copy link
Member

@Stephan202 Stephan202 commented Jun 6, 2022

Suggested commit message:

Fix build-time Refaster template loading (#121)

When using the system classloader, `RefasterCheckTest` is able to
successfully load the Refaster templates from the classpath, causing the
tests to pass. However, when during a build the Java compiler loads
`RefasterCheck`, the templates on the annotation processor classpath are
_not_ exposed through the system classloader.

To see the issue for yourself, apply the following patch and observe how before this change the redundant negations aren't flagged:

diff --git a/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/RefasterRuleCompilerTaskListener.java b/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/RefasterRuleCompilerTaskListener.java
index 7bc2d3ef..0947a468 100644
--- a/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/RefasterRuleCompilerTaskListener.java
+++ b/refaster-compiler/src/main/java/tech/picnic/errorprone/refaster/plugin/RefasterRuleCompilerTaskListener.java
@@ -54,7 +54,7 @@ final class RefasterRuleCompilerTaskListener implements TaskListener {
     }
 
     ClassTree tree = JavacTrees.instance(context).getTree(taskEvent.getTypeElement());
-    if (tree == null || !containsRefasterTemplates(tree)) {
+    if (tree == null || !!!containsRefasterTemplates(tree)) {
       return;
     }
 

@Stephan202 Stephan202 added this to the 0.1.0 milestone Jun 6, 2022
@Stephan202 Stephan202 requested review from Badbond and rickie June 6, 2022 10:18
@@ -195,7 +195,7 @@ private static ImmutableListMultimap<String, CodeTransformer> loadAllCodeTransfo

private static ImmutableSet<ResourceInfo> getClassPathResources() {
try {
return ClassPath.from(ClassLoader.getSystemClassLoader()).getResources();
return ClassPath.from(RefasterCheck.class.getClassLoader()).getResources();
Copy link
Member Author

Choose a reason for hiding this comment

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

This also impacts #43, and that PR doesn't actually apply the templates at build time for a second reason; will also push a commit to that branch.

Copy link
Member

@rickie rickie left a comment

Choose a reason for hiding this comment

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

Code changes LGTM.

I have one question as I fail to understand why. Why does this (not) happen?

However, when during a build the Java compiler loads
RefasterCheck, the templates on the annotation processor classpath are
not exposed through the system classloader.

When using the system classloader, `RefasterCheckTest` is able to
successfully load the Refaster templates from the classpath, causing the
tests to pass. However, when during a build the Java compiler loads
`RefasterCheck`, the templates on the annotation processor classpath are
_not_ exposed through the system classloader.
@rickie rickie force-pushed the sschroevers/fix-RefasterCheck branch from c6daede to 81f6180 Compare June 17, 2022 17:41
@Stephan202
Copy link
Member Author

I have one question as I fail to understand why. Why does this (not) happen?

However, when during a build the Java compiler loads
RefasterCheck, the templates on the annotation processor classpath are
not exposed through the system classloader.

There are short and long, simple and complex answers to this question, but the key point (IIUC) is that the system classloader loads only classes and resources on the "regular" classpath of the Java process. I think (but did not check) that this works at test time because Surefire creates new JVM processes with a classpath that matches the Maven test classpath. On the other hand, the annotation processor classpath is a special construct understood (and defined) only by the Maven Compiler Plugin, implemented using a custom classloader implementation; the system classloader is oblivious to that logic.

@Stephan202 Stephan202 merged commit bf5199e into master Jun 30, 2022
@Stephan202 Stephan202 deleted the sschroevers/fix-RefasterCheck branch June 30, 2022 11:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants