Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit f8b6186f authored by Peter Kalauskas's avatar Peter Kalauskas
Browse files

Update linter formatting, add new tests 

 - Enforce the bind/unbind service is only called on a @WorkerThread

 - Use heredoc for explanations

 - Add AccountManager.get() to NonInjectedService check

 - Test that sendBroadcast is okay inside BroadcastSender

 - Print only the relevant method name for error messages

 - Make briefDescriptions shorter because they are intended only as
   category headers. They are not intended to contain explanations

 - Follow go/error-messages-format-set-tone guidelines

 - Per lint guidelines, include the whole text output in the test

 - Drop "Detector" suffix from issue IDs

 - Call indented() on stub class files

 - Fix typos

Test: SystemUILintCheckerTest
Bug: 238923086
Change-Id: If74ca77717e5f8c2266fcecfdf59a83b05e06d22
parent 3e5c74ad

packages/SystemUI/checks/src/com/android/internal/systemui/lint/GetMainLooperViaContextDetector.kt→packages/SystemUI/checks/src/com/android/internal/systemui/lint/BindServiceOnMainThreadDetector.kt

+95 −0
Original line number Diff line number Diff line
@@ -16,6 +16,7 @@ 


package com.android.internal.systemui.lint



import com.android.SdkConstants.CLASS_CONTEXT

import com.android.tools.lint.detector.api.Category

import com.android.tools.lint.detector.api.Detector

import com.android.tools.lint.detector.api.Implementation
@@ -25,42 +26,70 @@ import com.android.tools.lint.detector.api.Scope 
import com.android.tools.lint.detector.api.Severity

import com.android.tools.lint.detector.api.SourceCodeScanner

import com.intellij.psi.PsiMethod

import com.intellij.psi.PsiModifierListOwner

import org.jetbrains.uast.UCallExpression

import org.jetbrains.uast.UClass

import org.jetbrains.uast.UMethod

import org.jetbrains.uast.getParentOfType



/**

 * Warns if {@code Context.bindService}, {@code Context.bindServiceAsUser}, or {@code

 * Context.unbindService} is not called on a {@code WorkerThread}

 */

@Suppress("UnstableApiUsage")

class GetMainLooperViaContextDetector : Detector(), SourceCodeScanner {

class BindServiceOnMainThreadDetector : Detector(), SourceCodeScanner {



    override fun getApplicableMethodNames(): List<String> {

        return listOf("getMainThreadHandler", "getMainLooper", "getMainExecutor")

        return listOf("bindService", "bindServiceAsUser", "unbindService")

    }



    private fun hasWorkerThreadAnnotation(

        context: JavaContext,

        annotated: PsiModifierListOwner?

    ): Boolean {

        return context.evaluator.getAnnotations(annotated, inHierarchy = true).any { uAnnotation ->

            uAnnotation.qualifiedName == "androidx.annotation.WorkerThread"

        }

    }



    override fun visitMethodCall(context: JavaContext, node: UCallExpression, method: PsiMethod) {

        if (context.evaluator.isMemberInSubClassOf(method, "android.content.Context")) {

        if (context.evaluator.isMemberInSubClassOf(method, CLASS_CONTEXT)) {

            if (

                !hasWorkerThreadAnnotation(context, node.getParentOfType(UMethod::class.java)) &&

                    !hasWorkerThreadAnnotation(context, node.getParentOfType(UClass::class.java))

            ) {

                context.report(

                    ISSUE,

                    method,

                    context.getNameLocation(node),

                    "Please inject a @Main Executor instead."

                    context.getLocation(node),

                    "This method should be annotated with `@WorkerThread` because " +

                        "it calls ${method.name}",

                )

            }

        }

    }



    companion object {

        @JvmField

        val ISSUE: Issue =

            Issue.create(

                        id = "GetMainLooperViaContextDetector",

                        briefDescription = "Please use idiomatic SystemUI executors, injecting " +

                                "them via Dagger.",

                        explanation = "Injecting the @Main Executor is preferred in order to make" +

                                "dependencies explicit and increase testability. It's much " +

                                "easier to pass a FakeExecutor on your test ctor than to " +

                                "deal with loopers in unit tests.",

                        category = Category.LINT,

                id = "BindServiceOnMainThread",

                briefDescription = "Service bound or unbound on main thread",

                explanation =

                    """

                    Binding and unbinding services are synchronous calls to `ActivityManager`. \

                    They usually take multiple milliseconds to complete. If called on the main \

                    thread, it will likely cause missed frames. To fix it, use a `@Background \

                    Executor` and annotate the calling method with `@WorkerThread`.

                    """,

                category = Category.PERFORMANCE,

                priority = 8,

                severity = Severity.WARNING,

                        implementation = Implementation(GetMainLooperViaContextDetector::class.java,

                                Scope.JAVA_FILE_SCOPE)

                implementation =

                    Implementation(

                        BindServiceOnMainThreadDetector::class.java,

                        Scope.JAVA_FILE_SCOPE

                    )

            )

    }

}

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

+13 −12
Original line number Diff line number Diff line
@@ -16,6 +16,7 @@ 


package com.android.internal.systemui.lint



import com.android.SdkConstants.CLASS_CONTEXT

import com.android.tools.lint.detector.api.Category

import com.android.tools.lint.detector.api.Detector

import com.android.tools.lint.detector.api.Implementation
@@ -48,14 +49,14 @@ class BroadcastSentViaContextDetector : Detector(), SourceCodeScanner { 
            return

        }



        val evaulator = context.evaluator

        if (evaulator.isMemberInSubClassOf(method, "android.content.Context")) {

        val evaluator = context.evaluator

        if (evaluator.isMemberInSubClassOf(method, CLASS_CONTEXT)) {

            context.report(

                    ISSUE,

                    method,

                    context.getNameLocation(node),

                    "Please don't call sendBroadcast/sendBroadcastAsUser directly on " +

                            "Context, use com.android.systemui.broadcast.BroadcastSender instead."

                    "`Context.${method.name}()` should be replaced with " +

                    "`BroadcastSender.${method.name}()`"

            )

        }

    }
@@ -65,14 +66,14 @@ class BroadcastSentViaContextDetector : Detector(), SourceCodeScanner { 
        val ISSUE: Issue =

            Issue.create(

                id = "BroadcastSentViaContext",

                briefDescription = "Broadcast sent via Context instead of BroadcastSender.",

                explanation =

                "Broadcast was sent via " +

                        "Context.sendBroadcast/Context.sendBroadcastAsUser. Please use " +

                        "BroadcastSender.sendBroadcast/BroadcastSender.sendBroadcastAsUser " +

                        "which will schedule dispatch of broadcasts on background thread. " +

                        "Sending broadcasts on main thread causes jank due to synchronous " +

                        "Binder calls.",

                briefDescription = "Broadcast sent via `Context` instead of `BroadcastSender`",

                // lint trims indents and converts \ to line continuations

                explanation = """

                        Broadcasts sent via `Context.sendBroadcast()` or \

                        `Context.sendBroadcastAsUser()` will block the main thread and may cause \

                        missed frames. Instead, use `BroadcastSender.sendBroadcast()` or \

                        `BroadcastSender.sendBroadcastAsUser()` which will schedule and dispatch \

                        broadcasts on a background worker thread.""",

                category = Category.PERFORMANCE,

                priority = 8,

                severity = Severity.WARNING,

packages/SystemUI/checks/src/com/android/internal/systemui/lint/BindServiceViaContextDetector.kt→packages/SystemUI/checks/src/com/android/internal/systemui/lint/NonInjectedMainThreadDetector.kt

+18 −16
Original line number Diff line number Diff line
@@ -16,6 +16,7 @@ 


package com.android.internal.systemui.lint



import com.android.SdkConstants.CLASS_CONTEXT

import com.android.tools.lint.detector.api.Category

import com.android.tools.lint.detector.api.Detector

import com.android.tools.lint.detector.api.Implementation
@@ -28,20 +29,19 @@ import com.intellij.psi.PsiMethod 
import org.jetbrains.uast.UCallExpression



@Suppress("UnstableApiUsage")

class BindServiceViaContextDetector : Detector(), SourceCodeScanner {

class NonInjectedMainThreadDetector : Detector(), SourceCodeScanner {



    override fun getApplicableMethodNames(): List<String> {

        return listOf("bindService", "bindServiceAsUser", "unbindService")

        return listOf("getMainThreadHandler", "getMainLooper", "getMainExecutor")

    }



    override fun visitMethodCall(context: JavaContext, node: UCallExpression, method: PsiMethod) {

        if (context.evaluator.isMemberInSubClassOf(method, "android.content.Context")) {

        if (context.evaluator.isMemberInSubClassOf(method, CLASS_CONTEXT)) {

            context.report(

                ISSUE,

                method,

                context.getNameLocation(node),

                    "Binding or unbinding services are synchronous calls, please make " +

                            "sure you're on a @Background Executor."

                "Replace with injected `@Main Executor`."

            )

        }

    }
@@ -50,18 +50,20 @@ class BindServiceViaContextDetector : Detector(), SourceCodeScanner { 
        @JvmField

        val ISSUE: Issue =

            Issue.create(

                id = "BindServiceViaContextDetector",

                briefDescription = "Service bound/unbound via Context, please make sure " +

                        "you're on a background thread.",

                id = "NonInjectedMainThread",

                briefDescription = "Main thread usage without dependency injection",

                explanation =

                "Binding or unbinding services are synchronous calls to ActivityManager, " +

                        "they usually take multiple milliseconds to complete and will make" +

                        "the caller drop frames. Make sure you're on a @Background Executor.",

                category = Category.PERFORMANCE,

                    """

                                Main thread should be injected using the `@Main Executor` instead \

                                of using the accessors in `Context`. This is to make the \

                                dependencies explicit and increase testability. It's much easier \

                                to pass a `FakeExecutor` on test constructors than it is to deal \

                                with loopers in unit tests.""",

                category = Category.LINT,

                priority = 8,

                severity = Severity.WARNING,

                implementation =

                Implementation(BindServiceViaContextDetector::class.java, Scope.JAVA_FILE_SCOPE)

                    Implementation(NonInjectedMainThreadDetector::class.java, Scope.JAVA_FILE_SCOPE)

            )

    }

}

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

+23 −11
Original line number Diff line number Diff line
@@ -16,6 +16,7 @@ 


package com.android.internal.systemui.lint



import com.android.SdkConstants.CLASS_CONTEXT

import com.android.tools.lint.detector.api.Category

import com.android.tools.lint.detector.api.Detector

import com.android.tools.lint.detector.api.Implementation
@@ -32,7 +33,7 @@ import org.jetbrains.uast.UCallExpression 
class NonInjectedServiceDetector : Detector(), SourceCodeScanner {



    override fun getApplicableMethodNames(): List<String> {

        return listOf("getSystemService")

        return listOf("getSystemService", "get")

    }



    override fun visitMethodCall(context: JavaContext, node: UCallExpression, method: PsiMethod) {
@@ -40,14 +41,25 @@ class NonInjectedServiceDetector : Detector(), SourceCodeScanner { 
        if (

            !evaluator.isStatic(method) &&

                method.name == "getSystemService" &&

                method.containingClass?.qualifiedName == "android.content.Context"

                method.containingClass?.qualifiedName == CLASS_CONTEXT

        ) {

            context.report(

                ISSUE,

                method,

                context.getNameLocation(node),

                "Use @Inject to get the handle to a system-level services instead of using " +

                    "Context.getSystemService()"

                "Use `@Inject` to get system-level service handles instead of " +

                    "`Context.getSystemService()`"

            )

        } else if (

            evaluator.isStatic(method) &&

                method.name == "get" &&

                method.containingClass?.qualifiedName == "android.accounts.AccountManager"

        ) {

            context.report(

                ISSUE,

                method,

                context.getNameLocation(node),

                "Replace `AccountManager.get()` with an injected instance of `AccountManager`"

            )

        }

    }
@@ -57,14 +69,14 @@ class NonInjectedServiceDetector : Detector(), SourceCodeScanner { 
        val ISSUE: Issue =

            Issue.create(

                id = "NonInjectedService",

                briefDescription =

                    "System-level services should be retrieved using " +

                        "@Inject instead of Context.getSystemService().",

                briefDescription = "System service not injected",

                explanation =

                    "Context.getSystemService() should be avoided because it makes testing " +

                        "difficult. Instead, use an injected service. For example, " +

                        "instead of calling Context.getSystemService(UserManager.class), " +

                        "use @Inject and add UserManager to the constructor",

                    """

                    `Context.getSystemService()` should be avoided because it makes testing \

                    difficult. Instead, use an injected service. For example, instead of calling \

                    `Context.getSystemService(UserManager.class)` in a class, annotate the class' \

                    constructor with `@Inject` and add `UserManager` to the parameters.

                    """,

                category = Category.CORRECTNESS,

                priority = 8,

                severity = Severity.WARNING,

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

+13 −10
Original line number Diff line number Diff line
@@ -16,6 +16,7 @@ 


package com.android.internal.systemui.lint



import com.android.SdkConstants.CLASS_CONTEXT

import com.android.tools.lint.detector.api.Category

import com.android.tools.lint.detector.api.Detector

import com.android.tools.lint.detector.api.Implementation
@@ -35,12 +36,12 @@ class RegisterReceiverViaContextDetector : Detector(), SourceCodeScanner { 
    }



    override fun visitMethodCall(context: JavaContext, node: UCallExpression, method: PsiMethod) {

        if (context.evaluator.isMemberInSubClassOf(method, "android.content.Context")) {

        if (context.evaluator.isMemberInSubClassOf(method, CLASS_CONTEXT)) {

            context.report(

                    ISSUE,

                    method,

                    context.getNameLocation(node),

                    "BroadcastReceivers should be registered via BroadcastDispatcher."

                    "Register `BroadcastReceiver` using `BroadcastDispatcher` instead of `Context`"

            )

        }

    }
@@ -49,14 +50,16 @@ class RegisterReceiverViaContextDetector : Detector(), SourceCodeScanner { 
        @JvmField

        val ISSUE: Issue =

            Issue.create(

                    id = "RegisterReceiverViaContextDetector",

                    briefDescription = "Broadcast registrations via Context are blocking " +

                            "calls. Please use BroadcastDispatcher.",

                    explanation =

                    "Context#registerReceiver is a blocking call to the system server, " +

                            "making it very likely that you'll drop a frame. Please use " +

                            "BroadcastDispatcher instead (or move this call to a " +

                            "@Background Executor.)",

                    id = "RegisterReceiverViaContext",

                    briefDescription = "Blocking broadcast registration",

                    // lint trims indents and converts \ to line continuations

                    explanation = """

                            `Context.registerReceiver()` is a blocking call to the system server, \

                            making it very likely that you'll drop a frame. Please use \

                            `BroadcastDispatcher` instead, which registers the receiver on a \

                             background thread. `BroadcastDispatcher` also improves our visibility \

                             into ANRs.""",

                            moreInfo = "go/identifying-broadcast-threads",

                    category = Category.PERFORMANCE,

                    priority = 8,

                    severity = Severity.WARNING,