diff --git a/config/detekt.yml b/config/detekt.yml index f195fc4df..8a28b5c78 100644 --- a/config/detekt.yml +++ b/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 diff --git a/mozilla-detekt-rules/build.gradle b/mozilla-detekt-rules/build.gradle index fd3f07c1f..f663472f3 100644 --- a/mozilla-detekt-rules/build.gradle +++ b/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 diff --git a/mozilla-detekt-rules/src/main/java/org/mozilla/fenix/detektrules/CustomRulesetProvider.kt b/mozilla-detekt-rules/src/main/java/org/mozilla/fenix/detektrules/CustomRulesetProvider.kt index 93ac40c0b..e7fec788d 100644 --- a/mozilla-detekt-rules/src/main/java/org/mozilla/fenix/detektrules/CustomRulesetProvider.kt +++ b/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) ) ) } diff --git a/mozilla-detekt-rules/src/main/java/org/mozilla/fenix/detektrules/perf/MozillaUseLazyMonitored.kt b/mozilla-detekt-rules/src/main/java/org/mozilla/fenix/detektrules/perf/MozillaUseLazyMonitored.kt new file mode 100644 index 000000000..3a9430af3 --- /dev/null +++ b/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" } + } +} diff --git a/mozilla-detekt-rules/src/test/java/org/mozilla/fenix/detektrules/perf/MozillaUseLazyMonitoredTest.kt b/mozilla-detekt-rules/src/test/java/org/mozilla/fenix/detektrules/perf/MozillaUseLazyMonitoredTest.kt new file mode 100644 index 000000000..8027403d3 --- /dev/null +++ b/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 `/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 +}