From c6018901514a81719d9d1952ec67bfce3821cd65 Mon Sep 17 00:00:00 2001 From: Alexey Tsvetkov Date: Mon, 31 Jul 2017 22:04:33 +0300 Subject: [PATCH] Fix comparing repeated proto extensions Previously ProtoCompareGenerated could go out of bounds when comparing protos with different number of annotations. That happend because JsProtoBuf.parameterAnnotation is a repeated extension, but the generated code for comparing repeated extensions was incorrect. JvmProtoBuf does not have repeated extensions (at least for the class and package descriptors), so the problem was not detected before. --- .../incremental/ProtoCompareGenerated.kt | 165 +++++++++++------- .../protobuf/GenerateProtoBufCompare.kt | 45 +++-- 2 files changed, 133 insertions(+), 77 deletions(-) diff --git a/build-common/src/org/jetbrains/kotlin/incremental/ProtoCompareGenerated.kt b/build-common/src/org/jetbrains/kotlin/incremental/ProtoCompareGenerated.kt index 67ffbe36918..554204147fb 100644 --- a/build-common/src/org/jetbrains/kotlin/incremental/ProtoCompareGenerated.kt +++ b/build-common/src/org/jetbrains/kotlin/incremental/ProtoCompareGenerated.kt @@ -57,10 +57,13 @@ open class ProtoCompareGenerated(val oldNameResolver: NameResolver, val newNameR if (!checkStringEquals(old.getExtension(JvmProtoBuf.packageModuleName), new.getExtension(JvmProtoBuf.packageModuleName))) return false } - if (old.getExtensionCount(JvmProtoBuf.packageLocalVariable) != new.getExtensionCount(JvmProtoBuf.packageLocalVariable)) return false - - for(i in 0..old.getExtensionCount(JvmProtoBuf.packageLocalVariable) - 1) { - if (!checkEquals(old.getExtension(JvmProtoBuf.packageLocalVariable, i), new.getExtension(JvmProtoBuf.packageLocalVariable, i))) return false + if (old.getExtensionCount(JvmProtoBuf.packageLocalVariable) != new.getExtensionCount(JvmProtoBuf.packageLocalVariable)) { + return false + } + else { + for(i in 0..old.getExtensionCount(JvmProtoBuf.packageLocalVariable) - 1) { + if (!checkEquals(old.getExtension(JvmProtoBuf.packageLocalVariable, i), new.getExtension(JvmProtoBuf.packageLocalVariable, i))) return false + } } if (old.hasExtension(JsProtoBuf.packageFqName) != new.hasExtension(JsProtoBuf.packageFqName)) return false @@ -105,10 +108,13 @@ open class ProtoCompareGenerated(val oldNameResolver: NameResolver, val newNameR if (!checkStringEquals(old.getExtension(JvmProtoBuf.packageModuleName), new.getExtension(JvmProtoBuf.packageModuleName))) result.add(ProtoBufPackageKind.JVM_EXT_PACKAGE_MODULE_NAME) } - if (old.getExtensionCount(JvmProtoBuf.packageLocalVariable) != new.getExtensionCount(JvmProtoBuf.packageLocalVariable)) result.add(ProtoBufPackageKind.JVM_EXT_PACKAGE_LOCAL_VARIABLE_LIST) - - for(i in 0..old.getExtensionCount(JvmProtoBuf.packageLocalVariable) - 1) { - if (!checkEquals(old.getExtension(JvmProtoBuf.packageLocalVariable, i), new.getExtension(JvmProtoBuf.packageLocalVariable, i))) result.add(ProtoBufPackageKind.JVM_EXT_PACKAGE_LOCAL_VARIABLE_LIST) + if (old.getExtensionCount(JvmProtoBuf.packageLocalVariable) != new.getExtensionCount(JvmProtoBuf.packageLocalVariable)) { + result.add(ProtoBufPackageKind.JVM_EXT_PACKAGE_LOCAL_VARIABLE_LIST) + } + else { + for(i in 0..old.getExtensionCount(JvmProtoBuf.packageLocalVariable) - 1) { + if (!checkEquals(old.getExtension(JvmProtoBuf.packageLocalVariable, i), new.getExtension(JvmProtoBuf.packageLocalVariable, i))) result.add(ProtoBufPackageKind.JVM_EXT_PACKAGE_LOCAL_VARIABLE_LIST) + } } if (old.hasExtension(JsProtoBuf.packageFqName) != new.hasExtension(JsProtoBuf.packageFqName)) result.add(ProtoBufPackageKind.JS_EXT_PACKAGE_FQ_NAME) @@ -172,16 +178,22 @@ open class ProtoCompareGenerated(val oldNameResolver: NameResolver, val newNameR if (!checkStringEquals(old.getExtension(JvmProtoBuf.classModuleName), new.getExtension(JvmProtoBuf.classModuleName))) return false } - if (old.getExtensionCount(JvmProtoBuf.classLocalVariable) != new.getExtensionCount(JvmProtoBuf.classLocalVariable)) return false - - for(i in 0..old.getExtensionCount(JvmProtoBuf.classLocalVariable) - 1) { - if (!checkEquals(old.getExtension(JvmProtoBuf.classLocalVariable, i), new.getExtension(JvmProtoBuf.classLocalVariable, i))) return false + if (old.getExtensionCount(JvmProtoBuf.classLocalVariable) != new.getExtensionCount(JvmProtoBuf.classLocalVariable)) { + return false + } + else { + for(i in 0..old.getExtensionCount(JvmProtoBuf.classLocalVariable) - 1) { + if (!checkEquals(old.getExtension(JvmProtoBuf.classLocalVariable, i), new.getExtension(JvmProtoBuf.classLocalVariable, i))) return false + } } - if (old.getExtensionCount(JsProtoBuf.classAnnotation) != new.getExtensionCount(JsProtoBuf.classAnnotation)) return false - - for(i in 0..old.getExtensionCount(JsProtoBuf.classAnnotation) - 1) { - if (!checkEquals(old.getExtension(JsProtoBuf.classAnnotation, i), new.getExtension(JsProtoBuf.classAnnotation, i))) return false + if (old.getExtensionCount(JsProtoBuf.classAnnotation) != new.getExtensionCount(JsProtoBuf.classAnnotation)) { + return false + } + else { + for(i in 0..old.getExtensionCount(JsProtoBuf.classAnnotation) - 1) { + if (!checkEquals(old.getExtension(JsProtoBuf.classAnnotation, i), new.getExtension(JsProtoBuf.classAnnotation, i))) return false + } } if (old.hasExtension(JsProtoBuf.classContainingFileId) != new.hasExtension(JsProtoBuf.classContainingFileId)) return false @@ -269,16 +281,22 @@ open class ProtoCompareGenerated(val oldNameResolver: NameResolver, val newNameR if (!checkStringEquals(old.getExtension(JvmProtoBuf.classModuleName), new.getExtension(JvmProtoBuf.classModuleName))) result.add(ProtoBufClassKind.JVM_EXT_CLASS_MODULE_NAME) } - if (old.getExtensionCount(JvmProtoBuf.classLocalVariable) != new.getExtensionCount(JvmProtoBuf.classLocalVariable)) result.add(ProtoBufClassKind.JVM_EXT_CLASS_LOCAL_VARIABLE_LIST) - - for(i in 0..old.getExtensionCount(JvmProtoBuf.classLocalVariable) - 1) { - if (!checkEquals(old.getExtension(JvmProtoBuf.classLocalVariable, i), new.getExtension(JvmProtoBuf.classLocalVariable, i))) result.add(ProtoBufClassKind.JVM_EXT_CLASS_LOCAL_VARIABLE_LIST) + if (old.getExtensionCount(JvmProtoBuf.classLocalVariable) != new.getExtensionCount(JvmProtoBuf.classLocalVariable)) { + result.add(ProtoBufClassKind.JVM_EXT_CLASS_LOCAL_VARIABLE_LIST) + } + else { + for(i in 0..old.getExtensionCount(JvmProtoBuf.classLocalVariable) - 1) { + if (!checkEquals(old.getExtension(JvmProtoBuf.classLocalVariable, i), new.getExtension(JvmProtoBuf.classLocalVariable, i))) result.add(ProtoBufClassKind.JVM_EXT_CLASS_LOCAL_VARIABLE_LIST) + } } - if (old.getExtensionCount(JsProtoBuf.classAnnotation) != new.getExtensionCount(JsProtoBuf.classAnnotation)) result.add(ProtoBufClassKind.JS_EXT_CLASS_ANNOTATION_LIST) - - for(i in 0..old.getExtensionCount(JsProtoBuf.classAnnotation) - 1) { - if (!checkEquals(old.getExtension(JsProtoBuf.classAnnotation, i), new.getExtension(JsProtoBuf.classAnnotation, i))) result.add(ProtoBufClassKind.JS_EXT_CLASS_ANNOTATION_LIST) + if (old.getExtensionCount(JsProtoBuf.classAnnotation) != new.getExtensionCount(JsProtoBuf.classAnnotation)) { + result.add(ProtoBufClassKind.JS_EXT_CLASS_ANNOTATION_LIST) + } + else { + for(i in 0..old.getExtensionCount(JsProtoBuf.classAnnotation) - 1) { + if (!checkEquals(old.getExtension(JsProtoBuf.classAnnotation, i), new.getExtension(JsProtoBuf.classAnnotation, i))) result.add(ProtoBufClassKind.JS_EXT_CLASS_ANNOTATION_LIST) + } } if (old.hasExtension(JsProtoBuf.classContainingFileId) != new.hasExtension(JsProtoBuf.classContainingFileId)) result.add(ProtoBufClassKind.JS_EXT_CLASS_CONTAINING_FILE_ID) @@ -341,10 +359,13 @@ open class ProtoCompareGenerated(val oldNameResolver: NameResolver, val newNameR if (!checkEquals(old.getExtension(JvmProtoBuf.methodSignature), new.getExtension(JvmProtoBuf.methodSignature))) return false } - if (old.getExtensionCount(JsProtoBuf.functionAnnotation) != new.getExtensionCount(JsProtoBuf.functionAnnotation)) return false - - for(i in 0..old.getExtensionCount(JsProtoBuf.functionAnnotation) - 1) { - if (!checkEquals(old.getExtension(JsProtoBuf.functionAnnotation, i), new.getExtension(JsProtoBuf.functionAnnotation, i))) return false + if (old.getExtensionCount(JsProtoBuf.functionAnnotation) != new.getExtensionCount(JsProtoBuf.functionAnnotation)) { + return false + } + else { + for(i in 0..old.getExtensionCount(JsProtoBuf.functionAnnotation) - 1) { + if (!checkEquals(old.getExtension(JsProtoBuf.functionAnnotation, i), new.getExtension(JsProtoBuf.functionAnnotation, i))) return false + } } if (old.hasExtension(JsProtoBuf.functionContainingFileId) != new.hasExtension(JsProtoBuf.functionContainingFileId)) return false @@ -415,10 +436,13 @@ open class ProtoCompareGenerated(val oldNameResolver: NameResolver, val newNameR if (!checkEquals(old.getExtension(JvmProtoBuf.propertySignature), new.getExtension(JvmProtoBuf.propertySignature))) return false } - if (old.getExtensionCount(JsProtoBuf.propertyAnnotation) != new.getExtensionCount(JsProtoBuf.propertyAnnotation)) return false - - for(i in 0..old.getExtensionCount(JsProtoBuf.propertyAnnotation) - 1) { - if (!checkEquals(old.getExtension(JsProtoBuf.propertyAnnotation, i), new.getExtension(JsProtoBuf.propertyAnnotation, i))) return false + if (old.getExtensionCount(JsProtoBuf.propertyAnnotation) != new.getExtensionCount(JsProtoBuf.propertyAnnotation)) { + return false + } + else { + for(i in 0..old.getExtensionCount(JsProtoBuf.propertyAnnotation) - 1) { + if (!checkEquals(old.getExtension(JsProtoBuf.propertyAnnotation, i), new.getExtension(JsProtoBuf.propertyAnnotation, i))) return false + } } if (old.hasExtension(JsProtoBuf.compileTimeValue) != new.hasExtension(JsProtoBuf.compileTimeValue)) return false @@ -510,16 +534,22 @@ open class ProtoCompareGenerated(val oldNameResolver: NameResolver, val newNameR if (!checkEqualsTypeParameterUpperBoundId(old, new)) return false - if (old.getExtensionCount(JvmProtoBuf.typeParameterAnnotation) != new.getExtensionCount(JvmProtoBuf.typeParameterAnnotation)) return false - - for(i in 0..old.getExtensionCount(JvmProtoBuf.typeParameterAnnotation) - 1) { - if (!checkEquals(old.getExtension(JvmProtoBuf.typeParameterAnnotation, i), new.getExtension(JvmProtoBuf.typeParameterAnnotation, i))) return false + if (old.getExtensionCount(JvmProtoBuf.typeParameterAnnotation) != new.getExtensionCount(JvmProtoBuf.typeParameterAnnotation)) { + return false + } + else { + for(i in 0..old.getExtensionCount(JvmProtoBuf.typeParameterAnnotation) - 1) { + if (!checkEquals(old.getExtension(JvmProtoBuf.typeParameterAnnotation, i), new.getExtension(JvmProtoBuf.typeParameterAnnotation, i))) return false + } } - if (old.getExtensionCount(JsProtoBuf.typeParameterAnnotation) != new.getExtensionCount(JsProtoBuf.typeParameterAnnotation)) return false - - for(i in 0..old.getExtensionCount(JsProtoBuf.typeParameterAnnotation) - 1) { - if (!checkEquals(old.getExtension(JsProtoBuf.typeParameterAnnotation, i), new.getExtension(JsProtoBuf.typeParameterAnnotation, i))) return false + if (old.getExtensionCount(JsProtoBuf.typeParameterAnnotation) != new.getExtensionCount(JsProtoBuf.typeParameterAnnotation)) { + return false + } + else { + for(i in 0..old.getExtensionCount(JsProtoBuf.typeParameterAnnotation) - 1) { + if (!checkEquals(old.getExtension(JsProtoBuf.typeParameterAnnotation, i), new.getExtension(JsProtoBuf.typeParameterAnnotation, i))) return false + } } return true @@ -593,10 +623,13 @@ open class ProtoCompareGenerated(val oldNameResolver: NameResolver, val newNameR if (old.flags != new.flags) return false } - if (old.getExtensionCount(JvmProtoBuf.typeAnnotation) != new.getExtensionCount(JvmProtoBuf.typeAnnotation)) return false - - for(i in 0..old.getExtensionCount(JvmProtoBuf.typeAnnotation) - 1) { - if (!checkEquals(old.getExtension(JvmProtoBuf.typeAnnotation, i), new.getExtension(JvmProtoBuf.typeAnnotation, i))) return false + if (old.getExtensionCount(JvmProtoBuf.typeAnnotation) != new.getExtensionCount(JvmProtoBuf.typeAnnotation)) { + return false + } + else { + for(i in 0..old.getExtensionCount(JvmProtoBuf.typeAnnotation) - 1) { + if (!checkEquals(old.getExtension(JvmProtoBuf.typeAnnotation, i), new.getExtension(JvmProtoBuf.typeAnnotation, i))) return false + } } if (old.hasExtension(JvmProtoBuf.isRaw) != new.hasExtension(JvmProtoBuf.isRaw)) return false @@ -604,10 +637,13 @@ open class ProtoCompareGenerated(val oldNameResolver: NameResolver, val newNameR if (old.getExtension(JvmProtoBuf.isRaw) != new.getExtension(JvmProtoBuf.isRaw)) return false } - if (old.getExtensionCount(JsProtoBuf.typeAnnotation) != new.getExtensionCount(JsProtoBuf.typeAnnotation)) return false - - for(i in 0..old.getExtensionCount(JsProtoBuf.typeAnnotation) - 1) { - if (!checkEquals(old.getExtension(JsProtoBuf.typeAnnotation, i), new.getExtension(JsProtoBuf.typeAnnotation, i))) return false + if (old.getExtensionCount(JsProtoBuf.typeAnnotation) != new.getExtensionCount(JsProtoBuf.typeAnnotation)) { + return false + } + else { + for(i in 0..old.getExtensionCount(JsProtoBuf.typeAnnotation) - 1) { + if (!checkEquals(old.getExtension(JsProtoBuf.typeAnnotation, i), new.getExtension(JsProtoBuf.typeAnnotation, i))) return false + } } return true @@ -631,10 +667,13 @@ open class ProtoCompareGenerated(val oldNameResolver: NameResolver, val newNameR if (!checkEquals(old.getExtension(JvmProtoBuf.constructorSignature), new.getExtension(JvmProtoBuf.constructorSignature))) return false } - if (old.getExtensionCount(JsProtoBuf.constructorAnnotation) != new.getExtensionCount(JsProtoBuf.constructorAnnotation)) return false - - for(i in 0..old.getExtensionCount(JsProtoBuf.constructorAnnotation) - 1) { - if (!checkEquals(old.getExtension(JsProtoBuf.constructorAnnotation, i), new.getExtension(JsProtoBuf.constructorAnnotation, i))) return false + if (old.getExtensionCount(JsProtoBuf.constructorAnnotation) != new.getExtensionCount(JsProtoBuf.constructorAnnotation)) { + return false + } + else { + for(i in 0..old.getExtensionCount(JsProtoBuf.constructorAnnotation) - 1) { + if (!checkEquals(old.getExtension(JsProtoBuf.constructorAnnotation, i), new.getExtension(JsProtoBuf.constructorAnnotation, i))) return false + } } return true @@ -646,10 +685,13 @@ open class ProtoCompareGenerated(val oldNameResolver: NameResolver, val newNameR if (!checkStringEquals(old.name, new.name)) return false } - if (old.getExtensionCount(JsProtoBuf.enumEntryAnnotation) != new.getExtensionCount(JsProtoBuf.enumEntryAnnotation)) return false - - for(i in 0..old.getExtensionCount(JsProtoBuf.enumEntryAnnotation) - 1) { - if (!checkEquals(old.getExtension(JsProtoBuf.enumEntryAnnotation, i), new.getExtension(JsProtoBuf.enumEntryAnnotation, i))) return false + if (old.getExtensionCount(JsProtoBuf.enumEntryAnnotation) != new.getExtensionCount(JsProtoBuf.enumEntryAnnotation)) { + return false + } + else { + for(i in 0..old.getExtensionCount(JsProtoBuf.enumEntryAnnotation) - 1) { + if (!checkEquals(old.getExtension(JsProtoBuf.enumEntryAnnotation, i), new.getExtension(JsProtoBuf.enumEntryAnnotation, i))) return false + } } return true @@ -691,10 +733,13 @@ open class ProtoCompareGenerated(val oldNameResolver: NameResolver, val newNameR if (old.varargElementTypeId != new.varargElementTypeId) return false } - if (old.getExtensionCount(JsProtoBuf.parameterAnnotation) != new.getExtensionCount(JsProtoBuf.parameterAnnotation)) return false - - for(i in 0..old.getExtensionCount(JsProtoBuf.parameterAnnotation) - 1) { - if (!checkEquals(old.getExtension(JsProtoBuf.parameterAnnotation, i), new.getExtension(JsProtoBuf.parameterAnnotation, i))) return false + if (old.getExtensionCount(JsProtoBuf.parameterAnnotation) != new.getExtensionCount(JsProtoBuf.parameterAnnotation)) { + return false + } + else { + for(i in 0..old.getExtensionCount(JsProtoBuf.parameterAnnotation) - 1) { + if (!checkEquals(old.getExtension(JsProtoBuf.parameterAnnotation, i), new.getExtension(JsProtoBuf.parameterAnnotation, i))) return false + } } return true diff --git a/generators/src/org/jetbrains/kotlin/generators/protobuf/GenerateProtoBufCompare.kt b/generators/src/org/jetbrains/kotlin/generators/protobuf/GenerateProtoBufCompare.kt index 68e824f1d57..616fd73b909 100644 --- a/generators/src/org/jetbrains/kotlin/generators/protobuf/GenerateProtoBufCompare.kt +++ b/generators/src/org/jetbrains/kotlin/generators/protobuf/GenerateProtoBufCompare.kt @@ -342,11 +342,16 @@ class GenerateProtoBufCompare { val fullFieldName = "$outerClassName.$fieldName" override fun printRepeatedField() { - p.println("if (old.getExtensionCount($fullFieldName) != new.getExtensionCount($fullFieldName)) $statement") - p.println() - p.println("for(i in 0..old.getExtensionCount($fullFieldName) - 1) {") - p.printlnIfWithComparisonIndent(field, "getExtension($fullFieldName, i)", statement) - p.println("}") + p.printlnMultiline(""" + if (old.getExtensionCount($fullFieldName) != new.getExtensionCount($fullFieldName)) { + $statement + } + else { + for(i in 0..old.getExtensionCount($fullFieldName) - 1) { + ${ifWithComparison(field, "getExtension($fullFieldName, i)", statement)} + } + } + """) } override fun printRequiredField() { @@ -390,19 +395,21 @@ class GenerateProtoBufCompare { } } + private fun ifWithComparison(field: Descriptors.FieldDescriptor, expr: String, statement: String) = + when { + field.options.getExtension(DebugExtOptionsProtoBuf.stringIdInTable) || + field.options.getExtension(DebugExtOptionsProtoBuf.nameIdInTable) -> + "if (!$CHECK_STRING_EQUALS_NAME(old.$expr, new.$expr)) $statement" + field.options.getExtension(DebugExtOptionsProtoBuf.fqNameIdInTable) -> + "if (!$CHECK_CLASS_ID_EQUALS_NAME(old.$expr, new.$expr)) $statement" + field.javaType in JAVA_TYPES_WITH_INLINED_EQUALS -> + "if (old.$expr != new.$expr) $statement" + else -> + "if (!$CHECK_EQUALS_NAME(old.$expr, new.$expr)) $statement" + } + private fun Printer.printlnIfWithComparison(field: Descriptors.FieldDescriptor, expr: String, statement: String = "return false") { - val line = when { - field.options.getExtension(DebugExtOptionsProtoBuf.stringIdInTable) || - field.options.getExtension(DebugExtOptionsProtoBuf.nameIdInTable) -> - "if (!$CHECK_STRING_EQUALS_NAME(old.$expr, new.$expr)) $statement" - field.options.getExtension(DebugExtOptionsProtoBuf.fqNameIdInTable) -> - "if (!$CHECK_CLASS_ID_EQUALS_NAME(old.$expr, new.$expr)) $statement" - field.javaType in JAVA_TYPES_WITH_INLINED_EQUALS -> - "if (old.$expr != new.$expr) $statement" - else -> - "if (!$CHECK_EQUALS_NAME(old.$expr, new.$expr)) $statement" - } - this.println(line) + this.println(ifWithComparison(field, expr, statement)) } fun Printer.printlnIfWithComparisonIndent(field: Descriptors.FieldDescriptor, expr: String, statement: String = "return false") { @@ -458,3 +465,7 @@ class GenerateProtoBufCompare { private val String.javaName: String get() = this.split("_").joinToString("") { it.capitalize() }.decapitalize() } + +private fun Printer.printlnMultiline(string: String) { + string.trimIndent().split("\n").forEach { this.println(it) } +} \ No newline at end of file