Merge "SysUI linter: Clean Architecture Dependency Rule." into tm-qpr-dev

packages/SystemUI/checks/src/com/android/internal/systemui/lint/CleanArchitectureDependencyViolationDetector.kt

@@ -0,0 +1,150 @@
+/*
+ * Copyright (C) 2023 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+
+package com.android.internal.systemui.lint
+
+import com.android.tools.lint.client.api.UElementHandler
+import com.android.tools.lint.detector.api.Category
+import com.android.tools.lint.detector.api.Detector
+import com.android.tools.lint.detector.api.Implementation
+import com.android.tools.lint.detector.api.Issue
+import com.android.tools.lint.detector.api.JavaContext
+import com.android.tools.lint.detector.api.Scope
+import com.android.tools.lint.detector.api.Severity
+import org.jetbrains.uast.UElement
+import org.jetbrains.uast.UFile
+import org.jetbrains.uast.UImportStatement
+
+/**
+ * Detects violations of the Dependency Rule of Clean Architecture.
+ *
+ * The rule states that code in each layer may only depend on code in the same layer or the layer
+ * directly "beneath" that layer in the layer diagram.
+ *
+ * In System UI, we have three layers; from top to bottom, they are: ui, domain, and data. As a
+ * convention, was used packages with those names to place code in the appropriate layer. We also
+ * make an exception and allow for shared models to live under a separate package named "shared" to
+ * avoid code duplication.
+ *
+ * For more information, please see go/sysui-arch.
+ */
+@Suppress("UnstableApiUsage")
+class CleanArchitectureDependencyViolationDetector : Detector(), Detector.UastScanner {
+    override fun getApplicableUastTypes(): List<Class<out UElement>> {
+        return listOf(UFile::class.java)
+    }
+
+    override fun createUastHandler(context: JavaContext): UElementHandler {
+        return object : UElementHandler() {
+            override fun visitFile(node: UFile) {
+                // Check which Clean Architecture layer this file belongs to:
+                matchingLayer(node.packageName)?.let { layer ->
+                    // The file matches with a Clean Architecture layer. Let's check all of its
+                    // imports.
+                    node.imports.forEach { importStatement ->
+                        visitImportStatement(context, layer, importStatement)
+                    }
+                }
+            }
+        }
+    }
+
+    private fun visitImportStatement(
+        context: JavaContext,
+        layer: Layer,
+        importStatement: UImportStatement,
+    ) {
+        val importText = importStatement.importReference?.asSourceString() ?: return
+        val importedLayer = matchingLayer(importText) ?: return
+
+        // Now check whether the layer of the file may depend on the layer of the import.
+        if (!layer.mayDependOn(importedLayer)) {
+            context.report(
+                issue = ISSUE,
+                scope = importStatement,
+                location = context.getLocation(importStatement),
+                message =
+                    "The ${layer.packageNamePart} layer may not depend on" +
+                        " the ${importedLayer.packageNamePart} layer.",
+            )
+        }
+    }
+
+    private fun matchingLayer(packageName: String): Layer? {
+        val packageNameParts = packageName.split(".").toSet()
+        return Layer.values()
+            .filter { layer -> packageNameParts.contains(layer.packageNamePart) }
+            .takeIf { it.size == 1 }
+            ?.first()
+    }
+
+    private enum class Layer(
+        val packageNamePart: String,
+        val canDependOn: Set<Layer>,
+    ) {
+        SHARED(
+            packageNamePart = "shared",
+            canDependOn = emptySet(), // The shared layer may not depend on any other layer.
+        ),
+        DATA(
+            packageNamePart = "data",
+            canDependOn = setOf(SHARED),
+        ),
+        DOMAIN(
+            packageNamePart = "domain",
+            canDependOn = setOf(SHARED, DATA),
+        ),
+        UI(
+            packageNamePart = "ui",
+            canDependOn = setOf(DOMAIN, SHARED),
+        ),
+        ;
+
+        fun mayDependOn(otherLayer: Layer): Boolean {
+            return this == otherLayer || canDependOn.contains(otherLayer)
+        }
+    }
+
+    companion object {
+        @JvmStatic
+        val ISSUE =
+            Issue.create(
+                id = "CleanArchitectureDependencyViolation",
+                briefDescription = "Violation of the Clean Architecture Dependency Rule.",
+                explanation =
+                    """
+                    Following the \"Dependency Rule\" from Clean Architecture, every layer of code \
+                    can only depend code in its own layer or code in the layer directly \
+                    \"beneath\" it. Therefore, the UI layer can only depend on the" Domain layer \
+                    and the Domain layer can only depend on the Data layer. We" do make an \
+                    exception to allow shared models to exist and be shared across layers by \
+                    placing them under shared/model, which should be done with care. For more \
+                    information about Clean Architecture in System UI, please see go/sysui-arch. \
+                    NOTE: if your code is not using Clean Architecture, please feel free to ignore \
+                    this warning.
+                """,
+                category = Category.CORRECTNESS,
+                priority = 8,
+                severity = Severity.WARNING,
+                implementation =
+                    Implementation(
+                        CleanArchitectureDependencyViolationDetector::class.java,
+                        Scope.JAVA_FILE_SCOPE,
+                    ),
+            )
+    }
+}

packages/SystemUI/checks/src/com/android/internal/systemui/lint/SystemUIIssueRegistry.kt

@@ -27,9 +27,11 @@ import com.google.auto.service.AutoService
 class SystemUIIssueRegistry : IssueRegistry() {
 
     override val issues: List<Issue>
-        get() = listOf(
+        get() =
+            listOf(
                 BindServiceOnMainThreadDetector.ISSUE,
                 BroadcastSentViaContextDetector.ISSUE,
+                CleanArchitectureDependencyViolationDetector.ISSUE,
                 SlowUserQueryDetector.ISSUE_SLOW_USER_ID_QUERY,
                 SlowUserQueryDetector.ISSUE_SLOW_USER_INFO_QUERY,
                 NonInjectedMainThreadDetector.ISSUE,
@@ -37,7 +39,7 @@ class SystemUIIssueRegistry : IssueRegistry() {
                 SoftwareBitmapDetector.ISSUE,
                 NonInjectedServiceDetector.ISSUE,
                 StaticSettingsProviderDetector.ISSUE
-        )
+            )
 
     override val api: Int
         get() = CURRENT_API
@@ -45,9 +47,9 @@ class SystemUIIssueRegistry : IssueRegistry() {
         get() = 8
 
     override val vendor: Vendor =
-            Vendor(
+        Vendor(
-                    vendorName = "Android",
+            vendorName = "Android",
-                    feedbackUrl = "http://b/issues/new?component=78010",
+            feedbackUrl = "http://b/issues/new?component=78010",
-                    contact = "jernej@google.com"
+            contact = "jernej@google.com"
-            )
+        )
 }

packages/SystemUI/checks/tests/com/android/internal/systemui/lint/CleanArchitectureDependencyViolationDetectorTest.kt

@@ -0,0 +1,296 @@
+/*
+ * Copyright (C) 2023 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *            http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+
+package com.android.internal.systemui.lint
+
+import com.android.tools.lint.checks.infrastructure.TestFiles
+import com.android.tools.lint.checks.infrastructure.TestMode
+import com.android.tools.lint.detector.api.Detector
+import com.android.tools.lint.detector.api.Issue
+import org.junit.Ignore
+import org.junit.Test
+
+@Suppress("UnstableApiUsage")
+@Ignore("b/254533331")
+class CleanArchitectureDependencyViolationDetectorTest : SystemUILintDetectorTest() {
+    override fun getDetector(): Detector {
+        return CleanArchitectureDependencyViolationDetector()
+    }
+
+    override fun getIssues(): List<Issue> {
+        return listOf(
+            CleanArchitectureDependencyViolationDetector.ISSUE,
+        )
+    }
+
+    @Test
+    fun `No violations`() {
+        lint()
+            .files(
+                *LEGITIMATE_FILES,
+            )
+            .issues(
+                CleanArchitectureDependencyViolationDetector.ISSUE,
+            )
+            .run()
+            .expectWarningCount(0)
+    }
+
+    @Test
+    fun `Violation - domain depends on ui`() {
+        lint()
+            .files(
+                *LEGITIMATE_FILES,
+                TestFiles.kotlin(
+                    """
+                        package test.domain.interactor
+
+                        import test.ui.viewmodel.ViewModel
+
+                        class BadClass(
+                            private val viewModel: ViewModel,
+                        )
+                    """.trimIndent()
+                )
+            )
+            .issues(
+                CleanArchitectureDependencyViolationDetector.ISSUE,
+            )
+            .testModes(TestMode.DEFAULT)
+            .run()
+            .expectWarningCount(1)
+            .expect(
+                expectedText =
+                    """
+                    src/test/domain/interactor/BadClass.kt:3: Warning: The domain layer may not depend on the ui layer. [CleanArchitectureDependencyViolation]
+                    import test.ui.viewmodel.ViewModel
+                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+                    0 errors, 1 warnings
+                """,
+            )
+    }
+
+    @Test
+    fun `Violation - ui depends on data`() {
+        lint()
+            .files(
+                *LEGITIMATE_FILES,
+                TestFiles.kotlin(
+                    """
+                        package test.ui.viewmodel
+
+                        import test.data.repository.Repository
+
+                        class BadClass(
+                            private val repository: Repository,
+                        )
+                    """.trimIndent()
+                )
+            )
+            .issues(
+                CleanArchitectureDependencyViolationDetector.ISSUE,
+            )
+            .testModes(TestMode.DEFAULT)
+            .run()
+            .expectWarningCount(1)
+            .expect(
+                expectedText =
+                    """
+                    src/test/ui/viewmodel/BadClass.kt:3: Warning: The ui layer may not depend on the data layer. [CleanArchitectureDependencyViolation]
+                    import test.data.repository.Repository
+                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+                    0 errors, 1 warnings
+                """,
+            )
+    }
+
+    @Test
+    fun `Violation - shared depends on all other layers`() {
+        lint()
+            .files(
+                *LEGITIMATE_FILES,
+                TestFiles.kotlin(
+                    """
+                        package test.shared.model
+
+                        import test.data.repository.Repository
+                        import test.domain.interactor.Interactor
+                        import test.ui.viewmodel.ViewModel
+
+                        class BadClass(
+                            private val repository: Repository,
+                            private val interactor: Interactor,
+                            private val viewmodel: ViewModel,
+                        )
+                    """.trimIndent()
+                )
+            )
+            .issues(
+                CleanArchitectureDependencyViolationDetector.ISSUE,
+            )
+            .testModes(TestMode.DEFAULT)
+            .run()
+            .expectWarningCount(3)
+            .expect(
+                expectedText =
+                    """
+                    src/test/shared/model/BadClass.kt:3: Warning: The shared layer may not depend on the data layer. [CleanArchitectureDependencyViolation]
+                    import test.data.repository.Repository
+                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+                    src/test/shared/model/BadClass.kt:4: Warning: The shared layer may not depend on the domain layer. [CleanArchitectureDependencyViolation]
+                    import test.domain.interactor.Interactor
+                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+                    src/test/shared/model/BadClass.kt:5: Warning: The shared layer may not depend on the ui layer. [CleanArchitectureDependencyViolation]
+                    import test.ui.viewmodel.ViewModel
+                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+                    0 errors, 3 warnings
+                """,
+            )
+    }
+
+    @Test
+    fun `Violation - data depends on domain`() {
+        lint()
+            .files(
+                *LEGITIMATE_FILES,
+                TestFiles.kotlin(
+                    """
+                        package test.data.repository
+
+                        import test.domain.interactor.Interactor
+
+                        class BadClass(
+                            private val interactor: Interactor,
+                        )
+                    """.trimIndent()
+                )
+            )
+            .issues(
+                CleanArchitectureDependencyViolationDetector.ISSUE,
+            )
+            .testModes(TestMode.DEFAULT)
+            .run()
+            .expectWarningCount(1)
+            .expect(
+                expectedText =
+                    """
+                    src/test/data/repository/BadClass.kt:3: Warning: The data layer may not depend on the domain layer. [CleanArchitectureDependencyViolation]
+                    import test.domain.interactor.Interactor
+                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+                    0 errors, 1 warnings
+                """,
+            )
+    }
+
+    companion object {
+        private val MODEL_FILE =
+            TestFiles.kotlin(
+                """
+                    package test.shared.model
+
+                    import test.some.other.thing.SomeOtherThing
+
+                    data class Model(
+                        private val name: String,
+                    )
+                """.trimIndent()
+            )
+        private val REPOSITORY_FILE =
+            TestFiles.kotlin(
+                """
+                    package test.data.repository
+
+                    import test.shared.model.Model
+                    import test.some.other.thing.SomeOtherThing
+
+                    class Repository {
+                        private val models = listOf(
+                            Model("one"),
+                            Model("two"),
+                            Model("three"),
+                        )
+
+                        fun getModels(): List<Model> {
+                            return models
+                        }
+                    }
+                """.trimIndent()
+            )
+        private val INTERACTOR_FILE =
+            TestFiles.kotlin(
+                """
+                    package test.domain.interactor
+
+                    import test.data.repository.Repository
+                    import test.shared.model.Model
+
+                    class Interactor(
+                        private val repository: Repository,
+                    ) {
+                        fun getModels(): List<Model> {
+                            return repository.getModels()
+                        }
+                    }
+                """.trimIndent()
+            )
+        private val VIEW_MODEL_FILE =
+            TestFiles.kotlin(
+                """
+                    package test.ui.viewmodel
+
+                    import test.domain.interactor.Interactor
+                    import test.some.other.thing.SomeOtherThing
+
+                    class ViewModel(
+                        private val interactor: Interactor,
+                    ) {
+                        fun getNames(): List<String> {
+                            return interactor.getModels().map { model -> model.name }
+                        }
+                    }
+                """.trimIndent()
+            )
+        private val NON_CLEAN_ARCHITECTURE_FILE =
+            TestFiles.kotlin(
+                """
+                    package test.some.other.thing
+
+                    import test.data.repository.Repository
+                    import test.domain.interactor.Interactor
+                    import test.ui.viewmodel.ViewModel
+
+                    class SomeOtherThing {
+                        init {
+                            val viewModel = ViewModel(
+                                interactor = Interactor(
+                                    repository = Repository(),
+                                ),
+                            )
+                        }
+                    }
+                """.trimIndent()
+            )
+        private val LEGITIMATE_FILES =
+            arrayOf(
+                MODEL_FILE,
+                REPOSITORY_FILE,
+                INTERACTOR_FILE,
+                VIEW_MODEL_FILE,
+                NON_CLEAN_ARCHITECTURE_FILE,
+            )
+    }
+}