236262744: Don't compute platform annotations from merge context

The parsing environment is not available when lint is only filtering and
merging module analysis results. The permission error detector was
looking up the platform annotations when analyzing the merged manifest,
which would throw exceptions (flakily, because it can normally access
the cached platform annotations from the module analysis phase, but this
won't run if the module analysis results have been cached).

However, it didn't actually *need* to compute the platform annotations,
because while it does look them up, it doesn't actually use them when
running on the merged manifest (it computes the nearest match, but then
discards the result if it's looking at a merged manifest).

This is deliberate; when we're analyzing the merged manifest, we can
(and usually do) come across permission declarations from libraries, and
we don't want to flag issues in these; there's nothing a developer can
do here.

We actually only use the merged manifest to check the
CustomPermissionTypo issue type. Therefore, the fix here is to use lint's built in `visitElement` method for all other checks, and use `checkMergedProject` ONLY for CustomPermissionTypo.

Test: Existing (though the unit test now nulls out the cached
permissions between phases, which means the test would crash as it
did flakily earlier if we were trying to compute platform
annotations from the merge phase.)

Fixes: 236262744
Change-Id: I505dffc7aee1d267362fdf72a70f5bf2217f9006
diff --git a/lint/libs/lint-checks/src/main/java/com/android/tools/lint/checks/PermissionErrorDetector.kt b/lint/libs/lint-checks/src/main/java/com/android/tools/lint/checks/PermissionErrorDetector.kt
index 49e0ef2..c7bf3d5 100644
--- a/lint/libs/lint-checks/src/main/java/com/android/tools/lint/checks/PermissionErrorDetector.kt
+++ b/lint/libs/lint-checks/src/main/java/com/android/tools/lint/checks/PermissionErrorDetector.kt
@@ -43,55 +43,76 @@
 import com.android.tools.lint.detector.api.editDistance
 import com.android.utils.XmlUtils.getFirstSubTag
 import com.android.utils.XmlUtils.getNextTag
+import com.google.common.annotations.VisibleForTesting
 import org.w3c.dom.Attr
 import org.w3c.dom.Document
 import org.w3c.dom.Element
 import java.util.Arrays
 
 /**
- * looks for obvious errors in the guarding of components with a
- * permission via the android:permission attribute
+ * looks for errors related to:
+ * Declaring permissions in a `<permission ... />` element
+ * Declaring permission usage in a `<uses-permission ... />` element
+ * Declaring components restricted by permissions in an `android:permission="..."` attribute
  */
 class PermissionErrorDetector : Detector(), XmlScanner {
+    override fun getApplicableElements(): Collection<String>? {
+        return listOf(
+            TAG_PERMISSION,
+            TAG_USES_PERMISSION,
+            TAG_APPLICATION,
+            TAG_ACTIVITY,
+            TAG_ACTIVITY_ALIAS,
+            TAG_RECEIVER,
+            TAG_SERVICE,
+            TAG_PROVIDER
+        )
+    }
+
+    override fun visitElement(context: XmlContext, element: Element) {
+        when (element.tagName) {
+            TAG_PERMISSION -> element.getAttributeNodeNS(ANDROID_URI, ATTR_NAME)?.let { reportPermissionDefinitionIncidents(context, it) }
+            TAG_USES_PERMISSION -> element.getAttributeNodeNS(ANDROID_URI, ATTR_NAME)?.let { reportPermissionUsageIncidents(context, it) }
+            else -> element.getAttributeNodeNS(ANDROID_URI, ATTR_PERMISSION)?.let { reportPermissionUsageIncidents(context, it) }
+        }
+    }
+
     override fun checkMergedProject(context: Context) {
         val root = context.mainProject.mergedManifest?.documentElement ?: return
-        checkDocument(context, root, true)
+        walkDocument(context, root)
     }
 
-    override fun visitDocument(context: XmlContext, document: Document) {
-        val root = document.documentElement ?: return
-        checkDocument(context, root, false)
-    }
-
-    private fun checkDocument(context: Context, root: Element, isMergedManifest: Boolean) {
+    /**
+     * Collect *all* custom permissions (and their usages)
+     * across manifests.  Then report on any typos.
+     * Many custom permissions may be included
+     * from libraries, etc., and we want to catch those typos as well
+     * as typos on custom permissions defined in the same manifest.
+     */
+    private fun walkDocument(context: Context, root: Element) {
         var customPermissions: MutableList<String>? = null
-        var customPermissionUsages: MutableList<Attr>? = null
-
-        fun reportOrAddCustomPermissions(attr: Attr) {
-            val evaluateCustomPermission = reportPermissionDefinitionIncidents(context, attr, isMergedManifest)
-            if (evaluateCustomPermission)
-                (customPermissions ?: mutableListOf<String>().also { customPermissions = it })
-                    .add(attr.value)
+        fun addCustomPermission(attr: Attr) {
+            (customPermissions ?: mutableListOf<String>().also { customPermissions = it })
+                .add(attr.value)
         }
 
-        fun reportOrAddPermissionUsages(attr: Attr) {
-            val evaluateCustomPermissionUsage = reportPermissionUsageIncidents(context, attr, isMergedManifest)
-            if (evaluateCustomPermissionUsage)
-                (customPermissionUsages ?: mutableListOf<Attr>().also { customPermissionUsages = it })
-                    .add(attr)
+        var customPermissionUsages: MutableList<Attr>? = null
+        fun addCustomPermissionUsage(attr: Attr) {
+            (customPermissionUsages ?: mutableListOf<Attr>().also { customPermissionUsages = it })
+                .add(attr)
         }
 
         var topLevel = getFirstSubTag(root)
         while (topLevel != null) {
             when (topLevel.tagName) {
                 TAG_PERMISSION -> {
-                    topLevel.getAttributeNodeNS(ANDROID_URI, ATTR_NAME)?.let(::reportOrAddCustomPermissions)
+                    topLevel.getAttributeNodeNS(ANDROID_URI, ATTR_NAME)?.let(::addCustomPermission)
                 }
                 TAG_USES_PERMISSION -> {
-                    topLevel.getAttributeNodeNS(ANDROID_URI, ATTR_NAME)?.let(::reportOrAddPermissionUsages)
+                    topLevel.getAttributeNodeNS(ANDROID_URI, ATTR_NAME)?.let(::addCustomPermissionUsage)
                 }
                 TAG_APPLICATION -> {
-                    topLevel.getAttributeNodeNS(ANDROID_URI, ATTR_PERMISSION)?.let(::reportOrAddPermissionUsages)
+                    topLevel.getAttributeNodeNS(ANDROID_URI, ATTR_PERMISSION)?.let(::addCustomPermissionUsage)
 
                     var componentLevel = getFirstSubTag(topLevel)
                     while (componentLevel != null) {
@@ -102,7 +123,7 @@
                             TAG_RECEIVER,
                             TAG_SERVICE,
                             TAG_PROVIDER -> {
-                                componentLevel.getAttributeNodeNS(ANDROID_URI, ATTR_PERMISSION)?.let(::reportOrAddPermissionUsages)
+                                componentLevel.getAttributeNodeNS(ANDROID_URI, ATTR_PERMISSION)?.let(::addCustomPermissionUsage)
                             }
                         }
                         componentLevel = getNextTag(componentLevel)
@@ -132,18 +153,17 @@
 
     /**
      * Report incidents related to permission definitions that are
-     * NOT related to custom permission typos. Returns a boolean
-     * representing whether this attribute should be evaluated for a
-     * custom permission typo.
+     * NOT related to custom permission typos.
      */
-    private fun reportPermissionDefinitionIncidents(context: Context, attr: Attr, isMergedManifest: Boolean): Boolean {
+    private fun reportPermissionDefinitionIncidents(context: Context, attr: Attr) {
         val packageName =
             (context.project.buildVariant?.`package` ?: attr.ownerDocument.documentElement?.getAttribute(ATTR_PACKAGE))
                 .orEmpty()
 
         val platformPermissions = getPlatformPermissions(context.project)
+
         if (isStandardPermission(attr.value, platformPermissions)) {
-            if (!isMergedManifest) context.report(
+            context.report(
                 Incident(
                     RESERVED_SYSTEM_PERMISSION,
                     attr.ownerElement,
@@ -151,11 +171,10 @@
                     "`${attr.value}` is a reserved permission"
                 )
             )
-            if (context.isEnabled(RESERVED_SYSTEM_PERMISSION)) return false
         }
 
         if (!followsCustomPermissionNamingConvention(packageName, attr.value)) {
-            if (!isMergedManifest) context.report(
+            context.report(
                 Incident(
                     PERMISSION_NAMING_CONVENTION,
                     attr.ownerElement,
@@ -163,21 +182,16 @@
                     "`${attr.value} does not follow recommended naming convention`"
                 )
             )
-            if (context.isEnabled(PERMISSION_NAMING_CONVENTION)) return false
         }
-
-        return isMergedManifest
     }
 
     /**
      * Report incidents related to permission usages that are
-     * NOT related to custom permission typos. Returns a boolean
-     * representing whether this attribute should be evaluated for a
-     * custom permission typo.
+     * NOT related to custom permission typos.
      */
-    private fun reportPermissionUsageIncidents(context: Context, attr: Attr, isMergedManifest: Boolean): Boolean {
+    private fun reportPermissionUsageIncidents(context: Context, attr: Attr) {
         if (KNOWN_PERMISSION_ERROR_VALUES.any { it.equals(attr.value, ignoreCase = true) }) {
-            if (!isMergedManifest) context.report(
+            context.report(
                 Incident(
                     KNOWN_PERMISSION_ERROR,
                     attr.ownerElement,
@@ -185,25 +199,19 @@
                     "`${attr.value}` is not a valid permission value"
                 )
             )
-            // signal that the attribute would have already been reported
-            if (context.isEnabled(KNOWN_PERMISSION_ERROR)) return false
         }
 
-        findAlmostPlatformPermission(context.project, attr.value)?.let {
-            if (!isMergedManifest) context.report(
+        findAlmostPlatformPermission(context.project, attr.value)?.let { almost ->
+            context.report(
                 Incident(
                     SYSTEM_PERMISSION_TYPO,
                     attr.ownerElement,
                     context.getLocation(attr, LocationType.VALUE),
-                    "Did you mean `$it`?",
-                    fix().replace().text(attr.value).with(it).build()
+                    "Did you mean `$almost`?",
+                    fix().replace().text(attr.value).with(almost).build()
                 )
             )
-            // signal that the attribute would have already been reported
-            if (context.isEnabled(SYSTEM_PERMISSION_TYPO)) return false
         }
-
-        return isMergedManifest
     }
 
     companion object {
@@ -451,6 +459,11 @@
         private var platformTarget: String? = null
         private var platformPermissions: Array<String>? = null
 
+        @VisibleForTesting
+        fun clearPlatformPermissions() {
+            platformPermissions = null
+        }
+
         /**
          * Returns the platform permissions: those permissions users are
          * allowed to access from their applications
diff --git a/lint/libs/lint-tests/src/test/java/com/android/tools/lint/checks/PermissionErrorDetectorTest.kt b/lint/libs/lint-tests/src/test/java/com/android/tools/lint/checks/PermissionErrorDetectorTest.kt
index 713ad84..80d42c4 100644
--- a/lint/libs/lint-tests/src/test/java/com/android/tools/lint/checks/PermissionErrorDetectorTest.kt
+++ b/lint/libs/lint-tests/src/test/java/com/android/tools/lint/checks/PermissionErrorDetectorTest.kt
@@ -26,7 +26,12 @@
 import com.android.tools.lint.checks.PermissionErrorDetector.Companion.permissionToPrefixAndSuffix
 import com.android.tools.lint.checks.SystemPermissionsDetector.SYSTEM_PERMISSIONS
 import com.android.tools.lint.checks.infrastructure.ProjectDescription
+import com.android.tools.lint.checks.infrastructure.TestLintTask
+import com.android.tools.lint.client.api.LintDriver
+import com.android.tools.lint.client.api.LintListener
+import com.android.tools.lint.detector.api.Context
 import com.android.tools.lint.detector.api.Detector
+import com.android.tools.lint.detector.api.Project
 import com.google.common.io.Files
 import com.intellij.openapi.Disposable
 import com.intellij.openapi.util.Disposer
@@ -36,6 +41,23 @@
 class PermissionErrorDetectorTest : AbstractCheckTest() {
     override fun getDetector(): Detector = PermissionErrorDetector()
 
+    override fun lint(): TestLintTask {
+        return super.lint()
+            // When switching to merging, clear out the platform cache (to simulate running lint where the analysis
+            // tasks have been cached so have not run in the current process. It would be better if the lint testing
+            // infrastructure did this automatically (e.g. loading everything into separate class loaders to enforce
+            // true separation) but that's hard to set up now.
+            .listener(object : LintListener {
+                private var mode: LintDriver.DriverMode? = null
+                override fun update(driver: LintDriver, type: LintListener.EventType, project: Project?, context: Context?) {
+                    if (driver.mode != mode) {
+                        PermissionErrorDetector.clearPlatformPermissions()
+                    }
+                    mode = driver.mode
+                }
+            })
+    }
+
     @Test
     fun testDocumentationExamplePermissionNamingConvention() {
         lint().files(
@@ -578,6 +600,42 @@
             )
     }
 
+    fun testDemonstrateMultipleIssuesAtSameLocation() {
+        val main = project(
+            manifest(
+                """
+                <manifest xmlns:android="http://schemas.android.com/apk/res/android"
+                  xmlns:tools="http://schemas.android.com/tools"
+                  package="com.example.helloworld">
+                  <permission android:name="android.permission.BIND_APPWIDGET" />
+                  <application>
+                    <service android:permission="android.permission.BINDAPPWIDGET" />
+                  </application>
+                </manifest>
+                """
+            ).indented()
+        )
+        lint().projects(main)
+            .run()
+            .expect(
+                """
+                AndroidManifest.xml:6: Warning: Did you mean android.permission.BIND_APPWIDGET? [CustomPermissionTypo]
+                    <service android:permission="android.permission.BINDAPPWIDGET" />
+                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+                AndroidManifest.xml:4: Warning: android.permission.BIND_APPWIDGET does not follow recommended naming convention [PermissionNamingConvention]
+                  <permission android:name="android.permission.BIND_APPWIDGET" />
+                                            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+                AndroidManifest.xml:4: Error: android.permission.BIND_APPWIDGET is a reserved permission [ReservedSystemPermission]
+                  <permission android:name="android.permission.BIND_APPWIDGET" />
+                                            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+                AndroidManifest.xml:6: Warning: Did you mean android.permission.BIND_APPWIDGET? [SystemPermissionTypo]
+                    <service android:permission="android.permission.BINDAPPWIDGET" />
+                                                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+                1 errors, 3 warnings
+                """
+            )
+    }
+
     @Test
     fun testFindAlmostCustomPermission() {
         val customPermissions = listOf("my.custom.permission.FOO_BAR", "my.custom.permission.BAZ_QUXX")