Merge "SysUI linter: Clean Architecture Dependency Rule." into tm-qpr-dev am: 2f12b73182
Original change: https://googleplex-android-review.googlesource.com/c/platform/frameworks/base/+/20989721 Change-Id: Iff5d3cdd9ee6f7715de60718e3cac5e3dd70f46a Signed-off-by: Automerger Merge Worker <android-build-automerger-merge-worker@system.gserviceaccount.com>
This commit is contained in:
Ale Nijamkin
@@ -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,
|
||||
),
|
||||
)
|
||||
}
|
||||
}
|
||||
@@ -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(
|
||||
vendorName = "Android",
|
||||
feedbackUrl = "http://b/issues/new?component=78010",
|
||||
contact = "jernej@google.com"
|
||||
)
|
||||
Vendor(
|
||||
vendorName = "Android",
|
||||
feedbackUrl = "http://b/issues/new?component=78010",
|
||||
contact = "jernej@google.com"
|
||||
)
|
||||
}
|
||||
|
||||
@@ -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,
|
||||
)
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user