Clean up synthetic accessor method rot (#2762)

* Clean up accessor method rotting.

* Make tests pass on Windows (10, JDK 8x64) (with less ignores than before)

* Fix error prone version

Author: TWiStErRob

.gitignore

@@ -23,5 +23,7 @@ docs/**/*
 
 # Intellij
 *.iml
+*.ipr
+*.iws
 .idea/**
 !.idea/codeStyleSettings.xml

build.gradle

@@ -15,7 +15,7 @@ buildscript {
     dependencies {
         classpath "com.android.tools.build:gradle:${ANDROID_GRADLE_VERSION}"
         if (!hasProperty('DISABLE_ERROR_PRONE')) {
-            classpath "net.ltgt.gradle:gradle-errorprone-plugin:${ERROR_PRONE_VERSION}"
+            classpath "net.ltgt.gradle:gradle-errorprone-plugin:${ERROR_PRONE_PLUGIN_VERSION}"
         }
         classpath "se.bjurr.violations:violations-gradle-plugin:${VIOLATIONS_PLUGIN_VERSION}"
     }

checkstyle.xml

@@ -73,7 +73,7 @@
 
         <!-- Allow common trailing comments used to describe suppressions -->
         <module name="TrailingComment">
-          <property name="legalComment" value="Public API" />
+          <property name="legalComment" value="^Public API.?$|^NOPMD .*$" />
         </module>
 
         <!-- Checks for imports. -->

gradle.properties

@@ -32,9 +32,10 @@ JSR_305_VERSION=3.0.2
 AUTO_SERVICE_VERSION=1.0-rc3
 JAVAPOET_VERSION=1.9.0
 
-PMD_VERSION=5.4.0
+PMD_VERSION=5.8.1
 FINDBUGS_VERSION=3.0.0
-ERROR_PRONE_VERSION=0.0.13
+ERROR_PRONE_VERSION=2.1.4-SNAPSHOT
+ERROR_PRONE_PLUGIN_VERSION=0.0.13
 VIOLATIONS_PLUGIN_VERSION=1.3
 
 COMPILE_SDK_VERSION=27

integration/gifencoder/src/test/java/com/bumptech/glide/integration/gifencoder/ReEncodingGifResourceEncoderTest.java

@@ -3,7 +3,6 @@
 import static com.google.common.truth.Truth.assertThat;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
-import static org.junit.Assume.assumeTrue;
 import static org.mockito.Matchers.any;
 import static org.mockito.Matchers.anyInt;
 import static org.mockito.Matchers.eq;
@@ -95,6 +94,8 @@ public void setUp() {
 
   @After
   public void tearDown() {
+    // GC before delete() to release files on Windows (https://stackoverflow.com/a/4213208/253468)
+    System.gc();
     if (file.exists() && !file.delete()) {
       throw new RuntimeException("Failed to delete file");
     }
@@ -120,8 +121,6 @@ public void testEncodeStrategy_withEncodeTransformationFalse_returnsSource() {
   @Test
   public void testEncode_withEncodeTransformationFalse_writesSourceDataToStream()
       throws IOException {
-    // Most likely an instance of http://stackoverflow.com/q/991489/253468
-    assumeTrue(!System.getProperty("os.name").startsWith("Windows"));
     options.set(ReEncodingGifResourceEncoder.ENCODE_TRANSFORMATION, false);
     String expected = "testString";
     byte[] data = expected.getBytes("UTF-8");
@@ -134,7 +133,6 @@ public void testEncode_withEncodeTransformationFalse_writesSourceDataToStream()
   @Test
   public void testEncode_WithEncodeTransformationFalse_whenOsThrows_returnsFalse()
       throws IOException {
-
     options.set(ReEncodingGifResourceEncoder.ENCODE_TRANSFORMATION, false);
     byte[] data = "testString".getBytes("UTF-8");
     when(gifDrawable.getBuffer()).thenReturn(ByteBuffer.wrap(data));
@@ -312,10 +310,7 @@ public void testRecyclesFrameResourceAfterWritingIfFrameResourceIsNotTransformed
   }
 
   @Test
-  public void testWritesBytesDirectlyToDiskIfTransformationIsUnitTransformation()
-      throws IOException {
-    // Most likely an instance of http://stackoverflow.com/q/991489/253468
-    assumeTrue(!System.getProperty("os.name").startsWith("Windows"));
+  public void testWritesBytesDirectlyToDiskIfTransformationIsUnitTransformation() {
     when(gifDrawable.getFrameTransformation()).thenReturn(UnitTransformation.<Bitmap>get());
     String expected = "expected";
     when(gifDrawable.getBuffer()).thenReturn(ByteBuffer.wrap(expected.getBytes()));

library/build.gradle

@@ -20,6 +20,10 @@ dependencies {
     testImplementation "org.robolectric:robolectric:${ROBOLECTRIC_VERSION}"
     testImplementation "com.squareup.okhttp3:mockwebserver:${MOCKWEBSERVER_VERSION}"
     testImplementation "com.android.support:support-v4:${ANDROID_SUPPORT_VERSION}"
+
+    if (project.plugins.hasPlugin('net.ltgt.errorprone')) {
+       errorprone "com.google.errorprone:error_prone_core:${ERROR_PRONE_VERSION}"
+    }
 }
 
 android.testOptions.unitTests.all { Test testTask ->

library/pmd-ruleset.xml

@@ -3,15 +3,17 @@
          xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
          xsi:schemaLocation="http://pmd.sourceforge.net/ruleset/2.0.0 http://pmd.sourceforge.net/ruleset_2_0_0.xsd">
 
-    <description>This ruleset was created from PMD.rul</description>
+    <description>Check for flaws in Glide's codebase.</description>
 
     <rule ref="rulesets/java/basic.xml">
         <exclude name="AvoidBranchingStatementAsLastInLoop"/>
     </rule>
     <rule ref="rulesets/java/braces.xml"/>
-    <rule ref="rulesets/java/strings.xml">
-        <!-- TODO: This warns about annotations, apparently fixed in a later version. -->
-        <exclude name="AvoidDuplicateLiterals"/>
+    <rule ref="rulesets/java/strings.xml"/>
+    <rule ref="rulesets/java/strings.xml/AvoidDuplicateLiterals">
+        <properties>
+            <property name="skipAnnotations" value="true" />
+        </properties>
     </rule>
     <rule ref="rulesets/java/unusedcode.xml"/>
 
@@ -28,31 +30,39 @@
         <exclude name="GodClass"/>
     </rule>
 
+    <rule ref="rulesets/java/design.xml/AccessorMethodGeneration"
+          message="Avoid autogenerated methods to access private fields and methods of inner / outer classes.
+                   Use @Synthetic to flag members made more visible than necessary to prevent accessors.">
+        <properties>
+            <!-- Ignore references to `private static final * * = <literal>`
+                 Suppress via XPath: current node (access that generates the accessor) is .
+                 Check if there exists a FieldDeclaration (private static final)
+                 which has a VariableInitializer with a Literal
+                 and the name (@Image) of the declaration is the same as the accessed member.
+                 TODO calculated constants are false positive https://github.com/pmd/pmd/issues/808
+            -->
+            <property name="violationSuppressXPath" value="
+            .[@Image =
+                //FieldDeclaration[@Private = 'true' and @Static='true' and @Final='true']
+                /VariableDeclarator[
+                    VariableInitializer/Expression/PrimaryExpression[not(PrimarySuffix)]/PrimaryPrefix/Literal
+                ]/VariableDeclaratorId/@Image
+             ]" />
+        </properties>
+    </rule>
+
     <rule ref="rulesets/java/empty.xml/EmptyCatchBlock" message="Commented blocks are ok">
         <properties>
             <property name="allowCommentedBlocks" value="true"/>
         </properties>
     </rule>
 
-
-    <!--Overrides default check to avoid violation when @Synthetic annotation is present-->
-    <rule ref="rulesets/java/design.xml/UncommentedEmptyConstructor"
-          message="Document empty constructor">
-
+    <!-- Configures check to avoid violation when @Synthetic annotation is present. -->
+    <rule ref="rulesets/java/design.xml/UncommentedEmptyConstructor">
         <properties>
-            <property name="xpath">
-                <value>
-                    <![CDATA[
-//ConstructorDeclaration[@Private='false'][count(BlockStatement) = 0 and
-($ignoreExplicitConstructorInvocation = 'true' or not(ExplicitConstructorInvocation)) and @containsComment = 'false'
- and not(../Annotation/MarkerAnnotation/Name[@Image='Synthetic'])]
- ]]>
-                </value>
-            </property>
-
+            <property name="violationSuppressXPath"
+                      value="../Annotation/MarkerAnnotation/Name[@Image='Synthetic']" />
         </properties>
-
     </rule>
 
-
 </ruleset>

library/src/main/java/com/bumptech/glide/RequestBuilder.java

@@ -30,6 +30,7 @@
 import com.bumptech.glide.request.target.ViewTarget;
 import com.bumptech.glide.signature.ApplicationVersionSignature;
 import com.bumptech.glide.util.Preconditions;
+import com.bumptech.glide.util.Synthetic;
 import com.bumptech.glide.util.Util;
 import java.io.File;
 import java.net.URL;
@@ -572,7 +573,7 @@ public <Y extends Target<TranscodeType>> Y into(@NonNull Y target) {
   }
 
   @NonNull
-  private <Y extends Target<TranscodeType>> Y into(
+  @Synthetic <Y extends Target<TranscodeType>> Y into(
       @NonNull Y target,
       @Nullable RequestListener<TranscodeType> targetListener) {
     return into(target, targetListener, getMutableOptions());

library/src/main/java/com/bumptech/glide/load/engine/ActiveResources.java

@@ -91,7 +91,8 @@ EngineResource<?> get(Key key) {
     return active;
   }
 
-  private void cleanupActiveReference(@NonNull ResourceWeakReference ref) {
+  @SuppressWarnings("WeakerAccess")
+  @Synthetic void cleanupActiveReference(@NonNull ResourceWeakReference ref) {
     Util.assertMainThread();
     activeEngineResources.remove(ref.key);
 
@@ -112,29 +113,33 @@ private ReferenceQueue<EngineResource<?>> getReferenceQueue() {
         @Override
         public void run() {
           Process.setThreadPriority(Process.THREAD_PRIORITY_BACKGROUND);
-          ResourceWeakReference ref;
-          while (!isShutdown) {
-            try {
-              ref = (ResourceWeakReference) resourceReferenceQueue.remove();
-              mainHandler.obtainMessage(MSG_CLEAN_REF, ref).sendToTarget();
-
-              // This section for testing only.
-              DequeuedResourceCallback current = cb;
-              if (current != null) {
-                current.onResourceDequeued();
-              }
-              // End for testing only.
-            } catch (InterruptedException e) {
-              Thread.currentThread().interrupt();
-            }
-          }
+          cleanReferenceQueue();
         }
       }, "glide-active-resources");
       cleanReferenceQueueThread.start();
     }
     return resourceReferenceQueue;
   }
 
+  @SuppressWarnings("WeakerAccess")
+  @Synthetic void cleanReferenceQueue() {
+    while (!isShutdown) {
+      try {
+        ResourceWeakReference ref = (ResourceWeakReference) resourceReferenceQueue.remove();
+        mainHandler.obtainMessage(MSG_CLEAN_REF, ref).sendToTarget();
+
+        // This section for testing only.
+        DequeuedResourceCallback current = cb;
+        if (current != null) {
+          current.onResourceDequeued();
+        }
+        // End for testing only.
+      } catch (InterruptedException e) {
+        Thread.currentThread().interrupt();
+      }
+    }
+  }
+
   @VisibleForTesting
   void setDequeuedResourceCallback(DequeuedResourceCallback cb) {
     this.cb = cb;