bumptech · sjudd · Dec 31, 2017 · Dec 25, 2017 · Dec 30, 2017 · Dec 26, 2017
diff --git a/gradle.properties b/gradle.properties
@@ -32,7 +32,7 @@ JSR_305_VERSION=3.0.2
 AUTO_SERVICE_VERSION=1.0-rc3
 JAVAPOET_VERSION=1.9.0
 
-PMD_VERSION=5.8.1
+PMD_VERSION=6.0.0
 FINDBUGS_VERSION=3.0.0
 ERROR_PRONE_VERSION=2.1.4-SNAPSHOT
 ERROR_PRONE_PLUGIN_VERSION=0.0.13

diff --git a/library/build.gradle b/library/build.gradle
@@ -22,7 +22,7 @@ dependencies {
     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}"
+        errorprone "com.google.errorprone:error_prone_core:${ERROR_PRONE_VERSION}"
     }
 }
 
@@ -74,13 +74,9 @@ afterEvaluate {
 
         classes = fileTree("${project.buildDir}/intermediates/classes/debug/")
         source android.sourceSets.main.java.srcDirs
-        classpath = project.configurations.compile
-        classpath += files(android.bootClasspath)
+        classpath = files()
         doFirst {
-            it.classpath +=
-                    files(project.android.libraryVariants.collect {
-                        it.javaCompile.classpath.files
-                    })
+            classpath += classPathForQuality()
         }
         effort = 'max'
         excludeFilter = file("findbugs-exclude.xml")
@@ -111,6 +107,15 @@ afterEvaluate {
         ruleSets = []
         ruleSetFiles = files('pmd-ruleset.xml')
         source android.sourceSets.main.java.srcDirs
+        classpath = files()
+        classpath += files("${project.buildDir}/intermediates/classes/debug/")
+        doFirst {
+            classpath += classPathForQuality()
+        }
+
+        //TODO enable this once new Gradle containing this flag is out
+        //see https://github.com/gradle/gradle/pull/3125#issuecomment-352442432
+        //incrementalAnalysis = true
 
         // Failures are caught and printed by the violations plugin.
         ignoreFailures = true
@@ -124,4 +129,12 @@ afterEvaluate {
     check.dependsOn('pmd')
 }
 
+def classPathForQuality() {
+    return files(
+            android.bootClasspath,
+            project.configurations.compile,
+            project.android.libraryVariants.collect { it.javaCompile.classpath }
+    )
+}
+
 apply from: "${rootProject.projectDir}/scripts/upload.gradle"
diff --git a/library/pmd-ruleset.xml b/library/pmd-ruleset.xml
@@ -5,64 +5,166 @@
 
     <description>Check for flaws in Glide's codebase.</description>
 
-    <rule ref="rulesets/java/basic.xml">
+    <rule ref="category/java/errorprone.xml">
         <exclude name="AvoidBranchingStatementAsLastInLoop"/>
+        <!-- Not using beans. -->
+        <exclude name="BeanMembersShouldSerialize" />
+        <!-- wat -->
+        <exclude name="AvoidFieldNameMatchingTypeName" />
+        <!-- This is identifying trivial cases that are clearly correct. -->
+        <exclude name="DataflowAnomalyAnalysis" />
+        <!-- Used regularly for object pooling. -->
+        <exclude name="NullAssignment" />
+        <!-- This can make the code easier to read and avoid duplicated logic in some cases. -->
+        <exclude name="AssignmentInOperand" />
+        <!-- I don't think this is confusing. -->
+        <exclude name="AvoidFieldNameMatchingMethodName" />
+        <!-- There are enough cases where this makes sense (typically related to logic around the number of items in a collection) that a blanket ban doesn't seem like a good idea. -->
+        <exclude name="AvoidLiteralsInIfCondition" />
+        <!-- It's clear that this is bad, but we have a number of cases where it makes sense and a blanket ban is irritating. -->
+        <exclude name="AvoidCatchingThrowable" />
     </rule>
-    <rule ref="rulesets/java/braces.xml"/>
-    <rule ref="rulesets/java/strings.xml"/>
-    <rule ref="rulesets/java/strings.xml/AvoidDuplicateLiterals">
-        <properties>
-            <property name="skipAnnotations" value="true" />
-        </properties>
+    <rule ref="category/java/errorprone.xml/AvoidDuplicateLiterals">
+      <properties>
+        <property name="skipAnnotations" value="true" />
+      </properties>
+    </rule>
+    <rule ref="category/java/codestyle.xml">
+      <!-- Abstract classes don't need to have Abstract in the name -->
+      <exclude name="AbstractNaming" />
+      <!-- Who cares? -->
+      <exclude name="AtLeastOneConstructor" />
+      <!-- Don't need to annotate package private methods. -->
+      <exclude name="DefaultPackage" />
+      <exclude name="CommentDefaultAccessModifier" />
+      <!-- Optionally implemented default empty methods are fine. -->
+      <exclude name="EmptyMethodInAbstractClassShouldBeAbstract" />
+      <!-- Why make generics less clear by using shorter names? -->
+      <exclude name="GenericsNaming" />
+      <!-- No need to enforce final if it's not necessary. -->
+      <exclude name="MethodArgumentCouldBeFinal" />
+      <exclude name="LocalVariableCouldBeFinal" />
+      <!-- This isn't always the easiest way to read a method. -->
+      <exclude name="OnlyOneReturn" />
+      <!-- Obfuscated code is best code? -->
+      <exclude name="LongVariable" />
+      <!-- This is not always true. -->
+      <exclude name="ShortClassName" />
+      <!-- A good idea but we have tons of violations. FIXME. -->
+      <exclude name="ShortMethodName" />
+      <exclude name="ShortVariable" />
+      <!-- We don't use in and out to mean modified or not modified by the method, it's useful to match framework methods. -->
+      <exclude name="AvoidPrefixingMethodParameters" />
+      <!-- No idea what this is supposed to accomplish. -->
+      <exclude name="AvoidFinalLocalVariable" />
+      <!-- These are often useful for clarity and explicitly suggested by Google's code style. -->
+      <exclude name="UselessParentheses" />
+      <!-- Theoretically this might be reasonable but the number of imports probably varies from class to class and this doesn't seem worth the overhead to maintain. -->
+      <exclude name="TooManyStaticImports" />
+      <!-- Lots of existing violations, not clear that the overhead is worthwhile though there are some cases where we definitely need to call super. FIXME. -->
+      <exclude name="CallSuperInConstructor" />
+      <!-- This is a reasonable idea, but in practice often the != null case is the expected case and it makes sense for it to come first. -->
+      <exclude name="ConfusingTernary" />
+    </rule>
+    <rule ref="category/java/performance.xml" >
+      <!-- Android may not behave the same as java VMs, using short can be clearer when working with binary data. -->
+      <exclude name="AvoidUsingShortType" />
+      <!-- The suggsted alternatives are not available until Glide's minsdk level is 26+ -->
+      <exclude name="AvoidFileStream" />
+    </rule>
+    <rule ref="category/java/bestpractices.xml" >
+      <!-- Catches any method, test or not, that has the name "tearDown". -->
+      <exclude name="JUnit4TestShouldUseAfterAnnotation" />
+      <!-- This is a good idea, but in practice it's often somewhat clearer than defining a temporary variable and we do it all over the place. -->
+      <exclude name="AvoidReassigningParameters" />
+      <!-- This ignores imports used by javadocs and is worse than the existing checkstyle check. -->
+      <exclude name="UnusedImports" />
+    </rule>
+    <rule ref="category/java/bestpractices.xml/OneDeclarationPerLine">
+      <properties>
+        <property name="strictMode" value="true" />
+        <!-- Allow `for (int i = 0, size = list.size(); i < size; i++) {`
+             Somewhat clearer to set size along with the index. -->
+        <property name="violationSuppressXPath"
+                  value="self::LocalVariableDeclaration
+                           [parent::ForInit]
+                           [Type/PrimitiveType[@Image = 'int']
+                             and VariableDeclarator/VariableDeclaratorId[@Image='i']
+                             and VariableDeclarator/VariableDeclaratorId[@Image='size']
+                           ]
+                        " />
+      </properties>
+    </rule>
+    <rule ref="category/java/bestpractices.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/unusedcode.xml"/>
 
-    <rule ref="rulesets/java/design.xml">
-        <exclude name="ConfusingTernary"/>
-        <exclude name="EmptyMethodInAbstractClassShouldBeAbstract"/>
-        <exclude name="AvoidSynchronizedAtMethodLevel"/>
+    <rule ref="category/java/design.xml">
+        <exclude name="GodClass" />
+        <!-- No idea how you reasonably define this. -->
+        <exclude name="ExcessiveImports" />
+        <exclude name="CouplingBetweenObjects" />
+        <exclude name="TooManyMethods" />
+        <exclude name="LawOfDemeter" />
+        <exclude name="NcssCount" />
+        <exclude name="ExcessiveParameterList" />
+        <exclude name="TooManyFields" />
+        <!-- We don't define any packages to use with this rule. -->
+        <exclude name="LoosePackageCoupling" />
+        <!-- Throwing other types of exceptions doesn't seem to add much to clarify. -->
+        <exclude name="AvoidThrowingRawExceptionTypes" />
+        <exclude name="AvoidThrowingNullPointerException" />
+        <!-- TODO: explore these further. -->
+        <exclude name="CyclomaticComplexity" />
+        <exclude name="NPathComplexity" />
+        <exclude name="ExcessiveMethodLength" />
+        <exclude name="ExcessiveClassLength" />
+        <exclude name="ExcessivePublicCount" />
+        <!-- This is redundant, also caught with AvoidCatchingNPEs. -->
+        <exclude name="AvoidCatchingGenericException" />
+    </rule>
 
+    <rule ref="category/java/multithreading.xml">
+        <exclude name="AvoidSynchronizedAtMethodLevel"/>
         <!-- This check breaks on double checked locking which is safe in Java 6/7 -->
         <exclude name="NonThreadSafeSingleton"/>
-
-        <!-- TODO: Fix these -->
-        <exclude name="AvoidReassigningParameters"/>
-        <exclude name="GodClass"/>
+        <!-- Used frequently in the singleton pattern. -->
+        <exclude name="AvoidUsingVolatile" />
+        <!-- No reason to do this by default. -->
+        <exclude name="UseConcurrentHashMap" />
+        <exclude name="DoNotUseThreads" />
     </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">
+    <rule ref="category/java/errorprone.xml/EmptyCatchBlock" message="Commented blocks are ok">
         <properties>
             <property name="allowCommentedBlocks" value="true"/>
         </properties>
     </rule>
 
-    <!-- Configures check to avoid violation when @Synthetic annotation is present. -->
-    <rule ref="rulesets/java/design.xml/UncommentedEmptyConstructor">
-        <properties>
-            <property name="violationSuppressXPath"
-                      value="../Annotation/MarkerAnnotation/Name[@Image='Synthetic']" />
-        </properties>
+
+  <!-- Configures check to avoid violation when @Synthetic annotation is present. -->
+    <rule ref="category/java/documentation.xml/UncommentedEmptyConstructor">
+      <properties>
+        <property name="violationSuppressXPath"
+          value="../Annotation/MarkerAnnotation/Name[@Image='Synthetic']" />
+      </properties>
     </rule>
 
 </ruleset>
diff --git a/library/src/main/java/com/bumptech/glide/Glide.java b/library/src/main/java/com/bumptech/glide/Glide.java
@@ -577,6 +577,7 @@ GlideContext getGlideContext() {
    * {@link com.bumptech.glide.load.engine.prefill.PreFillType.Builder Builders} representing
    * individual sizes and configurations of {@link android.graphics.Bitmap}s to be pre-filled.
    */
+  @SuppressWarnings("unused") // Public API
   public void preFillBitmapPool(@NonNull PreFillType.Builder... bitmapAttributeBuilders) {
     bitmapPreFiller.preFill(bitmapAttributeBuilders);
   }
@@ -644,6 +645,7 @@ public RequestManagerRetriever getRequestManagerRetriever() {
    *
    * @return the previous MemoryCategory used by Glide.
    */
+  @SuppressWarnings("WeakerAccess") // Public API
   @NonNull
   public MemoryCategory setMemoryCategory(@NonNull MemoryCategory memoryCategory) {
     // Engine asserts this anyway when removing resources, fail faster and consistently

diff --git a/library/src/main/java/com/bumptech/glide/ListPreloader.java b/library/src/main/java/com/bumptech/glide/ListPreloader.java
@@ -224,6 +224,8 @@ private void cancelAll() {
   private static final class PreloadTargetQueue {
     private final Queue<PreloadTarget> queue;
 
+    // The loop is short and the only point is to create the objects.
+    @SuppressWarnings("PMD.AvoidInstantiatingObjectsInLoops")
     PreloadTargetQueue(int size) {
       queue = Util.createQueue(size);
 

diff --git a/library/src/main/java/com/bumptech/glide/Registry.java b/library/src/main/java/com/bumptech/glide/Registry.java
@@ -511,8 +511,11 @@ private <Data, TResource, Transcode> List<DecodePath<Data, TResource, Transcode>
             decoderRegistry.getDecoders(dataClass, registeredResourceClass);
         ResourceTranscoder<TResource, Transcode> transcoder =
             transcoderRegistry.get(registeredResourceClass, registeredTranscodeClass);
-        decodePaths.add(new DecodePath<>(dataClass, registeredResourceClass,
-            registeredTranscodeClass, decoders, transcoder, throwableListPool));
+        @SuppressWarnings("PMD.AvoidInstantiatingObjectsInLoops")
+        DecodePath<Data, TResource, Transcode> path =
+            new DecodePath<>(dataClass, registeredResourceClass, registeredTranscodeClass,
+                decoders, transcoder, throwableListPool);
+        decodePaths.add(path);
       }
     }
     return decodePaths;

diff --git a/library/src/main/java/com/bumptech/glide/RequestBuilder.java b/library/src/main/java/com/bumptech/glide/RequestBuilder.java
@@ -546,7 +546,11 @@ public RequestBuilder<TranscodeType> load(@Nullable byte[] model) {
    * arguments, the current model is not copied copied so changes to the model will affect both
    * builders. </p>
    */
-  @SuppressWarnings("unchecked")
+  @SuppressWarnings({
+      "unchecked",
+      // we don't want to throw to be user friendly
+      "PMD.CloneThrowsCloneNotSupportedException"
+  })
   @CheckResult
   @Override
   public RequestBuilder<TranscodeType> clone() {

diff --git a/library/src/main/java/com/bumptech/glide/TransitionOptions.java b/library/src/main/java/com/bumptech/glide/TransitionOptions.java
@@ -71,9 +71,16 @@ public final CHILD transition(
     return self();
   }
 
-  @SuppressWarnings("unchecked")
+  @SuppressWarnings({
+      // cast to CHILD is safe given the generic argument represents the object's runtime class
+      "unchecked",
+      // CHILD is the correct class name.
+      "PMD.CloneMethodReturnTypeMustMatchClassName",
+      // we don't want to throw to be user friendly
+      "PMD.CloneThrowsCloneNotSupportedException"
+  })
   @Override
-  protected final CHILD clone() {
+  public final CHILD clone() {
     try {
       return (CHILD) super.clone();
     } catch (CloneNotSupportedException e) {

diff --git a/library/src/main/java/com/bumptech/glide/load/MultiTransformation.java b/library/src/main/java/com/bumptech/glide/load/MultiTransformation.java
@@ -18,7 +18,7 @@ public class MultiTransformation<T> implements Transformation<T> {
   @SafeVarargs
   @SuppressWarnings("varargs")
   public MultiTransformation(Transformation<T>... transformations) {
-    if (transformations.length < 1) {
+    if (transformations.length == 0) {
       throw new IllegalArgumentException(
           "MultiTransformation must contain at least one Transformation");
     }