From 7358980cbc7493f2e560970c09bfb211d88f5254 Mon Sep 17 00:00:00 2001 From: Mikhail Glukhikh Date: Thu, 14 Dec 2017 12:51:19 +0300 Subject: [PATCH] Add inspection to detect non-const vals used as Java annotation args So #KT-20615 Fixed --- .../FakeJvmFieldConstant.html | 17 +++++ idea/src/META-INF/plugin.xml | 9 +++ .../FakeJvmFieldConstantInspection.kt | 62 +++++++++++++++++ .../inspections/MayBeConstantInspection.kt | 64 ++++++++++++------ .../kotlin/idea/quickfix/AddModifierFix.kt | 2 +- .../inspectionsLocal/mayBeConstant/array.kt | 3 + .../inspectionsLocal/mayBeConstant/enum.kt | 7 ++ .../before/First/First.iml | 20 ++++++ .../before/First/lib/FakeJvmFieldConstant.jar | Bin 0 -> 1152 bytes .../before/First/src/JavaUser.java | 13 ++++ .../before/First/src/KotlinProperties.kt | 7 ++ .../fakeJvmFieldConstant/expected.xml | 20 ++++++ .../fakeJvmFieldConstant.test | 5 ++ .../quickfix/modifiers/constVal.after.kt | 2 +- .../modifiers/constVal.before.Main.kt | 2 +- .../MultiFileInspectionTestGenerated.java | 6 ++ 16 files changed, 215 insertions(+), 24 deletions(-) create mode 100644 idea/resources/inspectionDescriptions/FakeJvmFieldConstant.html create mode 100644 idea/src/org/jetbrains/kotlin/idea/inspections/FakeJvmFieldConstantInspection.kt create mode 100644 idea/testData/inspectionsLocal/mayBeConstant/array.kt create mode 100644 idea/testData/inspectionsLocal/mayBeConstant/enum.kt create mode 100644 idea/testData/multiFileInspections/fakeJvmFieldConstant/before/First/First.iml create mode 100644 idea/testData/multiFileInspections/fakeJvmFieldConstant/before/First/lib/FakeJvmFieldConstant.jar create mode 100644 idea/testData/multiFileInspections/fakeJvmFieldConstant/before/First/src/JavaUser.java create mode 100644 idea/testData/multiFileInspections/fakeJvmFieldConstant/before/First/src/KotlinProperties.kt create mode 100644 idea/testData/multiFileInspections/fakeJvmFieldConstant/expected.xml create mode 100644 idea/testData/multiFileInspections/fakeJvmFieldConstant/fakeJvmFieldConstant.test diff --git a/idea/resources/inspectionDescriptions/FakeJvmFieldConstant.html b/idea/resources/inspectionDescriptions/FakeJvmFieldConstant.html new file mode 100644 index 00000000000..90de4cd24ad --- /dev/null +++ b/idea/resources/inspectionDescriptions/FakeJvmFieldConstant.html @@ -0,0 +1,17 @@ + + +

+ This inspection reports non-const Kotlin properties used as Java annotation arguments. + It's possible for property which is annotated by @JvmField, + 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. +

+

+ This behaviour is subject to change in Kotlin 1.3 (no ConstantValue attribute more). + Recommended action: replace JvmField annotation with const modifier on relevant Kotlin property or inline it. +

+ + + diff --git a/idea/src/META-INF/plugin.xml b/idea/src/META-INF/plugin.xml index d1d23c2cc8e..d34be2d324e 100644 --- a/idea/src/META-INF/plugin.xml +++ b/idea/src/META-INF/plugin.xml @@ -2721,6 +2721,15 @@ language="kotlin" /> + + diff --git a/idea/src/org/jetbrains/kotlin/idea/inspections/FakeJvmFieldConstantInspection.kt b/idea/src/org/jetbrains/kotlin/idea/inspections/FakeJvmFieldConstantInspection.kt new file mode 100644 index 00000000000..5cb16c1056a --- /dev/null +++ b/idea/src/org/jetbrains/kotlin/idea/inspections/FakeJvmFieldConstantInspection.kt @@ -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() + 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() + ) + } + } + } + } + } + } +} \ No newline at end of file diff --git a/idea/src/org/jetbrains/kotlin/idea/inspections/MayBeConstantInspection.kt b/idea/src/org/jetbrains/kotlin/idea/inspections/MayBeConstantInspection.kt index 21d8dc609bb..b021856ce98 100644 --- a/idea/src/org/jetbrains/kotlin/idea/inspections/MayBeConstantInspection.kt +++ b/idea/src/org/jetbrains/kotlin/idea/inspections/MayBeConstantInspection.kt @@ -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 } } } diff --git a/idea/src/org/jetbrains/kotlin/idea/quickfix/AddModifierFix.kt b/idea/src/org/jetbrains/kotlin/idea/quickfix/AddModifierFix.kt index bc3aa95fd37..cf671fb9e52 100644 --- a/idea/src/org/jetbrains/kotlin/idea/quickfix/AddModifierFix.kt +++ b/idea/src/org/jetbrains/kotlin/idea/quickfix/AddModifierFix.kt @@ -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" diff --git a/idea/testData/inspectionsLocal/mayBeConstant/array.kt b/idea/testData/inspectionsLocal/mayBeConstant/array.kt new file mode 100644 index 00000000000..e2e54ed7f6e --- /dev/null +++ b/idea/testData/inspectionsLocal/mayBeConstant/array.kt @@ -0,0 +1,3 @@ +// WITH_RUNTIME + +val arr = arrayOf("alpha", "omega") \ No newline at end of file diff --git a/idea/testData/inspectionsLocal/mayBeConstant/enum.kt b/idea/testData/inspectionsLocal/mayBeConstant/enum.kt new file mode 100644 index 00000000000..c820c8e3272 --- /dev/null +++ b/idea/testData/inspectionsLocal/mayBeConstant/enum.kt @@ -0,0 +1,7 @@ +// PROBLEM: none + +enum class E { + FIRST +} + +val e = E.FIRST \ No newline at end of file diff --git a/idea/testData/multiFileInspections/fakeJvmFieldConstant/before/First/First.iml b/idea/testData/multiFileInspections/fakeJvmFieldConstant/before/First/First.iml new file mode 100644 index 00000000000..6c1501649ec --- /dev/null +++ b/idea/testData/multiFileInspections/fakeJvmFieldConstant/before/First/First.iml @@ -0,0 +1,20 @@ + + + + + + + + + + + + + + + + + + + + \ No newline at end of file diff --git a/idea/testData/multiFileInspections/fakeJvmFieldConstant/before/First/lib/FakeJvmFieldConstant.jar b/idea/testData/multiFileInspections/fakeJvmFieldConstant/before/First/lib/FakeJvmFieldConstant.jar new file mode 100644 index 0000000000000000000000000000000000000000..7558ba93726b2f4f40299e1b8d5eeef97fd294be GIT binary patch literal 1152 zcmWIWW@Zs#;Nak3@U7|dWvRa*us4irPt2Yeh5&DN zj{n70GATee%>v>8pi8+J7&sU(T*?WQg}W4_4B{alG!JPbD|1WCPW39wb<0f6Npa54 zD=taQE78l&FUiTwi_gtZDa}buPDn^d1LBq>KfORbHbuw(lX|W!YFPBA>B^!>XRai@ z7Xmv`#I7n>8ECRP)QQ;b76;1umt>?C1r+5Mq!yKArWSjb=q2YQ78j=m_~u`B5SddN zb)EBX!DSH-?xphV51OKnFn02Din?;OhzN>=zRH}t=92H5yIFg`S(^W2S6|m5%`Wov zM8DG>QjcT3cut>6r|oN7$( zqRkaLY)irN@g8r%ygv>e~7&X-ptZX3nk?J^f+zhYilsKhnGV)utupK78|8 zMVx21d{Jtu)~!_&X7&hwd{q_FwlvsQ<+7W**2S}zBWCo^vDtEB-np|5Tb3xF-7op~ zzfghGTAiIAFP6*|-2Ix(y}|a7)ZA67R_+bb5vqIKbNVmX*4h4OySx7B-+hml`gtF4 zoLS7?-lFdjx%kgl?vPtIH0}$`<_^nG=H?CGrhmZb&Jo>Rr#8%aSl0F|ACyRcWVQS5 zWn^HeVge;nMkWykM2136z@Q9;3gC$rEqj4X;bOp@*b%0z0y5#IAX^Sf{K!cYl=u + + JavaUser.java + 3 + First + <default> + + Kotlin non-const property used as annotation argument + Use of @JvmField non-const Kotlin property as annotation argument is incorrect. Will be forbidden in 1.3 + + + JavaUser.java + 7 + First + <default> + + Kotlin non-const property used as annotation argument + Use of @JvmField non-const Kotlin property as annotation argument is incorrect. Will be forbidden in 1.3 + + diff --git a/idea/testData/multiFileInspections/fakeJvmFieldConstant/fakeJvmFieldConstant.test b/idea/testData/multiFileInspections/fakeJvmFieldConstant/fakeJvmFieldConstant.test new file mode 100644 index 00000000000..2246059b4af --- /dev/null +++ b/idea/testData/multiFileInspections/fakeJvmFieldConstant/fakeJvmFieldConstant.test @@ -0,0 +1,5 @@ +{ + "inspectionClass": "org.jetbrains.kotlin.idea.inspections.FakeJvmFieldConstantInspection", + "withRuntime": "true", + "isMultiModule": "true" +} \ No newline at end of file diff --git a/idea/testData/quickfix/modifiers/constVal.after.kt b/idea/testData/quickfix/modifiers/constVal.after.kt index 51a147a3402..16ea1d430e4 100644 --- a/idea/testData/quickfix/modifiers/constVal.after.kt +++ b/idea/testData/quickfix/modifiers/constVal.after.kt @@ -1,4 +1,4 @@ -// "Add 'const' modifier" "true" +// "Make 'i' const" "true" // ERROR: Only 'const val' can be used in constant expressions package constVal diff --git a/idea/testData/quickfix/modifiers/constVal.before.Main.kt b/idea/testData/quickfix/modifiers/constVal.before.Main.kt index 14e16dd2a30..47f9a5cc296 100644 --- a/idea/testData/quickfix/modifiers/constVal.before.Main.kt +++ b/idea/testData/quickfix/modifiers/constVal.before.Main.kt @@ -1,4 +1,4 @@ -// "Add 'const' modifier" "true" +// "Make 'i' const" "true" // ERROR: Only 'const val' can be used in constant expressions package constVal diff --git a/idea/tests/org/jetbrains/kotlin/idea/codeInsight/MultiFileInspectionTestGenerated.java b/idea/tests/org/jetbrains/kotlin/idea/codeInsight/MultiFileInspectionTestGenerated.java index e23d6024167..57df749915e 100644 --- a/idea/tests/org/jetbrains/kotlin/idea/codeInsight/MultiFileInspectionTestGenerated.java +++ b/idea/tests/org/jetbrains/kotlin/idea/codeInsight/MultiFileInspectionTestGenerated.java @@ -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");