Add inspection to detect non-const vals used as Java annotation args

So #KT-20615 Fixed
2017-12-14 12:51:19 +03:00
parent 70b7e5eb68
commit 7358980cbc
16 changed files with 215 additions and 24 deletions
@@ -0,0 +1,17 @@
+<html>
+<body>
+<p>
+  This inspection reports non-const Kotlin properties used as Java annotation arguments.
+  It's possible for property which is annotated by <code>@JvmField</code>,
+  has initializer that can be evaluated in compile-time, has primitive or String type.
+  Such properties have ConstantValue attribute in bytecode in Kotlin 1.1-1.2.
+  This attribute allows javac to fold usages of the corresponding field, and use that field in annotations.
+  This can cause incorrect behavior in case of separate or incremental compilation in mixed Java/Kotlin code.
+</p>
+<p>
+  This behaviour is subject to change in Kotlin 1.3 (no ConstantValue attribute more).
+  Recommended action: replace <code>JvmField</code> annotation with <code>const</code> modifier on relevant Kotlin property or inline it.
+</p>
+
+</body>
+</html>
@@ -2721,6 +2721,15 @@
                     language="kotlin"
    />

+    <localInspection implementationClass="org.jetbrains.kotlin.idea.inspections.FakeJvmFieldConstantInspection"
+                     displayName="Kotlin non-const property used as annotation argument"
+                     groupPath="Kotlin"
+                     groupName="Java interop issues"
+                     enabledByDefault="true"
+                     level="WARNING"
+                     language="JAVA"
+    />
+
    <referenceImporter implementation="org.jetbrains.kotlin.idea.quickfix.KotlinReferenceImporter"/>

    <fileType.fileViewProviderFactory filetype="KJSM" implementationClass="com.intellij.psi.ClassFileViewProviderFactory"/>
@@ -0,0 +1,62 @@
+/*
+ * Copyright 2010-2017 JetBrains s.r.o.
+ *
+ * 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 org.jetbrains.kotlin.idea.inspections
+
+import com.intellij.codeInspection.IntentionWrapper
+import com.intellij.codeInspection.LocalQuickFix
+import com.intellij.codeInspection.ProblemHighlightType
+import com.intellij.codeInspection.ProblemsHolder
+import com.intellij.psi.*
+import org.jetbrains.kotlin.asJava.elements.KtLightFieldImpl.KtLightFieldForDeclaration
+import org.jetbrains.kotlin.idea.inspections.MayBeConstantInspection.Status.JVM_FIELD_MIGHT_BE_CONST
+import org.jetbrains.kotlin.idea.inspections.MayBeConstantInspection.Status.JVM_FIELD_MIGHT_BE_CONST_NO_INITIALIZER
+import org.jetbrains.kotlin.idea.quickfix.AddConstModifierFix
+import org.jetbrains.kotlin.psi.KtProperty
+
+class FakeJvmFieldConstantInspection : AbstractKotlinInspection() {
+    override fun buildVisitor(holder: ProblemsHolder, isOnTheFly: Boolean): PsiElementVisitor {
+        return object : JavaElementVisitor() {
+            override fun visitAnnotationParameterList(list: PsiAnnotationParameterList) {
+                super.visitAnnotationParameterList(list)
+
+                for (attribute in list.attributes) {
+                    val valueExpression = attribute.value as? PsiExpression ?: continue
+                    val resolvedLightField = (valueExpression as? PsiReference)?.resolve() as? KtLightFieldForDeclaration ?: continue
+                    val resolvedProperty = resolvedLightField.kotlinOrigin as? KtProperty ?: continue
+                    with(MayBeConstantInspection) {
+                        if (resolvedProperty.annotationEntries.isEmpty()) return@with
+                        val resolvedPropertyStatus = resolvedProperty.getStatus()
+                        if (resolvedPropertyStatus == JVM_FIELD_MIGHT_BE_CONST ||
+                            resolvedPropertyStatus == JVM_FIELD_MIGHT_BE_CONST_NO_INITIALIZER) {
+                            val fixes = mutableListOf<LocalQuickFix>()
+                            if (resolvedPropertyStatus == JVM_FIELD_MIGHT_BE_CONST) {
+                                fixes += IntentionWrapper(AddConstModifierFix(resolvedProperty), resolvedProperty.containingFile)
+                            }
+                            holder.registerProblem(
+                                    valueExpression,
+                                    "Use of @JvmField non-const Kotlin property as annotation argument is incorrect." +
+                                    " Will be forbidden in 1.3",
+                                    ProblemHighlightType.GENERIC_ERROR_OR_WARNING,
+                                    *fixes.toTypedArray()
+                            )
+                        }
+                    }
+                }
+            }
+        }
+    }
+}
@@ -22,6 +22,7 @@ import com.intellij.codeInspection.ProblemsHolder
 import com.intellij.psi.PsiElementVisitor
 import org.jetbrains.kotlin.descriptors.VariableDescriptor
 import org.jetbrains.kotlin.idea.caches.resolve.analyze
+import org.jetbrains.kotlin.idea.inspections.MayBeConstantInspection.Status.*
 import org.jetbrains.kotlin.idea.quickfix.AddConstModifierFix
 import org.jetbrains.kotlin.lexer.KtTokens
 import org.jetbrains.kotlin.psi.KtClass
@@ -35,34 +36,55 @@ import org.jetbrains.kotlin.resolve.jvm.annotations.hasJvmFieldAnnotation
 import org.jetbrains.kotlin.resolve.lazy.BodyResolveMode

 class MayBeConstantInspection : AbstractKotlinInspection() {
+    enum class Status {
+        NONE,
+        MIGHT_BE_CONST,
+        JVM_FIELD_MIGHT_BE_CONST,
+        JVM_FIELD_MIGHT_BE_CONST_NO_INITIALIZER
+    }
+
    override fun buildVisitor(holder: ProblemsHolder, isOnTheFly: Boolean): PsiElementVisitor {
        return object : KtVisitorVoid() {
            override fun visitProperty(property: KtProperty) {
                super.visitProperty(property)
+                val status = property.getStatus()
+                when (status) {
+                    NONE, JVM_FIELD_MIGHT_BE_CONST_NO_INITIALIZER -> return
+                    else -> {
+                        holder.registerProblem(
+                                property.nameIdentifier ?: property,
+                                if (status == JVM_FIELD_MIGHT_BE_CONST) "'const' might be used instead of '@JvmField'" else "Might be 'const'",
+                                ProblemHighlightType.GENERIC_ERROR_OR_WARNING,
+                                IntentionWrapper(AddConstModifierFix(property), property.containingFile)
+                        )
+                    }
+                }
+            }
+        }
+    }

-                if (property.isLocal || property.isVar) return
-                val initializer = property.initializer ?: return
-                if (property.hasModifier(KtTokens.CONST_KEYWORD)) return
-                // Top-level or object only
-                if (property.containingClassOrObject is KtClass) return
+    companion object {
+        fun KtProperty.getStatus(): Status {
+            if (isLocal || isVar || hasModifier(KtTokens.CONST_KEYWORD) || containingClassOrObject is KtClass) {
+                return NONE
+            }
+            val initializer = initializer

-                // For some reason constant evaluation does not work for property.analyze()
-                val context = initializer.analyze(BodyResolveMode.PARTIAL)
-                val propertyDescriptor = context[BindingContext.DECLARATION_TO_DESCRIPTOR, property] as? VariableDescriptor ?: return
-                val withJvmField = propertyDescriptor.hasJvmFieldAnnotation()
-                if (property.annotationEntries.isNotEmpty() && !withJvmField) return
+            // For some reason constant evaluation does not work for property.analyze()
+            val context = (initializer ?: this).analyze(BodyResolveMode.PARTIAL)
+            val propertyDescriptor = context[BindingContext.DECLARATION_TO_DESCRIPTOR, this] as? VariableDescriptor ?: return NONE
+            val withJvmField = propertyDescriptor.hasJvmFieldAnnotation()
+            if (annotationEntries.isNotEmpty() && !withJvmField) return NONE

-                val initializerValue = ConstantExpressionEvaluator.getConstant(
-                        initializer, context
-                )?.toConstantValue(propertyDescriptor.type) ?: return
-                if (initializerValue.isStandaloneOnlyConstant()) return
-
-                holder.registerProblem(
-                        property.nameIdentifier ?: property,
-                        if (withJvmField) "'const' might be used instead of '@JvmField'" else "Might be 'const'",
-                        ProblemHighlightType.GENERIC_ERROR_OR_WARNING,
-                        IntentionWrapper(AddConstModifierFix(property), property.containingFile)
-                )
+            return when {
+                initializer != null -> {
+                    ConstantExpressionEvaluator.getConstant(
+                            initializer, context
+                    )?.toConstantValue(propertyDescriptor.type)?.takeIf { !it.isStandaloneOnlyConstant() } ?: return NONE
+                    if (withJvmField) JVM_FIELD_MIGHT_BE_CONST else MIGHT_BE_CONST
+                }
+                withJvmField -> JVM_FIELD_MIGHT_BE_CONST_NO_INITIALIZER
+                else -> NONE
            }
        }
    }
@@ -47,7 +47,7 @@ open class AddModifierFix(

    override fun getText(): String {
        val element = element ?: return ""
-        if (modifier in modalityModifiers || modifier in KtTokens.VISIBILITY_MODIFIERS) {
+        if (modifier in modalityModifiers || modifier in KtTokens.VISIBILITY_MODIFIERS || modifier == KtTokens.CONST_KEYWORD) {
            return "Make ${getElementName(element)} ${modifier.value}"
        }
        return "Add '${modifier.value}' modifier"
@@ -0,0 +1,3 @@
+// WITH_RUNTIME
+
+val <caret>arr = arrayOf("alpha", "omega")
@@ -0,0 +1,7 @@
+// PROBLEM: none
+
+enum class E {
+    FIRST
+}
+
+val <caret>e = E.FIRST
@@ -0,0 +1,20 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<module type="JAVA_MODULE" version="4">
+  <component name="NewModuleRootManager" inherit-compiler-output="true">
+    <exclude-output />
+    <content url="file://$MODULE_DIR$">
+      <sourceFolder url="file://$MODULE_DIR$/src" isTestSource="false" />
+    </content>
+    <orderEntry type="inheritedJdk" />
+    <orderEntry type="sourceFolder" forTests="false" />
+    <orderEntry type="module-library">
+      <library>
+        <CLASSES>
+          <root url="jar://$MODULE_DIR$/lib/FakeJvmFieldConstant.jar!/" />
+        </CLASSES>
+        <JAVADOC />
+        <SOURCES />
+      </library>
+    </orderEntry>
+  </component>
+</module>
@@ -0,0 +1,13 @@
+public class JavaUser {
+    // Dangerous
+    @Ann(s = KotlinPropertiesKt.importantString)
+    public void foo() {}
+
+    // Also dangerous
+    @AnnValue(OtherPropertiesKt.notSoImportantString)
+    public void bar() {}
+
+    // Safe
+    @Ann(s = KotlinPropertiesKt.constantString)
+    public void baz() {}
+}
@@ -0,0 +1,7 @@
+annotation class Ann(val s: String)
+
+annotation class AnnValue(val value: String)
+
+@JvmField val importantString = "important"
+
+const val constantString = "constant"
@@ -0,0 +1,20 @@
+<problems>
+    <problem>
+        <file>JavaUser.java</file>
+        <line>3</line>
+        <module>First</module>
+        <package>&lt;default&gt;</package>
+        <entry_point TYPE="file" FQNAME="file://C:/Users/Mikhail.Glukhikh/AppData/Local/Temp/unitTest_fakeJvmFieldConstant_FakeJvmFieldConstant/unitTest1/First/src/JavaUser.java" />
+        <problem_class severity="WARNING" attribute_key="WARNING_ATTRIBUTES">Kotlin non-const property used as annotation argument</problem_class>
+        <description>Use of @JvmField non-const Kotlin property as annotation argument is incorrect. Will be forbidden in 1.3</description>
+    </problem>
+    <problem>
+        <file>JavaUser.java</file>
+        <line>7</line>
+        <module>First</module>
+        <package>&lt;default&gt;</package>
+        <entry_point TYPE="file" FQNAME="file://C:/Users/Mikhail.Glukhikh/AppData/Local/Temp/unitTest_fakeJvmFieldConstant_FakeJvmFieldConstant/unitTest1/First/src/JavaUser.java" />
+        <problem_class severity="WARNING" attribute_key="WARNING_ATTRIBUTES">Kotlin non-const property used as annotation argument</problem_class>
+        <description>Use of @JvmField non-const Kotlin property as annotation argument is incorrect. Will be forbidden in 1.3</description>
+    </problem>
+</problems>
@@ -0,0 +1,5 @@
+{
+    "inspectionClass": "org.jetbrains.kotlin.idea.inspections.FakeJvmFieldConstantInspection",
+    "withRuntime": "true",
+    "isMultiModule": "true"
+}
@@ -1,4 +1,4 @@
-// "Add 'const' modifier" "true"
+// "Make 'i' const" "true"
 // ERROR: Only 'const val' can be used in constant expressions

 package constVal
@@ -1,4 +1,4 @@
-// "Add 'const' modifier" "true"
+// "Make 'i' const" "true"
 // ERROR: Only 'const val' can be used in constant expressions

 package constVal
@@ -36,6 +36,12 @@ public class MultiFileInspectionTestGenerated extends AbstractMultiFileInspectio
        KotlinTestUtils.assertAllTestsPresentInSingleGeneratedClass(this.getClass(), new File("idea/testData/multiFileInspections"), Pattern.compile("^(.+)\\.test$"), TargetBackend.ANY);
    }

+    @TestMetadata("fakeJvmFieldConstant/fakeJvmFieldConstant.test")
+    public void testFakeJvmFieldConstant_FakeJvmFieldConstant() throws Exception {
+        String fileName = KotlinTestUtils.navigationMetadata("idea/testData/multiFileInspections/fakeJvmFieldConstant/fakeJvmFieldConstant.test");
+        doTest(fileName);
+    }
+
    @TestMetadata("invalidBundleOrProperty/invalidBundleOrProperty.test")
    public void testInvalidBundleOrProperty_InvalidBundleOrProperty() throws Exception {
        String fileName = KotlinTestUtils.navigationMetadata("idea/testData/multiFileInspections/invalidBundleOrProperty/invalidBundleOrProperty.test");