ronantakizawa/codereview-bench huggingface dataset:
Repository: square/leakcanary
File: leakcanary-android-core/src/test/java/leakcanary/LeakCanaryConfigTest.kt (Kotlin)

Code before:
package leakcanary

import org.assertj.core.api.Assertions.assertThat
import org.junit.Test
import kotlin.reflect.full.memberFunctions
import kotlin.reflect.full.memberProperties

class LeakCanaryConfigTest {

  /**
   * Validates that each field in [LeakCanary.Config] has a matching builder function
   * in [LeakCanary.Config.Builder]
   */
  @Test fun `LeakCanary Config Builder matches LeakCanary Config`() {
    val propertiesInConfig = LeakCanary.Config::class.memberProperties
        .filter { member ->
          // Ignore deprecated fields, we don't need builders for those
          member.annotations.none { it is Deprecated }
        }
        .map { it.name }

    val functionsInConfigBuilder = LeakCanary.Config.Builder::class.memberFunctions
        .map { it.name }


    val missingInConfigBuilder = propertiesInConfig.subtract(functionsInConfigBuilder)
    assertThat(missingInConfigBuilder)
        .withFailMessage("Fields in LeakCanary.Config don't have matching " +
            "builder functions in LeakCanary.Config.Builder: $missingInConfigBuilder")
        .isEmpty()
  }
}

Review comment (suggestion):
Should have another test for the opposite case, a field in the builder that's not in the config class. Less likely but maybe with deprecation.

Code after:
package leakcanary

import org.assertj.core.api.Assertions.assertThat
import org.junit.Test
import kotlin.reflect.full.memberFunctions
import kotlin.reflect.full.memberProperties

class LeakCanaryConfigTest {

  /**
   * Validates that each field in [LeakCanary.Config] has a matching builder function
   * in [LeakCanary.Config.Builder]
   */
  @Test fun `LeakCanary Config Builder matches LeakCanary Config`() {
    assertThat(configProperties())
        .containsExactlyInAnyOrderElementsOf(configBuilderFunctions())
  }

  private fun configBuilderFunctions() = LeakCanary.Config.Builder::class.memberFunctions
      .map { it.name }
      .subtract(listOf("build", "equals", "hashCode", "toString"))

  private fun configProperties() = LeakCanary.Config::class.memberProperties
      .filter { member ->
        // Ignore deprecated fields, we don't need builders for those
        member.annotations.none { it is Deprecated }
      }
      .map { it.name }
}
Repository: square/leakcanary
File: leakcanary-android-core/src/test/java/leakcanary/LeakCanaryConfigTest.kt (Kotlin)

Code before:
import kotlin.reflect.full.memberProperties

class LeakCanaryConfigTest {

  private fun configBuilderFunctions() = LeakCanary.Config.Builder::class.memberFunctions
      .map { it.name }

  private fun configProperties() = LeakCanary.Config::class.memberProperties
      .filter { member ->
        // Ignore deprecated fields, we don't need builders for those
        member.annotations.none { it is Deprecated }
      }
      .map { it.name }

  /**
   * Validates that each field in [LeakCanary.Config] has a matching builder function
   * in [LeakCanary.Config.Builder]
   */
  @Test fun `LeakCanary Config Builder matches LeakCanary Config`() {
    val propertiesInConfig = configProperties()
    val functionsInConfigBuilder = configBuilderFunctions()

    val missingInConfigBuilder = propertiesInConfig.subtract(functionsInConfigBuilder)
    assertThat(missingInConfigBuilder)
        .withFailMessage("Fields in LeakCanary.Config don't have matching " +
            "builder functions in LeakCanary.Config.Builder: $missingInConfigBuilder")
        .isEmpty()
  }

  /**
   * Validates that each method in [LeakCanary.Config.Builder] has a matching field
   * in [LeakCanary.Config]
   */
  @Test fun `LeakCanary Config matches LeakCanary Config Builder`() {
    // Grab list of functions in Builder (without build(), toString(), etc)
    val functionsInConfigBuilder = configBuilderFunctions()
        .subtract(listOf("build", "equals", "hashCode", "toString"))

    val propertiesInConfig = configProperties()
    val missingInConfig = functionsInConfigBuilder.subtract(propertiesInConfig)
    assertThat(missingInConfig)
        .withFailMessage("Builder functions in LeakCanary.Config.Builder don't " +
            "have matching fields in LeakCanary.Config: $missingInConfig")
        .isEmpty()
  }
}

Review comment (suggestion):
instead of subtract, how about asserting the two lists are equal? That way the framework tells us what's different. If there's a reason not to use equals, then the error message should include both lists

Code after:
package leakcanary

import org.assertj.core.api.Assertions.assertThat
import org.junit.Test
import kotlin.reflect.full.memberFunctions
import kotlin.reflect.full.memberProperties

class LeakCanaryConfigTest {

  /**
   * Validates that each field in [LeakCanary.Config] has a matching builder function
   * in [LeakCanary.Config.Builder]
   */
  @Test fun `LeakCanary Config Builder matches LeakCanary Config`() {
    assertThat(configProperties())
        .containsExactlyInAnyOrderElementsOf(configBuilderFunctions())
  }

  private fun configBuilderFunctions() = LeakCanary.Config.Builder::class.memberFunctions
      .map { it.name }
      .subtract(listOf("build", "equals", "hashCode", "toString"))

  private fun configProperties() = LeakCanary.Config::class.memberProperties
      .filter { member ->
        // Ignore deprecated fields, we don't need builders for those
        member.annotations.none { it is Deprecated }
      }
      .map { it.name }
}
Repository: square/leakcanary
File: leakcanary-object-watcher-android/src/main/java/leakcanary/AppWatcher.kt (Kotlin)

Code before:
  ) {
    SharkLog.d {
      val changedFields = mutableListOf<String>()
      Config::class.java.declaredFields.forEach { field ->
        field.isAccessible = true
        val previousValue = field[previousConfig]
        val newValue = field[newConfig]
        if (previousValue != newValue) {
          changedFields += "${field.name}=$newValue"
        }
      }

      val changesInConfig =
        if (changedFields.isNotEmpty()) changedFields.joinToString(", ") else "no changes"

      "Updated AppWatcher.config: Config($changesInConfig)"
    }
  }

  /**
   * The [ObjectWatcher] used by AppWatcher to detect retained objects.
   */
  val objectWatcher
    get() = InternalAppWatcher.objectWatcher

  val isDebuggableBuild
    get() = InternalAppWatcher.isDebuggableBuild

  /** @see [manualInstall] */
  val isInstalled
    get() = InternalAppWatcher.isInstalled

  /**
   * [AppWatcher] is automatically installed on main process start by
   * [leakcanary.internal.AppWatcherInstaller] which is registered in the AndroidManifest.xml of
   * your app. If you disabled [leakcanary.internal.AppWatcherInstaller] or you need AppWatcher
   * or LeakCanary to run outside of the main process then you can call this method to install
   * [AppWatcher].
   */
  fun manualInstall(application: Application) {
    InternalAppWatcher.install(application)
    config = config.copy(enabled = InternalAppWatcher.isDebuggableBuild)
  }

}

Review comment (bug):
This is not pretty. But it was the easiest way I could think of to make `InternalAppWatcher.isDebuggableBuild` accessible to `InternalLeakCanary`.

Code after:
  ) {
    SharkLog.d {
      val changedFields = mutableListOf<String>()
      Config::class.java.declaredFields.forEach { field ->
        field.isAccessible = true
        val previousValue = field[previousConfig]
        val newValue = field[newConfig]
        if (previousValue != newValue) {
          changedFields += "${field.name}=$newValue"
        }
      }

      val changesInConfig =
        if (changedFields.isNotEmpty()) changedFields.joinToString(", ") else "no changes"

      "Updated AppWatcher.config: Config($changesInConfig)"
    }
  }

  /**
   * The [ObjectWatcher] used by AppWatcher to detect retained objects.
   */
  val objectWatcher
    get() = InternalAppWatcher.objectWatcher

  /** @see [manualInstall] */
  val isInstalled
    get() = InternalAppWatcher.isInstalled

  /**
   * [AppWatcher] is automatically installed on main process start by
   * [leakcanary.internal.AppWatcherInstaller] which is registered in the AndroidManifest.xml of
   * your app. If you disabled [leakcanary.internal.AppWatcherInstaller] or you need AppWatcher
   * or LeakCanary to run outside of the main process then you can call this method to install
   * [AppWatcher].
   */
  fun manualInstall(application: Application) {
    InternalAppWatcher.install(application)
    config = config.copy(enabled = InternalAppWatcher.isDebuggableBuild)
  }

}
Repository: square/leakcanary
File: leakcanary-android-core/src/main/java/leakcanary/internal/activity/screen/LeakScreen.kt (Kotlin)

Code before:
            shareHeapDump(analysis.heapDumpFile)
          }
        }
        else -> null
      }
    }

    val adapter = DisplayLeakAdapter(context, leakTrace, title)
    listView.adapter = adapter
  }

  private fun leakToString(
    leakTrace: LeakTrace,
    analysis: HeapAnalysisSuccess
  ): String {
    var leakStr = """$leakTrace

METADATA

${if (analysis.metadata.isNotEmpty()) analysis.metadata.map { "${it.key}: ${it.value}" }.joinToString(
        "\n"
    ) else ""}
Analysis duration: ${analysis.analysisDurationMillis} ms"""

    if (LeakCanary.config.wrapLeakTraceWhenSharing) {
      leakStr = LeakTraceWordWrapper.wrap(leakStr, 80)
    }
    return leakStr
  }
}


Review comment (suggestion):
nit: avoid 3 letter variable names (e.g. str), use full nouns instead. 

Here, instead of using a var you can return the original vs the wrapped one.

Code after:
    val titleText = """
      Open <a href="open_analysis">Heap Dump</a><br><br>
      Share leak trace <a href="share">as text</a> or on <a href="share_stack_overflow">Stack Overflow</a><br><br>
      Share <a href="share_hprof">Heap Dump file</a><br><br>
      References <b><u>underlined</u></b> are the likely causes of the leak.
      Learn more at <a href="https://squ.re/leaks">https://squ.re/leaks</a>
    """.trimIndent() + if (selectedLeak is LibraryLeak) "<br><br>" +
        "A <font color='#FFCC32'>Library Leak</font> is a leak caused by a known bug in 3rd party code that you do not have control over. " +
        "(<a href=\"https://square.github.io/leakcanary/fundamentals-how-leakcanary-works/#4-categorizing-leaks\">Learn More</a>)<br><br>" +
        "<b>Leak pattern</b>: ${selectedLeak.pattern}<br><br>" +
        "<b>Description</b>: ${parseLinks(selectedLeak.description)}" else ""

    val title = Html.fromHtml(titleText) as SpannableStringBuilder
    SquigglySpan.replaceUnderlineSpans(title, context)

    replaceUrlSpanWithAction(title) { urlSpan ->
      when (urlSpan) {
        "share" -> {
          {
            share(LeakTraceWrapper.wrap(leakToString(leakTrace, analysis), 80))
          }
        }
        "share_stack_overflow" -> {
          {
            shareToStackOverflow(LeakTraceWrapper.wrap(leakToString(leakTrace, analysis), 80))
          }
        }
        "open_analysis" -> {
          {
            goTo(HeapDumpScreen(heapAnalysisId))
          }
        }
        "share_hprof" -> {
          {
            shareHeapDump(analysis.heapDumpFile)
          }
        }
        else -> null
      }
    }

    val adapter = DisplayLeakAdapter(context, leakTrace, title)
    listView.adapter = adapter
  }

  private fun leakToString(
    leakTrace: LeakTrace,
    analysis: HeapAnalysisSuccess
  ) = """$leakTrace

METADATA
Repository: square/leakcanary
File: shark-android/src/main/java/shark/AndroidObjectInspectors.kt (Kotlin)

Code before:
            if (mPhoneWindowField != null) {
              val phoneWindow = mPhoneWindowField.valueAsInstance!!
              context = phoneWindow.getOrThrow("android.view.Window", "mContext").valueAsInstance!!
              if (context instanceOf "android.app.Activity") {
                return context
              }
            }
          }
          if (context instanceOf "android.content.ContextWrapper" &&
              // Avoids infinite loops
              context.objectId !in visitedInstances
          ) {
            keepUnwrapping = true
          }
        }
      }
    }
  }
  return null
}

/**
 * Same as [HeapInstance.readField] but throws if the field doesnt exist
 */
internal fun HeapInstance.getOrThrow(
  declaringClassName: String,
  fieldName: String
): HeapField {
  return this[declaringClassName, fieldName] ?: throw IllegalStateException(
      """
The heap dump may contain obfuscated class names. If that's the case then the heap analysis 
is unable to proceed without a mapping file to deobfuscate class names. You can turn off
obfuscation or run LeakCanary on obfuscated builds by following the instructions at 
https://square.github.io/leakcanary/recipes/#using-leakcanary-with-obfuscated-apps
      """
  )
}

Review comment (suggestion):
Let's add a bit more context, e.g.:

```
"""
$instanceClassName is expected to have a $declaringClassName.$fieldName field which cannot be found. This might be due to the app code being obfuscated. If that's the case, then the heap analysis 
is unable to proceed without a mapping file to deobfuscate class names. You can run LeakCanary on obfuscated builds by following the instructions at https://square.github.io/leakcanary/recipes/#using-leakcanary-with-obfuscated-apps
"""
```

Code after:
              context.objectId !in visitedInstances
          ) {
            keepUnwrapping = true
          }
        }
      }
    }
  }
  return null
}

/**
 * Same as [HeapInstance.readField] but throws if the field doesnt exist
 */
internal fun HeapInstance.getOrThrow(
  declaringClassName: String,
  fieldName: String
): HeapField {
  return this[declaringClassName, fieldName] ?: throw IllegalStateException(
      """
$instanceClassName is expected to have a $declaringClassName.$fieldName field which cannot be found. 
This might be due to the app code being obfuscated. If that's the case, then the heap analysis 
is unable to proceed without a mapping file to deobfuscate class names. 
You can run LeakCanary on obfuscated builds by following the instructions at 
https://square.github.io/leakcanary/recipes/#using-leakcanary-with-obfuscated-apps
      """
  )
}
Repository: square/leakcanary
File: shark/src/main/java/shark/HeapAnalysis.kt (Kotlin)

Code before:
package shark

import shark.internal.createSHA1Hash
import java.io.File
import java.io.Serializable

/**
 * The result of an analysis performed by [HeapAnalyzer], either a [HeapAnalysisSuccess] or a
 * [HeapAnalysisFailure]. This class is serializable however there are no guarantees of forward
 * compatibility.
 */
sealed class HeapAnalysis : Serializable {
  /**
   * The hprof file that was analyzed.
   */
  abstract val heapDumpFile: File

  /**
   * The [System.currentTimeMillis] when this [HeapAnalysis] instance was created.
   */
  abstract val createdAtTimeMillis: Long

  /**
   * Total time spent dumping the heap.
   */
  abstract val dumpDurationMillis: Long

  /**
   * Total time spent analyzing the heap.
   */
  abstract val analysisDurationMillis: Long

  companion object {
    private const val serialVersionUID: Long = -8657286725869987172
  }
}

/**
 * The analysis performed by [HeapAnalyzer] did not complete successfully.
 */
data class HeapAnalysisFailure(
    override val heapDumpFile: File,
    override val createdAtTimeMillis: Long,
    override val dumpDurationMillis: Long,
    override val analysisDurationMillis: Long,
    /**
     * An exception wrapping the actual exception that was thrown.
     */
    val exception: HeapAnalysisException
) : HeapAnalysis() {


Review comment (suggestion):
Add a `DUMP_DURATION_UNKNOWN = -1` constant, mention it here and then use it everywhere instead of `-1`

Code after:
package shark

import shark.internal.createSHA1Hash
import java.io.File
import java.io.Serializable

/**
 * The result of an analysis performed by [HeapAnalyzer], either a [HeapAnalysisSuccess] or a
 * [HeapAnalysisFailure]. This class is serializable however there are no guarantees of forward
 * compatibility.
 */
sealed class HeapAnalysis : Serializable {
  /**
   * The hprof file that was analyzed.
   */
  abstract val heapDumpFile: File

  /**
   * The [System.currentTimeMillis] when this [HeapAnalysis] instance was created.
   */
  abstract val createdAtTimeMillis: Long

  /**
   * Total time spent dumping the heap.
   */
  abstract val dumpDurationMillis: Long

  /**
   * Total time spent analyzing the heap.
   */
  abstract val analysisDurationMillis: Long

  companion object {
    private const val serialVersionUID: Long = -8657286725869987172
    const val DUMP_DURATION_UNKNOWN: Long = -1
  }
}

/**
 * The analysis performed by [HeapAnalyzer] did not complete successfully.
 */
data class HeapAnalysisFailure(
  override val heapDumpFile: File,
  override val createdAtTimeMillis: Long,
  override val dumpDurationMillis: Long = DUMP_DURATION_UNKNOWN,
  override val analysisDurationMillis: Long,
  /**
   * An exception wrapping the actual exception that was thrown.
   */
  val exception: HeapAnalysisException
) : HeapAnalysis() {
Repository: square/leakcanary
File: leakcanary-android-core/src/main/java/leakcanary/internal/activity/db/HeapAnalysisTable.kt (Kotlin)

Code before:
              )
            }
      }
      heapAnalysisId
    }.apply { notifyUpdateOnMainThread() }
  }

  private fun notifyUpdateOnMainThread() {
    if (Looper.getMainLooper().thread === Thread.currentThread()) {
      throw UnsupportedOperationException(
          "Should not be called from the main thread"
      )
    }
    mainHandler.post {
      updateListeners.forEach { it() }
    }
  }

  inline fun <reified T : HeapAnalysis> retrieve(
    db: SQLiteDatabase,
    id: Long
  ): T? {
    db.inTransaction {
      return db.rawQuery(
          """
              SELECT object
              FROM heap_analysis
              WHERE id=$id
              """, null
      )
          .use { cursor ->
            if (cursor.moveToNext()) {
              val analysis = Serializables.fromByteArray<T>(cursor.getBlob(0))
              if (analysis == null) {
                delete(db, id, null)
              }
              analysis
            } else {
              null
            }
          } ?: return null
    }
  }

  fun retrieveAll(db: SQLiteDatabase): List<Projection> {
    return db.rawQuery(
        """
          SELECT
          id
          , created_at_time_millis
          , leak_count

Review comment (suggestion):
One thing to test here is that leaks that were recorded prior to this update can still be displayed after this update

Code after:
              )
            }
      }
      heapAnalysisId
    }.apply { notifyUpdateOnMainThread() }
  }

  private fun notifyUpdateOnMainThread() {
    if (Looper.getMainLooper().thread === Thread.currentThread()) {
      throw UnsupportedOperationException(
          "Should not be called from the main thread"
      )
    }
    mainHandler.post {
      updateListeners.forEach { it() }
    }
  }

  inline fun <reified T : HeapAnalysis> retrieve(
    db: SQLiteDatabase,
    id: Long
  ): T? {
    db.inTransaction {
      return db.rawQuery(
          """
              SELECT
              object
              FROM heap_analysis
              WHERE id=$id
              """, null
      )
          .use { cursor ->
            if (cursor.moveToNext()) {
              val analysis = Serializables.fromByteArray<T>(cursor.getBlob(0))
              if (analysis == null) {
                delete(db, id, null)
              }
              analysis
            } else {
              null
            }
          } ?: return null
    }
  }

  fun retrieveAll(db: SQLiteDatabase): List<Projection> {
    return db.rawQuery(
        """
          SELECT
          id
          , created_at_time_millis
Repository: square/leakcanary
File: leakcanary-android-core/src/main/java/leakcanary/internal/utils/Size.kt (Kotlin)

Code before:
package leakcanary.internal.utils


// https://stackoverflow.com/a/3758880
fun humanReadableByteCount(
        bytes: Long,
        si: Boolean
): String {
    val unit = if (si) 1000 else 1024
    if (bytes < unit) return "$bytes B"
    val exp = (Math.log(bytes.toDouble()) / Math.log(unit.toDouble())).toInt()
    val pre = (if (si) "kMGTPE" else "KMGTPE")[exp - 1] + if (si) "" else "i"
    return String.format("%.1f %sB", bytes / Math.pow(unit.toDouble(), exp.toDouble()), pre)
}


Review comment (bug):
Here, I kept original Stackoverflow impl not applying Kotlin linter suggestion (replacing Java Math APIs with Kotlin equivalents).
Shouldn't we fully embrace Kotlin here given that original source is already altered from Java to Kotlin?

Code after:
package leakcanary.internal.utils

import kotlin.math.ln
import kotlin.math.pow


// https://stackoverflow.com/a/3758880
internal fun humanReadableByteCount(
        bytes: Long,
        si: Boolean
): String {
    val unit = if (si) 1000 else 1024
    if (bytes < unit) return "$bytes B"
    val exp = (ln(bytes.toDouble()) / ln(unit.toDouble())).toInt()
    val pre = (if (si) "kMGTPE" else "KMGTPE")[exp - 1] + if (si) "" else "i"
    return String.format("%.1f %sB", bytes / unit.toDouble().pow(exp.toDouble()), pre)
}
Repository: square/leakcanary
File: leakcanary-android-core/src/main/java/leakcanary/internal/NotificationReceiver.kt (Kotlin)

Code before:
  override fun onReceive(
    context: Context,
    intent: Intent
  ) {
    when (intent.action) {
      DUMP_HEAP.name -> {
        InternalLeakCanary.onDumpHeapReceived(forceDump = false)
      }
      CANCEL_NOTIFICATION.name -> {
        // Do nothing, the notification has auto cancel true.
      }
      else -> {
        SharkLog.d { "NotificationReceiver received unknown intent action for $intent" }
      }
    }
  }

  companion object {
    fun pendingIntent(
      context: Context,
      action: Action
    ): PendingIntent {
      val broadcastIntent = Intent(context, NotificationReceiver::class.java)
      broadcastIntent.action = action.name
      return PendingIntent.getBroadcast(context, 0, broadcastIntent, 0)
    }
  }
}

Review comment (suggestion):
Using Build.VERSION_CODES.R instead of "30" requires updating `compileSDK` from 29 to 30. This update caused other issues. So, it is better to do `compileSDK` update in a separate PR

Code after:

  override fun onReceive(
    context: Context,
    intent: Intent
  ) {
    when (intent.action) {
      DUMP_HEAP.name -> {
        InternalLeakCanary.onDumpHeapReceived(forceDump = false)
      }
      CANCEL_NOTIFICATION.name -> {
        // Do nothing, the notification has auto cancel true.
      }
      else -> {
        SharkLog.d { "NotificationReceiver received unknown intent action for $intent" }
      }
    }
  }

  companion object {
    fun pendingIntent(
      context: Context,
      action: Action
    ): PendingIntent {
      val broadcastIntent = Intent(context, NotificationReceiver::class.java)
      broadcastIntent.action = action.name
      val flags = if (Build.VERSION.SDK_INT > 30) {
        PendingIntent.FLAG_IMMUTABLE
      } else {
        0
      }
      return PendingIntent.getBroadcast(context, 0, broadcastIntent, flags)
    }
  }
}