For #15279: add MozillaUseLazyMonitored custom detekt rule.

config/detekt.yml

@@ -120,6 +120,8 @@ mozilla-detekt-rules:
   MozillaRunBlockingCheck:
     active: true
     excludes: "**/*Test.kt, **/*Spec.kt, **/test/**, **/androidTest/**"
+  MozillaUseLazyMonitored:
+    active: true
 
 empty-blocks:
   active: true

mozilla-detekt-rules/build.gradle

@@ -2,6 +2,7 @@ apply plugin: 'kotlin'
 
 dependencies {
   compileOnly Deps.detektApi
+  implementation Deps.androidx_annotation
 
   // I didn't look thoroughly enough to really know what's going on here but I think
   // the detekt API uses jdk8 so if we provide jdk7, the dependency collision system

mozilla-detekt-rules/src/main/java/org/mozilla/fenix/detektrules/CustomRulesetProvider.kt

@@ -7,6 +7,7 @@ package org.mozilla.fenix.detektrules
 import io.gitlab.arturbosch.detekt.api.Config
 import io.gitlab.arturbosch.detekt.api.RuleSet
 import io.gitlab.arturbosch.detekt.api.RuleSetProvider
+import org.mozilla.fenix.detektrules.perf.MozillaUseLazyMonitored
 
 class CustomRulesetProvider : RuleSetProvider {
     override val ruleSetId: String = "mozilla-detekt-rules"
@@ -17,7 +18,8 @@ class CustomRulesetProvider : RuleSetProvider {
             MozillaBannedPropertyAccess(config),
             MozillaStrictModeSuppression(config),
             MozillaCorrectUnitTestRunner(config),
-            MozillaRunBlockingCheck(config)
+            MozillaRunBlockingCheck(config),
+            MozillaUseLazyMonitored(config)
         )
     )
 }

mozilla-detekt-rules/src/main/java/org/mozilla/fenix/detektrules/perf/MozillaUseLazyMonitored.kt

@@ -0,0 +1,86 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+package org.mozilla.fenix.detektrules.perf
+
+import androidx.annotation.VisibleForTesting
+import androidx.annotation.VisibleForTesting.PRIVATE
+import io.gitlab.arturbosch.detekt.api.CodeSmell
+import io.gitlab.arturbosch.detekt.api.Config
+import io.gitlab.arturbosch.detekt.api.Debt
+import io.gitlab.arturbosch.detekt.api.Entity
+import io.gitlab.arturbosch.detekt.api.Issue
+import io.gitlab.arturbosch.detekt.api.Rule
+import io.gitlab.arturbosch.detekt.api.Severity
+import io.gitlab.arturbosch.detekt.api.internal.PathFilters
+import org.jetbrains.kotlin.psi.KtProperty
+import org.jetbrains.kotlin.psi.KtPropertyDelegate
+
+private const val FUN_LAZY = "lazy"
+
+private const val ERROR_MESSAGE = "Please use `by lazyMonitored` instead of `by lazy` " +
+    "to declare application components (e.g. variables in Components.kt): it contains extra code to " +
+    "monitor for performance regressions. This change is not necessary for non-components."
+
+/**
+ * Prevents use of lazy in component groups files where lazyMonitored should be used instead.
+ */
+open class MozillaUseLazyMonitored(config: Config) : Rule(config) {
+    override val issue = Issue(
+        this::class.java.simpleName,
+        Severity.Performance,
+        "Prevents use of lazy in component groups files where lazyMonitored should be used instead",
+        Debt.FIVE_MINS
+    )
+
+    override val filters = PathFilters.of(
+        includes = COMPONENT_GROUP_FILES.map { "**$it" },
+        excludes = emptyList()
+    )
+
+    override fun visitPropertyDelegate(delegate: KtPropertyDelegate) {
+        super.visitPropertyDelegate(delegate)
+
+        val delegateFunction = delegate.expression?.node?.firstChildNode // the "lazy" in "by lazy { ..."
+        if (delegateFunction?.chars == FUN_LAZY) {
+            report(CodeSmell(issue, Entity.from(delegate), ERROR_MESSAGE))
+        }
+    }
+
+    override fun visitProperty(property: KtProperty) {
+        super.visitProperty(property)
+
+        // I only expect components to be declared as members, not at the top level or inside block scopes.
+        if (!property.isMember) return
+
+        val rightHandSideOfAssignment = property.initializer
+
+        // This is intended to catch `val example = lazy {` or `... lazy(`. It may be possible to
+        // make lazy not the first node to the right of the equal sign but it's probably uncommon
+        // enough that we don't handle that case.
+        val assignedFunction = rightHandSideOfAssignment?.firstChild
+        if (assignedFunction?.node?.chars == FUN_LAZY) {
+            report(CodeSmell(issue, Entity.from(property), ERROR_MESSAGE))
+        }
+    }
+
+    companion object {
+        /**
+         * All files that represent a "component group".
+         */
+        @VisibleForTesting(otherwise = PRIVATE)
+        val COMPONENT_GROUP_FILES = listOf(
+            "Analytics",
+            "BackgroundServices",
+            "Components",
+            "Core",
+            "IntentProcessors",
+            "PerformanceComponent",
+            "Push",
+            "Search",
+            "Services",
+            "UseCases"
+        ).map { "app/src/main/java/org/mozilla/fenix/components/$it.kt" }
+    }
+}

mozilla-detekt-rules/src/test/java/org/mozilla/fenix/detektrules/perf/MozillaUseLazyMonitoredTest.kt

@@ -0,0 +1,100 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+package org.mozilla.fenix.detektrules.perf
+
+import io.gitlab.arturbosch.detekt.api.Config
+import io.gitlab.arturbosch.detekt.api.internal.PathFilters
+import io.gitlab.arturbosch.detekt.test.lint
+import org.junit.jupiter.api.Assertions.*
+import org.junit.jupiter.api.BeforeEach
+import org.junit.jupiter.api.Test
+import org.junit.jupiter.params.ParameterizedTest
+import org.junit.jupiter.params.provider.ValueSource
+import java.io.File
+import java.nio.file.Paths
+
+internal class MozillaUseLazyMonitoredTest {
+
+    private lateinit var actualCheck: MozillaUseLazyMonitored
+    private lateinit var check: TestMozillaUseLazyMonitored
+
+    @BeforeEach
+    fun setUp() {
+        actualCheck = MozillaUseLazyMonitored(Config.empty)
+        check = TestMozillaUseLazyMonitored()
+    }
+
+    @Test
+    fun `GIVEN the hard-coded component group file paths THEN there are some to run the lint on`() {
+        assertFalse(MozillaUseLazyMonitored.COMPONENT_GROUP_FILES.isEmpty())
+    }
+
+    @Test
+    fun `GIVEN the hard-coded component group file paths THEN they all exist`() {
+        MozillaUseLazyMonitored.COMPONENT_GROUP_FILES.forEach { path ->
+            // The tests working directory is `<repo>/mozilla-detekt-rules` so we need to back out.
+            val modifiedPath = "../$path"
+
+            val errorMessage = "This lint rule hard-codes paths to files containing \"component groups\". " +
+                "One of these files - $path - no longer exists. Please update the check " +
+                "${MozillaUseLazyMonitored::class.java.simpleName} with the new path."
+            assertTrue(File(modifiedPath).exists(), errorMessage)
+        }
+    }
+
+    @Test
+    fun `GIVEN the hard-coded component group file paths THEN none of the files are ignored`() {
+        MozillaUseLazyMonitored.COMPONENT_GROUP_FILES.forEach {
+            assertTrue(actualCheck.filters?.isIgnored(Paths.get(it)) == false)
+        }
+    }
+
+    @Test
+    fun `GIVEN a snippet with lazyMonitored THEN there are no findings`() {
+        val text = getCodeInClass("val example by lazyMonitored { 4 }")
+        assertNoFindings(text)
+    }
+
+    @ParameterizedTest @ValueSource(strings = [
+        "val example by lazy { 4 }",
+        "val example    by    lazy    {    4    }",
+        "val example\nby\nlazy\n{\n4\n}"
+    ])
+    fun `GIVEN a snippet with by lazy THEN there is a finding`(innerText: String) {
+        val text = getCodeInClass(innerText)
+        assertOneFinding(text)
+    }
+
+    @ParameterizedTest @ValueSource(strings = [
+        "val example = lazy(::exampleFun)",
+        "val example   =   lazy   (  ::exampleFun)",
+        "val example\n=\nlazy\n(\n::exampleFun\n)"
+    ])
+    fun `GIVEN a snippet with = lazy THEN there is a finding`(innerText: String) {
+        val text = getCodeInClass(innerText)
+        assertOneFinding(text)
+    }
+
+    private fun assertNoFindings(text: String) {
+        val findings = check.lint(text)
+        assertEquals(0, findings.size)
+    }
+
+    private fun assertOneFinding(text: String) {
+        val findings = check.lint(text)
+        assertEquals(1, findings.size)
+        assertEquals(check.issue, findings[0].issue)
+    }
+}
+
+private fun getCodeInClass(text: String): String = """class Example() {
+    $text
+}"""
+
+class TestMozillaUseLazyMonitored : MozillaUseLazyMonitored(Config.empty) {
+    // If left on their own, the path filters will prevent us from running checks on raw strings.
+    // I'm not sure the best way to work around that so we just override the value.
+    override val filters: PathFilters? = null
+}