search

square / leakcanary

A memory leak detection library for Android.
https://square.github.io/leakcanary
Apache License 2.0
29.34k stars 3.97k forks source link

Enable detekt #1396

Closed pyricau closed 5 years ago

pyricau commented 5 years ago

See https://github.com/square/workflow/pull/2

Armaxis commented 5 years ago

How to proceed about existing violations of the rules? Some of the checks might be fine-tuned (e.g. Max Line Length, Complexity, etc), others might be disabled... Another solution would be creating Baseline files to ensure that no new code violates the rules. Or do we want to just fix most of the things as bulk in the same PR? We can start a discussion on the rules here, I can prepare a list of more or less controversial rule violations

Meanwhile, here's Detekt report just for the leakcanary-android-core: leakcanary-detekt-core.txt

A small excerpt from it:

Ruleset: comments Ruleset: complexity - 7h 40min debt LongMethod - 69/60 - [retrieveGroup] at C:\Users\armax\Documents\leakcanary\leakcanary\leakcanary-android-core\src\main\java\leakcanary\internal\activity\db\LeakingInstanceTable.kt:182:3 ComplexMethod - 12/10 - [retrieveGroup] at C:\Users\armax\Documents\leakcanary\leakcanary\leakcanary-android-core\src\main\java\leakcanary\internal\activity\db\LeakingInstanceTable.kt:182:3 NestedBlockDepth - 6/4 - [retrieveGroup] at C:\Users\armax\Documents\leakcanary\leakcanary\leakcanary-android-core\src\main\java\leakcanary\internal\activity\db\LeakingInstanceTable.kt:182:3 NestedBlockDepth - 5/4 - [importHprof] at C:\Users\armax\Documents\leakcanary\leakcanary\leakcanary-android-core\src\main\java\leakcanary\internal\activity\LeakActivity.kt:62:3 LongMethod - 85/60 - [onSuccessRetrieved] at C:\Users\armax\Documents\leakcanary\leakcanary\leakcanary-android-core\src\main\java\leakcanary\internal\activity\screen\HeapAnalysisSuccessScreen.kt:46:3 LongMethod - 201/60 - [render] at C:\Users\armax\Documents\leakcanary\leakcanary\leakcanary-android-core\src\main\java\leakcanary\internal\activity\screen\HeapDumpRenderer.kt:49:3 ComplexMethod - 21/10 - [render] at C:\Users\armax\Documents\leakcanary\leakcanary\leakcanary-android-core\src\main\java\leakcanary\internal\activity\screen\HeapDumpRenderer.kt:49:3 ..... Ruleset: empty-blocks - 20min debt EmptyFunctionBlock - [onViewAttachedToWindow] at C:\Users\armax\Documents\leakcanary\leakcanary\leakcanary-android-core\src\main\java\leakcanary\internal\activity\db\Io.kt:38:53 EmptyFunctionBlock - [onViewAttachedToWindow] at C:\Users\armax\Documents\leakcanary\leakcanary\leakcanary-android-core\src\main\java\leakcanary\internal\activity\db\Io.kt:38:53 EmptyFunctionBlock - [onViewAttachedToWindow] at C:\Users\armax\Documents\leakcanary\leakcanary\leakcanary-android-core\src\main\java\leakcanary\internal\activity\screen\HprofExplorerScreen.kt:57:57 EmptyFunctionBlock - [onViewAttachedToWindow] at C:\Users\armax\Documents\leakcanary\leakcanary\leakcanary-android-core\src\main\java\leakcanary\internal\activity\screen\HprofExplorerScreen.kt:57:57 Ruleset: exceptions - 1h debt TooGenericExceptionCaught - [e] at C:\Users\armax\Documents\leakcanary\leakcanary\leakcanary-android-core\src\main\java\leakcanary\internal\activity\db\Cursors.kt:14:12 TooGenericExceptionCaught - [e] at C:\Users\armax\Documents\leakcanary\leakcanary\leakcanary-android-core\src\main\java\leakcanary\internal\AndroidHeapDumper.kt:95:14 TooGenericExceptionThrown - [wait] at C:\Users\armax\Documents\leakcanary\leakcanary\leakcanary-android-core\src\main\java\leakcanary\internal\FutureResult.kt:34:7 Ruleset: performance - 40min debt SpreadOperator - [listFiles] at C:\Users\armax\Documents\leakcanary\leakcanary\leakcanary-android-core\src\main\java\leakcanary\internal\LeakDirectoryProvider.kt:59:33 SpreadOperator - [listFiles] at C:\Users\armax\Documents\leakcanary\leakcanary\leakcanary-android-core\src\main\java\leakcanary\internal\LeakDirectoryProvider.kt:64:33 Ruleset: style - 9h debt NewLineAtEndOfFile - [DefaultOnHeapAnalyzedListener.kt] at C:\Users\armax\Documents\leakcanary\leakcanary\leakcanary-android-core\src\main\java\leakcanary\DefaultOnHeapAnalyzedListener.kt:80:1 NewLineAtEndOfFile - [Cursors.kt] at C:\Users\armax\Documents\leakcanary\leakcanary\leakcanary-android-core\src\main\java\leakcanary\internal\activity\db\Cursors.kt:37:1 NewLineAtEndOfFile - [Db.kt] at C:\Users\armax\Documents\leakcanary\leakcanary\leakcanary-android-core\src\main\java\leakcanary\internal\activity\db\Db.kt:53:1 NewLineAtEndOfFile - [HeapAnalysisTable.kt] at C:\Users\armax\Documents\leakcanary\leakcanary\leakcanary-android-core\src\main\java\leakcanary\internal\activity\db\HeapAnalysisTable.kt:160:1 MagicNumber - [summary] at C:\Users\armax\Documents\leakcanary\leakcanary\leakcanary-android-core\src\main\java\leakcanary\internal\activity\db\HeapAnalysisTable.kt:110:53 NewLineAtEndOfFile - [Io.kt] at C:\Users\armax\Documents\leakcanary\leakcanary\leakcanary-android-core\src\main\java\leakcanary\internal\activity\db\Io.kt:80:1 NewLineAtEndOfFile - [LeakingInstanceTable.kt] at C:\Users\armax\Documents\leakcanary\leakcanary\leakcanary-android-core\src\main\java\leakcanary\internal\activity\db\LeakingInstanceTable.kt:286:1 ...

Armaxis commented 5 years ago

Compiled a table with all the rules that are currently violated - and it's not as bad as it seems from the detekt report! Full report here: leakcanary-full.txt

Rule Name Violations Threshold Average / Max
ComplexCondition 1 - PathFinder.kt
LargeClass 1 - HprofReader.kt
LongMethod 12 60 lines ~85 / 348 (HprofReader)
ComplexMethod 30 10 18 / 84 (HprofReader)
NestedBlockDepth 13 4 5 / 7 (AndroidObjectInspectors)
TooManyFunctions 11 11 14 / 39 (HprofReader)
LongParameterList 3 6 6 / 6
EmptyFunctionBlock 8 - -
TooGenericExceptionCaught 6 - -
TooGenericExceptionThrown 9 - -
SpreadOperator 9 - -
NewLineAtEndOfFile 67 - -
MagicNumber 167 - -
MaxLineLength 21 - -
ReturnCount 22 - -
SafeCast 1 - Serializables.kt
ExplicitGarbageCollectionCall 2 - Lol
pyricau commented 5 years ago

Thanks for doing this!

I think it makes sense to start with a baseline, and over time tweak some of the rules / clean up existing violations.

Armaxis commented 5 years ago

Sounds good! I'll prepare a pull request today/tomorrow.

What to do about Checkstyle? Everything is on Kotlin now, so Checkstyle is no longer doing anything and Detekt comes with KtLint for style checks. Remove Checkstyle?

I also need to make sure it integrates with Travis properly.

pyricau commented 5 years ago

I'm fine with removing checkstyle :)

Armaxis commented 5 years ago

Created a PR, still need to make sure Travis picked up Detekt properly

Armaxis commented 5 years ago

Hm, Travis' build failed, can't compile the code:

Task :leakcanary-android-core:compileReleaseKotlin FAILED e: /home/travis/build/square/leakcanary/leakcanary-android-core/src/main/java/leakcanary/internal/activity/LeakViews.kt: (53, 5): Val cannot be reassigned

I observed this issue locally too, it somehow can't recognize the method setPrimaryClip from the ClipboardManager class and thinks that there's no setter. Changing compileSdk to 28 fixed the issue for me locally, but I wonder why none of the other builds are failing on CI?

pyricau commented 5 years ago

I had noticed this error in the IDE but the app builds and deploys fine