Skip to content

Commit

Permalink
PMD 6.0.0 (#2771)
Browse files Browse the repository at this point in the history 
* Update to PMD 6.0.0

* Revert: Fix deprecation warnings in PMD 6.0.0.
Move @SuppressWarnings closer to the problem to avoid suppressing future similar issues.
Reduce amount of C-style (declare first and then use) variable definitions
Make some variables final in tricky methods (enabled by above).
Remove some unnecessary PMD suppressions
Move PMD.OneDeclarationPerLine suppression for for loops into pmd config
Small method extractions towards Clean Code
Minor formatting/interface-impl enhancements in touched classes

* Fix classpath of PMD so UnnecessaryModifierRule doesn't crash (pmd/pmd#817)
Fix hidden PMD violations

* Fix false positives by providing the correct classpath to PMD

* Simplify ThumbnailStreamOpener.open

* Add TODO to make PMD build faster
  • Loading branch information
@TWiStErRob @sjudd
TWiStErRob authored and sjudd committed Dec 31, 2017
1 parent fae9444 commit f16aef4
Show file tree
Hide file tree
Showing 43 changed files with 487 additions and 245 deletions.
2 changes: 1 addition & 1 deletion gradle.properties
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
27 changes: 20 additions & 7 deletions library/build.gradle
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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}"
}
}

Expand Down Expand Up @@ -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")
Expand Down Expand Up @@ -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
Expand All @@ -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"
190 changes: 146 additions & 44 deletions library/pmd-ruleset.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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>
2 changes: 2 additions & 0 deletions library/src/main/java/com/bumptech/glide/Glide.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down Expand Up @@ -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
Expand Down
2 changes: 2 additions & 0 deletions library/src/main/java/com/bumptech/glide/ListPreloader.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
7 changes: 5 additions & 2 deletions library/src/main/java/com/bumptech/glide/Registry.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
6 changes: 5 additions & 1 deletion library/src/main/java/com/bumptech/glide/RequestBuilder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
11 changes: 9 additions & 2 deletions library/src/main/java/com/bumptech/glide/TransitionOptions.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
2 changes: 1 addition & 1 deletion library/src/main/java/com/bumptech/glide/load/MultiTransformation.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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");
}
Expand Down
Loading

0 comments on commit f16aef4

Please sign in to comment.