Android Extensions: Show warning on a dot-call of a reference which is missing in some configurations (KT-18012)

When more than one layout configuration is available, a particular resource (view or fragment) may be absent in some of them. We should show a warning on such resource reference calls as the call may lead to NPE.
2017-05-26 22:49:35 +03:00
parent f4acf404ca
commit f7786a42ab
9 changed files with 126 additions and 50 deletions
@@ -188,7 +188,7 @@ class AndroidExpressionCodegenExtension : ExpressionCodegenExtension {
                val declarationDescriptorType = typeMapper.mapType(receiverDescriptor)
                receiver.put(declarationDescriptorType, v)

-                val resourceId = syntheticProperty.resourceId
+                val resourceId = syntheticProperty.resource.id
                val packageName = resourceId.packageName ?: androidPackage
                v.getstatic(packageName.replace(".", "/") + "/R\$id", resourceId.name, "I")

@@ -22,10 +22,15 @@ import org.jetbrains.kotlin.android.synthetic.diagnostic.ErrorsAndroid.*
 import org.jetbrains.kotlin.android.synthetic.res.AndroidSyntheticProperty
 import org.jetbrains.kotlin.descriptors.PropertyDescriptor
 import org.jetbrains.kotlin.diagnostics.DiagnosticSink
+import org.jetbrains.kotlin.psi.KtDotQualifiedExpression
+import org.jetbrains.kotlin.psi.KtElement
 import org.jetbrains.kotlin.psi.KtExpression
+import org.jetbrains.kotlin.psi.KtQualifiedExpression
 import org.jetbrains.kotlin.resolve.calls.checkers.CallChecker
 import org.jetbrains.kotlin.resolve.calls.checkers.CallCheckerContext
 import org.jetbrains.kotlin.resolve.calls.model.ResolvedCall
+import org.jetbrains.kotlin.types.TypeUtils
+import org.jetbrains.kotlin.types.isFlexible

 class AndroidExtensionPropertiesCallChecker : CallChecker {
    override fun check(resolvedCall: ResolvedCall<*>, reportOn: PsiElement, context: CallCheckerContext) {
@@ -38,6 +43,7 @@ class AndroidExtensionPropertiesCallChecker : CallChecker {
        with(context.trace) {
            checkUnresolvedWidgetType(reportOn, androidSyntheticProperty)
            checkDeprecated(reportOn, containingPackage)
+            checkPartiallyDefinedResource(resolvedCall, androidSyntheticProperty, context)
        }
    }

@@ -54,4 +60,36 @@ class AndroidExtensionPropertiesCallChecker : CallChecker {
        val warning = if (type.contains('.')) SYNTHETIC_UNRESOLVED_WIDGET_TYPE else SYNTHETIC_INVALID_WIDGET_TYPE
        report(warning.on(expression, type))
    }
+
+    private fun DiagnosticSink.checkPartiallyDefinedResource(
+            resolvedCall: ResolvedCall<*>,
+            property: AndroidSyntheticProperty,
+            context: CallCheckerContext
+    ) {
+        if (!property.resource.partiallyDefined) return
+        val calleeExpression = resolvedCall.call.calleeExpression ?: return
+
+        val expectedType = context.resolutionContext.expectedType
+        if (!TypeUtils.noExpectedType(expectedType) && !expectedType.isMarkedNullable && !expectedType.isFlexible()) {
+            report(UNSAFE_CALL_ON_PARTIALLY_DEFINED_RESOURCE.on(calleeExpression))
+            return
+        }
+
+        val outermostQualifiedExpression = findLeftOutermostQualifiedExpression(calleeExpression) ?: return
+        val usage = outermostQualifiedExpression.parent
+
+        if (usage is KtDotQualifiedExpression && usage.receiverExpression == outermostQualifiedExpression) {
+            report(UNSAFE_CALL_ON_PARTIALLY_DEFINED_RESOURCE.on(calleeExpression))
+        }
+    }
+
+    private fun findLeftOutermostQualifiedExpression(calleeExpression: KtExpression?): KtElement? {
+        val parent = calleeExpression?.parent ?: return null
+
+        if (parent is KtQualifiedExpression && parent.selectorExpression == calleeExpression) {
+            return findLeftOutermostQualifiedExpression(parent)
+        }
+
+        return calleeExpression
+    }
 }
@@ -36,6 +36,9 @@ class DefaultErrorMessagesAndroid : DefaultErrorMessages.Extension {

            MAP.put(ErrorsAndroid.SYNTHETIC_DEPRECATED_PACKAGE,
                    "Use properties from the build variant packages")
+
+            MAP.put(ErrorsAndroid.UNSAFE_CALL_ON_PARTIALLY_DEFINED_RESOURCE,
+                    "Potential NullPointerException. The resource is missing in some of layout versions")
        }
    }

@@ -27,6 +27,7 @@ public interface ErrorsAndroid {
    DiagnosticFactory1<KtExpression, String> SYNTHETIC_INVALID_WIDGET_TYPE = DiagnosticFactory1.create(WARNING);
    DiagnosticFactory1<KtExpression, String> SYNTHETIC_UNRESOLVED_WIDGET_TYPE = DiagnosticFactory1.create(WARNING);
    DiagnosticFactory0<KtExpression> SYNTHETIC_DEPRECATED_PACKAGE = DiagnosticFactory0.create(WARNING);
+    DiagnosticFactory0<KtExpression> UNSAFE_CALL_ON_PARTIALLY_DEFINED_RESOURCE = DiagnosticFactory0.create(WARNING);

    @SuppressWarnings("UnusedDeclaration")
    Object _initializer = new Object() {
@@ -88,7 +88,7 @@ abstract class AndroidLayoutXmlFileManager(val project: Project) {
        return filterDuplicates(doExtractResources(files, module))
    }

-    protected abstract fun doExtractResources(files: List<PsiFile>, module: ModuleDescriptor): List<AndroidResource>
+    protected abstract fun doExtractResources(files: List<PsiFile>, module: ModuleDescriptor): List<AndroidLayoutGroup>

    protected fun parseAndroidResource(id: ResourceIdentifier, tag: String, sourceElement: PsiElement?): AndroidResource {
        return when (tag) {
@@ -98,27 +98,46 @@ abstract class AndroidLayoutXmlFileManager(val project: Project) {
        }
    }

-    private fun filterDuplicates(resources: List<AndroidResource>): List<AndroidResource> {
+    private fun filterDuplicates(layoutGroups: List<AndroidLayoutGroup>): List<AndroidResource> {
        val resourceMap = linkedMapOf<String, AndroidResource>()
        val resourcesToExclude = hashSetOf<String>()

-        for (res in resources) {
-            if (resourceMap.contains(res.id.name)) {
-                val existing = resourceMap[res.id.name]!!
+        for (layoutGroup in layoutGroups) {
+            val resources = layoutGroup.layouts.flatMap { it.resources }.groupBy {
+                val id = it.id
+                if (id.packageName == null) id.name else id.packageName + "/" + id.name
+            }

-                if (!res.sameClass(existing) || res.id.packageName != existing.id.packageName) {
-                    resourcesToExclude.add(res.id.name)
-                }
-                else if (res is AndroidResource.Widget && existing is AndroidResource.Widget) {
-                    // Widgets with the same id but different types exist.
-                    if (res.xmlType != existing.xmlType && existing.xmlType != AndroidConst.VIEW_FQNAME) {
-                        resourceMap.put(res.id.name, AndroidResource.Widget(res.id, AndroidConst.VIEW_FQNAME, res.sourceElement))
+            for (resources in resources.values) {
+                val isPartiallyDefined = resources.size < layoutGroup.layouts.size
+
+                for (res in resources) {
+                    if (resourceMap.contains(res.id.name)) {
+                        val existing = resourceMap[res.id.name]!!
+
+                        if (!res.sameClass(existing) || res.id.packageName != existing.id.packageName) {
+                            resourcesToExclude.add(res.id.name)
+                        }
+                        else if (res is AndroidResource.Widget && existing is AndroidResource.Widget) {
+                            // Widgets with the same id but different types exist.
+                            if (res.xmlType != existing.xmlType && existing.xmlType != AndroidConst.VIEW_FQNAME) {
+                                val mergedWidget = AndroidResource.Widget(
+                                        res.id, AndroidConst.VIEW_FQNAME, res.sourceElement, isPartiallyDefined)
+                                resourceMap.put(res.id.name, mergedWidget)
+                            }
+                        }
+                    }
+                    else if (isPartiallyDefined) {
+                        resourceMap.put(res.id.name, res.partiallyDefined())
+                    }
+                    else {
+                        resourceMap.put(res.id.name, res)
                    }
                }
            }
-            else resourceMap.put(res.id.name, res)
        }
-        resourcesToExclude.forEach { resourceMap.remove(it) }
+
+        resourceMap.keys.removeAll(resourcesToExclude)
        return resourceMap.values.toList()
    }

@@ -16,7 +16,6 @@

 package org.jetbrains.kotlin.android.synthetic.res

-import com.intellij.openapi.diagnostic.Logger
 import com.intellij.openapi.project.Project
 import com.intellij.psi.PsiFile
 import org.jetbrains.kotlin.android.synthetic.AndroidXmlHandler
@@ -30,31 +29,28 @@ class CliAndroidLayoutXmlFileManager(
        applicationPackage: String,
        variants: List<AndroidVariant>
 ) : AndroidLayoutXmlFileManager(project) {
-    private companion object {
-        val LOG = Logger.getInstance(CliAndroidLayoutXmlFileManager::class.java)
-    }
-
    override val androidModule = AndroidModule(applicationPackage, variants)

    private val saxParser: SAXParser = initSAX()

-    override fun doExtractResources(files: List<PsiFile>, module: ModuleDescriptor): List<AndroidResource> {
-        val resources = arrayListOf<AndroidResource>()
+    override fun doExtractResources(files: List<PsiFile>, module: ModuleDescriptor): List<AndroidLayoutGroup> {
+        val layoutGroupFiles = files.groupBy { it.name }
+        val layoutGroups = mutableListOf<AndroidLayoutGroup>()

-        val handler = AndroidXmlHandler { id, tag ->
-            resources += parseAndroidResource(id, tag, null)
+        for ((name, layouts) in layoutGroupFiles) {
+            layoutGroups += AndroidLayoutGroup(name, layouts.map { layout ->
+                val resources = arrayListOf<AndroidResource>()
+
+                val inputStream = ByteArrayInputStream(layout.virtualFile.contentsToByteArray())
+                saxParser.parse(inputStream, AndroidXmlHandler { id, tag ->
+                    resources += parseAndroidResource(id, tag, null)
+                })
+
+                AndroidLayout(resources)
+            })
        }

-        for (file in files) {
-            try {
-                val inputStream = ByteArrayInputStream(file.virtualFile.contentsToByteArray())
-                saxParser.parse(inputStream, handler)
-            } catch (e: Throwable) {
-                LOG.error(e)
-            }
-        }
-
-        return resources
+        return layoutGroups
    }

    private fun initSAX(): SAXParser {
@@ -61,19 +61,31 @@ class ResourceIdentifier(val name: String, val packageName: String?) {
    }
 }

-sealed class AndroidResource(val id: ResourceIdentifier, val sourceElement: PsiElement?) {
+class AndroidLayoutGroup(val name: String, val layouts: List<AndroidLayout>)
+
+class AndroidLayout(val resources: List<AndroidResource>)
+
+sealed class AndroidResource(val id: ResourceIdentifier, val sourceElement: PsiElement?, val partiallyDefined: Boolean) {
    open fun sameClass(other: AndroidResource): Boolean = false
+    open fun partiallyDefined(): AndroidResource = this

    class Widget(
            id: ResourceIdentifier,
            val xmlType: String,
-            sourceElement: PsiElement?
-    ) : AndroidResource(id, sourceElement) {
+            sourceElement: PsiElement?,
+            partiallyDefined: Boolean = false
+    ) : AndroidResource(id, sourceElement, partiallyDefined) {
        override fun sameClass(other: AndroidResource) = other is Widget
+        override fun partiallyDefined() = Widget(id, xmlType, sourceElement, true)
    }

-    class Fragment(id: ResourceIdentifier, sourceElement: PsiElement?) : AndroidResource(id, sourceElement) {
+    class Fragment(
+            id: ResourceIdentifier,
+            sourceElement: PsiElement?,
+            partiallyDefined: Boolean = false
+    ) : AndroidResource(id, sourceElement, partiallyDefined) {
        override fun sameClass(other: AndroidResource) = other is Fragment
+        override fun partiallyDefined() = Fragment(id, sourceElement, true)
    }
 }

@@ -68,7 +68,7 @@ internal fun genPropertyForWidget(
                                                defaultType.constructor.parameters.map(::StarProjectionImpl))
    } ?: context.viewType

-    return genProperty(resolvedWidget.widget.id, receiverType, type, packageFragmentDescriptor, sourceEl, resolvedWidget.errorType)
+    return genProperty(resolvedWidget.widget, receiverType, type, packageFragmentDescriptor, sourceEl, resolvedWidget.errorType)
 }

 internal fun genPropertyForFragment(
@@ -78,11 +78,11 @@ internal fun genPropertyForFragment(
        fragment: AndroidResource.Fragment
 ): PropertyDescriptor {
    val sourceElement = fragment.sourceElement?.let(::XmlSourceElement) ?: SourceElement.NO_SOURCE
-    return genProperty(fragment.id, receiverType, type, packageFragmentDescriptor, sourceElement, null)
+    return genProperty(fragment, receiverType, type, packageFragmentDescriptor, sourceElement, null)
 }

 private fun genProperty(
-        id: ResourceIdentifier,
+        resource: AndroidResource,
        receiverType: KotlinType,
        type: SimpleType,
        containingDeclaration: AndroidSyntheticPackageFragmentDescriptor,
@@ -98,7 +98,7 @@ private fun genProperty(
            Modality.FINAL,
            Visibilities.PUBLIC,
            false,
-            Name.identifier(id.name),
+            Name.identifier(resource.id.name),
            CallableMemberDescriptor.Kind.SYNTHESIZED,
            sourceElement,
            /* lateinit = */ false,
@@ -110,7 +110,7 @@ private fun genProperty(
    ) {
        override val errorType = errorType
        override val cacheView = cacheView
-        override val resourceId = id
+        override val resource = resource
    }

    // todo support (Mutable)List
@@ -146,7 +146,7 @@ interface AndroidSyntheticFunction
 interface AndroidSyntheticProperty {
    val errorType: String?
    val cacheView: Boolean
-    val resourceId: ResourceIdentifier
+    val resource: AndroidResource

    val isErrorType: Boolean
        get() = errorType != null