Add LeakCanary exclusions to default configuration

[pyricau]

LeakCanary does not depend on the support library but contains code that only executes if the support library is in the classpath, as well as code to analyze heap dumps that were produced from an app running with the support library. This plugin reports LeakCanary as an offender (https://github.com/square/leakcanary/issues/2020). Since it's a widely used library, I'm afraid this is going to come up repeatedly so wondering if LeakCanary could be added to the default config, e.g.

byeByeJetifier {
    legacyGroupIdPrefixes = ["android.arch", "com.android.support"]
    excludedConfigurations = ["lintClassPath"]
    excludedFilesFromScanning = [
        // com.squareup.leakcanary:leakcanary-object-watcher-android-support-fragments:2.5
        "leakcanary/internal/AndroidSupportFragmentDestroyWatcher$fragmentLifecycleCallbacks$1",
        "leakcanary/internal/AndroidSupportFragmentDestroyWatcher",

        // com.squareup.leakcanary:shark-android:2.5
        "shark/AndroidReferenceMatchers$ACTIVITY_CHOOSE_MODEL",

        // org.jetbrains.kotlin:kotlin-compiler-embeddable:1.4.20
        "org/jetbrains/kotlin/load/java/JvmAnnotationNamesKt",

        // org.jetbrains.kotlin:kotlin-reflect:1.4.20
        "kotlin/reflect/jvm/internal/impl/load/java/JvmAnnotationNamesKt",

        // org.jetbrains.kotlin:kotlin-android-extensions:1.4.20
        "org/jetbrains/kotlin/android/synthetic/AndroidConst",
        "org/jetbrains/kotlin/android/synthetic/codegen/AndroidIrTransformer",
        "org/jetbrains/kotlin/android/synthetic/codegen/ResourcePropertyStackValue",

        // org.jetbrains.kotlin:kotlin-compiler-embeddable:1.4.10
        "org/jetbrains/kotlin/com/intellij/codeInsight/NullableNotNullManager"
    ]
    excludeSupportAnnotations = true
    verbose = false
}

I was also wondering if it's possible to exclude an entire artifact group (i.e. com.squareup.leakcanary) instead of specific class names to simplify this config a bit, but it looks like not?

[maxirosson]

Sure. I am going to exclude Leak Canary by default. I could exclude all the packages starting with leakcanary or shark

Thanks for reporting this.

[Dimezis]

Could you please also add this one?

Scanning com.squareup.curtains:curtains:1.0.1
 Absoulute path: ...\modules-2\files-2.1\com.squareup.curtains\curtains\1.0.1\7105d2a1cb4c94b9c86149f922a9f9290304a06d\curtains-1.0.1.aar
 Graphs to this dependency:
 +---com.squareup.leakcanary:leakcanary-android:2.7
     +---com.squareup.leakcanary:leakcanary-android-core:2.7
          +---com.squareup.leakcanary:leakcanary-object-watcher-android:2.7
               +---com.squareup.curtains:curtains:1.0.1
 +---com.squareup.leakcanary:leakcanary-android:2.7
     +---com.squareup.leakcanary:leakcanary-android-core:2.7
          +---com.squareup.leakcanary:leakcanary-object-watcher-android-androidx:2.7
               +---com.squareup.leakcanary:leakcanary-object-watcher-android:2.7
                    +---com.squareup.curtains:curtains:1.0.1
 +---com.squareup.leakcanary:leakcanary-android:2.7
     +---com.squareup.leakcanary:leakcanary-android-core:2.7
          +---com.squareup.leakcanary:leakcanary-object-watcher-android-support-fragments:2.7
               +---com.squareup.leakcanary:leakcanary-object-watcher-android:2.7
                    +---com.squareup.curtains:curtains:1.0.1
 Issues found:
 * curtains\internal\WindowCallbackWrapper$Companion$jetpackWrapperClass$2.class -> android.support.v7.view.WindowCallbackWrapper

[maxirosson]

The curtains artifact could potentially be used on production (if any project declares it directly instead of a leak canary transitive dependency). So, I can't ignore it, because it will be dangerous for some projects. In this particular case, you should manually ignore it (only if you are not using that artifact on production code)

[r4zzz4k]

@maxirosson Could you please clarify how exactly does this class can be an offender? I've walked through the history of the file, this is the first revision where the reference to android.support.v7.view.WindowCallbackWrapper appears, and it's already loaded dynamically and is superseded by androidx counterpart if present. Genuinely asking, as I might be missing something obvious.

https://github.com/square/curtains/blob/ab6b6dbe9ab0db307a7251a32d87cbc8e8e7ff8e/curtains/src/main/java/curtains/internal/WindowCallbackWrapper.kt#L51-L58

[pyricau]

Noticed the answers here as I'm going through my notifications => yes, Curtains usage is 100% safe, doesn't require either the support or the androidx libs, and isn't a proper offender.

[maxirosson]

Hi. Sorry for my late response.

According to this code: https://github.com/square/curtains/blob/main/curtains/src/main/java/curtains/internal/WindowCallbackWrapper.kt

    private val jetpackWrapperClass by lazy(NONE) {
      try {
        Class.forName("androidx.appcompat.view.WindowCallbackWrapper")
      } catch (ignored: Throwable) {
        try {
          Class.forName("android.support.v7.view.WindowCallbackWrapper")
        } catch (ignored: Throwable) {
          null
        }
      }
    }

the curtains lib is not a real offender, you are right. The Bye-bye-jetifier can't detect these cases where there is a condition on the code, so a false positive is triggered.

Given that this lib could be also used in production (not only on development environments), I can't exclude it by default. Excluding it by default would be dangerous, any real support reference could appear on the lib and the plugin wouldn't detect it. So, you have two possible solutions:

1. Create a flag on gradle config to only add the leak canary dependency when it is true. So, any dev can decide to enable or not leak canary, and you can also can run bye bye jetifier with the flag on false.
2. Manually exclude the WindowCallbackWrapper class using the excludedFilesFromScanning extension

[pyricau]

These solutions add friction for every dev using both leakcanary / curtains and bye-bye-jetifier, so that's not ideal.

I'm guessing one possible fix is to mingle the names so that bye-bye-jetifier can't match the class names. But that doesn't work if a library optionally loads support lib bytecode IF the support lib is in the classpath (leakcanary does that).

I kinda wish libraries could automatically embed a config file that tells bye-bye-jetifier that some classes are not going to cause trouble / should be excluded from scanning (ie same as excludedFilesFromScanning but provided by the lib)

[r4zzz4k]

Given that this lib could be also used in production (not only on development environments), I can't exclude it by default. Excluding it by default would be dangerous, any real support reference could appear on the lib and the plugin wouldn't detect it.

Please correct me if I didn't get that right. You mean if you exclude WindowCallbackWrapper, and then library authors add unconditional dependency on some support library class (to the library where they purposely avoided that), there would be an issue for bye-bye-jetifier users as this dependency would be ignored? Doesn't the same argumentation work for any kind of exclusions mentioned here, including LeakCanary itself?

I mean, I'm okay with adding this exclusion manually as an end user, I'm just curious about the reasoning.

[maxirosson]

After thinking this again, I decided to add the curtains exclude by default, so we simplify the configuration for leak canary users. I will release v1.2.1 with this fix, soon.

[maxirosson]

Bye Bye Jetifier v1.2.1 released. The following exclusion was added by default:

"curtains/internal/WindowCallbackWrapper"

[GrahamBorland]

I've just upgraded to v1.2.1 and I'm still getting failures with LeakCanary if I disable my custom exclusion rules.

./gradlew canISayByeByeJetifier -Pandroid.enableJetifier=false

Type-safe dependency accessors is an incubating feature.
Type-safe project accessors is an incubating feature.

> Configure project :app
WARNING:API 'BaseVariant.getApplicationIdTextResource' is obsolete and has been replaced with 'VariantProperties.applicationId'.
It will be removed in version 7.0 of the Android Gradle plugin.
For more information, see TBD.
To determine what is calling BaseVariant.getApplicationIdTextResource, use -Pandroid.debug.obsoleteApi=true on the command line to display more information.

> Task :canISayByeByeJetifier

=========================================
Project: ynab_android
=========================================
 * No legacy android support usages found

=========================================
Project: app
=========================================

Scanning com.squareup.leakcanary:shark-android:2.8.1
 Absolute path: /Users/graham/.gradle/caches/modules-2/files-2.1/com.squareup.leakcanary/shark-android/2.8.1/ddc4790724faa2256709077d440e13e66dc57175/shark-android-2.8.1.jar
 Graphs to this dependency:
 +---com.squareup.leakcanary:leakcanary-android:2.8.1
     +---com.squareup.leakcanary:leakcanary-android-core:2.8.1
          +---com.squareup.leakcanary:shark-android:2.8.1
 Issues found:
 * shark/AndroidReferenceMatchers$ACTIVITY_CHOOSE_MODEL.class -> android.support.v7.internal.widget.ActivityChooserModel

Scanning com.squareup.leakcanary:leakcanary-object-watcher-android-support-fragments:2.8.1
 Absolute path: /Users/graham/.gradle/caches/modules-2/files-2.1/com.squareup.leakcanary/leakcanary-object-watcher-android-support-fragments/2.8.1/18d1cb2d7b6c3271d02d3e7f39da91c99d8680ea/leakcanary-object-watcher-android-support-fragments-2.8.1.aar
 Graphs to this dependency:
 +---com.squareup.leakcanary:leakcanary-android:2.8.1
     +---com.squareup.leakcanary:leakcanary-android-core:2.8.1
          +---com.squareup.leakcanary:leakcanary-object-watcher-android-support-fragments:2.8.1
 Issues found:
 * leakcanary/internal/AndroidSupportFragmentDestroyWatcher$fragmentLifecycleCallbacks$1.class -> android/support/v4/app/FragmentManager$FragmentLifecycleCallbacks
 * leakcanary/internal/AndroidSupportFragmentDestroyWatcher$fragmentLifecycleCallbacks$1.class -> android/support/v4/app/FragmentManager
 * leakcanary/internal/AndroidSupportFragmentDestroyWatcher$fragmentLifecycleCallbacks$1.class -> android/support/v4/app/Fragment
 * leakcanary/internal/AndroidSupportFragmentDestroyWatcher.class -> android/support/v4/app/FragmentActivity
 * leakcanary/internal/AndroidSupportFragmentDestroyWatcher.class -> android/support/v4/app/FragmentManager
 * leakcanary/internal/AndroidSupportFragmentDestroyWatcher.class -> android/support/v4/app/FragmentManager$FragmentLifecycleCallbacks

Scanning com.squareup.curtains:curtains:1.2.3
 Absolute path: /Users/graham/.gradle/caches/modules-2/files-2.1/com.squareup.curtains/curtains/1.2.3/69b71aa1f4662085af4183ccefd6fcef201befef/curtains-1.2.3.aar
 Graphs to this dependency:
 +---com.squareup.leakcanary:leakcanary-android:2.8.1
     +---com.squareup.leakcanary:leakcanary-android-core:2.8.1
          +---com.squareup.leakcanary:leakcanary-object-watcher-android-core:2.8.1
               +---com.squareup.curtains:curtains:1.2.3
 +---com.squareup.leakcanary:leakcanary-android:2.8.1
     +---com.squareup.leakcanary:leakcanary-object-watcher-android:2.8.1
          +---com.squareup.leakcanary:leakcanary-object-watcher-android-core:2.8.1
               +---com.squareup.curtains:curtains:1.2.3
 +---com.squareup.leakcanary:leakcanary-android:2.8.1
     +---com.squareup.leakcanary:leakcanary-android-core:2.8.1
          +---com.squareup.leakcanary:leakcanary-object-watcher-android-androidx:2.8.1
               +---com.squareup.leakcanary:leakcanary-object-watcher-android-core:2.8.1
                    +---com.squareup.curtains:curtains:1.2.3
 +---com.squareup.leakcanary:leakcanary-android:2.8.1
     +---com.squareup.leakcanary:leakcanary-android-core:2.8.1
          +---com.squareup.leakcanary:leakcanary-object-watcher-android-support-fragments:2.8.1
               +---com.squareup.leakcanary:leakcanary-object-watcher-android-core:2.8.1
                    +---com.squareup.curtains:curtains:1.2.3
 +---com.squareup.leakcanary:leakcanary-android:2.8.1
     +---com.squareup.leakcanary:plumber-android:2.8.1
          +---com.squareup.leakcanary:plumber-android-core:2.8.1
               +---com.squareup.curtains:curtains:1.2.3
 Issues found:
 * curtains/internal/WindowCallbackWrapper$Companion$jetpackWrapperClass$2.class -> android.support.v7.view.WindowCallbackWrapper

If I add these lines back into the excludedFilesFromScanning config, then the check passes.

            "shark/AndroidReferenceMatchers",
            "leakcanary/internal/AndroidSupportFragmentDestroyWatcher",
            "curtains/internal/WindowCallbackWrapper",

[maxirosson]

I tried these dependencies

    implementation('com.squareup.leakcanary:leakcanary-android:2.8.1')
    implementation('com.squareup.leakcanary:shark-android:2.8.1')
    implementation('com.squareup.leakcanary:leakcanary-object-watcher-android-support-fragments:2.8.1')
    implementation('com.squareup.curtains:curtains:1.2.3')

with Bye Bye Jetifier v1.2.2 and couldn't reproduce this issue.

Are you sure you are not overriding the default configuration, doing something like this?

excludedFilesFromScanning = []

[GrahamBorland]

I still have other custom exclusions in the excludedFilesFromScanning block.

What's the best way to extend, rather than replace, the default configuration?

[maxirosson]

Try with

excludedFilesFromScanning += [custom exclusions]

[GrahamBorland]

That solves the problem, thanks. 😄