From d7c397b9abf91b41138e31b98778c5dc40e4b227 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20Quenaudon?= Date: Thu, 20 Jun 2024 12:58:18 +0100 Subject: [PATCH] Allow edition parsing defaulting to proto2 behavior --- .../squareup/wire/kotlin/KotlinGenerator.kt | 4 +- .../kotlin/com/squareup/wire/Syntax.kt | 49 +++++++++++++++---- .../com/squareup/wire/schema/SyntaxRules.kt | 5 +- .../internal/parser/ProtoFileElement.kt | 7 ++- .../schema/internal/parser/ProtoParser.kt | 8 +-- .../schema/internal/parser/ProtoParserTest.kt | 47 +++++++++++++++++- .../com/squareup/wire/swift/SwiftGenerator.kt | 21 ++++++-- 7 files changed, 116 insertions(+), 25 deletions(-) diff --git a/wire-kotlin-generator/src/main/java/com/squareup/wire/kotlin/KotlinGenerator.kt b/wire-kotlin-generator/src/main/java/com/squareup/wire/kotlin/KotlinGenerator.kt index 432c112678..9ef5ccefb4 100644 --- a/wire-kotlin-generator/src/main/java/com/squareup/wire/kotlin/KotlinGenerator.kt +++ b/wire-kotlin-generator/src/main/java/com/squareup/wire/kotlin/KotlinGenerator.kt @@ -1555,7 +1555,7 @@ class KotlinGenerator private constructor( .addSuperclassConstructorParameter("\n%S", type.type.typeUrl!!) .addSuperclassConstructorParameter( "\n%M", - MemberName(Syntax::class.asClassName(), type.syntax.name), + MemberName(Syntax::class.asClassName(), type.syntax.name()), ) .addSuperclassConstructorParameter("\nnull") .addSuperclassConstructorParameter("\n%S\n⇤", type.location.path) @@ -2469,7 +2469,7 @@ class KotlinGenerator private constructor( .addSuperclassConstructorParameter("\n⇥%T::class", parentClassName) .addSuperclassConstructorParameter( "\n%M", - MemberName(Syntax::class.asClassName(), message.syntax.name), + MemberName(Syntax::class.asClassName(), message.syntax.name()), ) .addSuperclassConstructorParameter("\n%L\n⇤", message.identity()) .addFunction( diff --git a/wire-runtime/src/commonMain/kotlin/com/squareup/wire/Syntax.kt b/wire-runtime/src/commonMain/kotlin/com/squareup/wire/Syntax.kt index 72a983ff55..391035ea80 100644 --- a/wire-runtime/src/commonMain/kotlin/com/squareup/wire/Syntax.kt +++ b/wire-runtime/src/commonMain/kotlin/com/squareup/wire/Syntax.kt @@ -15,20 +15,51 @@ */ package com.squareup.wire -/** Syntax version. */ -enum class Syntax(private val string: String) { - PROTO_2("proto2"), - PROTO_3("proto3"), - ; +/** Syntax and edition version. */ +sealed class Syntax(internal val string: String) { + data object PROTO_2 : Syntax("proto2") { + override fun toString(): String = string + } + data object PROTO_3 : Syntax("proto3") { + override fun toString(): String = string + } + data class Edition(val value: String) : Syntax(value) { + override fun toString(): String = value + } - override fun toString(): String = string + // Created for backward capability with when Syntax used to be an enum. + fun name(): String { + return when (this) { + // TODO(Benoit) Edition needs to return something like `Edition()`. + is Edition, + PROTO_2, + -> "PROTO_2" + PROTO_3 -> "PROTO_3" + } + } companion object { + @Deprecated("Use get(string: String, edition: Boolean) instead", ReplaceWith("Syntax.get(string, edition)")) operator fun get(string: String): Syntax { - for (syntax in values()) { - if (syntax.string == string) return syntax - } + if (string == PROTO_2.toString()) return PROTO_2 + if (string == PROTO_3.toString()) return PROTO_3 throw IllegalArgumentException("unexpected syntax: $string") } + + fun get(string: String, edition: Boolean): Syntax { + if (edition) { + if (string in KNOWN_EDITIONS) return Edition(string) + throw IllegalArgumentException("unknown edition: $string") + } else { + if (string == PROTO_2.toString()) return PROTO_2 + if (string == PROTO_3.toString()) return PROTO_3 + throw IllegalArgumentException("unexpected syntax: $string") + } + } + + private val KNOWN_EDITIONS = listOf( + "2023", + "2024", + ) } } diff --git a/wire-schema/src/commonMain/kotlin/com/squareup/wire/schema/SyntaxRules.kt b/wire-schema/src/commonMain/kotlin/com/squareup/wire/schema/SyntaxRules.kt index 21160b5902..2858f70c15 100644 --- a/wire-schema/src/commonMain/kotlin/com/squareup/wire/schema/SyntaxRules.kt +++ b/wire-schema/src/commonMain/kotlin/com/squareup/wire/schema/SyntaxRules.kt @@ -40,8 +40,9 @@ interface SyntaxRules { companion object { fun get(syntax: Syntax?): SyntaxRules { return when (syntax) { - PROTO_3 -> PROTO_3_SYNTAX_RULES - PROTO_2, + is PROTO_3 -> PROTO_3_SYNTAX_RULES + is Syntax.Edition, + is PROTO_2, null, -> PROTO_2_SYNTAX_RULES } diff --git a/wire-schema/src/commonMain/kotlin/com/squareup/wire/schema/internal/parser/ProtoFileElement.kt b/wire-schema/src/commonMain/kotlin/com/squareup/wire/schema/internal/parser/ProtoFileElement.kt index af2bbce211..dbf3850f49 100644 --- a/wire-schema/src/commonMain/kotlin/com/squareup/wire/schema/internal/parser/ProtoFileElement.kt +++ b/wire-schema/src/commonMain/kotlin/com/squareup/wire/schema/internal/parser/ProtoFileElement.kt @@ -16,6 +16,7 @@ package com.squareup.wire.schema.internal.parser import com.squareup.wire.Syntax +import com.squareup.wire.Syntax.Edition import com.squareup.wire.schema.Location import kotlin.jvm.JvmStatic @@ -37,7 +38,11 @@ data class ProtoFileElement( if (syntax != null) { append('\n') - append("syntax = \"$syntax\";\n") + when (syntax) { + is Edition -> append("edition") + else -> append("syntax") + } + append(" = \"$syntax\";\n") } if (packageName != null) { append('\n') diff --git a/wire-schema/src/commonMain/kotlin/com/squareup/wire/schema/internal/parser/ProtoParser.kt b/wire-schema/src/commonMain/kotlin/com/squareup/wire/schema/internal/parser/ProtoParser.kt index e010a188eb..fbaa187b8a 100644 --- a/wire-schema/src/commonMain/kotlin/com/squareup/wire/schema/internal/parser/ProtoParser.kt +++ b/wire-schema/src/commonMain/kotlin/com/squareup/wire/schema/internal/parser/ProtoParser.kt @@ -121,15 +121,15 @@ class ProtoParser internal constructor( null } - label == "syntax" && context.permitsSyntax() -> { - reader.expect(syntax == null, location) { "too many syntax definitions" } + (label == "syntax" || label == "edition") && context.permitsSyntax() -> { + reader.expect(syntax == null, location) { "too many syntax or edition definitions" } reader.require('=') reader.expect(index == 0, location) { - "'syntax' element must be the first declaration in a file" + "'syntax' or 'edition' element must be the first declaration in a file" } val syntaxString = reader.readQuotedString() try { - syntax = Syntax[syntaxString] + syntax = Syntax.get(syntaxString, edition = label == "edition") } catch (e: IllegalArgumentException) { throw reader.unexpected(e.message!!, location) } diff --git a/wire-schema/src/commonTest/kotlin/com/squareup/wire/schema/internal/parser/ProtoParserTest.kt b/wire-schema/src/commonTest/kotlin/com/squareup/wire/schema/internal/parser/ProtoParserTest.kt index 9d081abf53..eccf91578b 100644 --- a/wire-schema/src/commonTest/kotlin/com/squareup/wire/schema/internal/parser/ProtoParserTest.kt +++ b/wire-schema/src/commonTest/kotlin/com/squareup/wire/schema/internal/parser/ProtoParserTest.kt @@ -21,6 +21,7 @@ import assertk.assertions.hasMessage import assertk.assertions.isEqualTo import assertk.assertions.isNull import assertk.assertions.messageContains +import com.squareup.wire.Syntax.Edition import com.squareup.wire.Syntax.PROTO_2 import com.squareup.wire.Syntax.PROTO_3 import com.squareup.wire.schema.Field.Label.OPTIONAL @@ -627,6 +628,48 @@ class ProtoParserTest { assertThat(parsed.syntax).isNull() } + @Test + fun editionParsing() { + val proto = """ + |edition = "2024"; + |message Foo {} + """.trimMargin() + val parsed = ProtoParser.parse(location, proto) + assertThat(parsed.syntax).isEqualTo(Edition("2024")) + } + + @Test + fun syntaxAndEditionConflictAreNotAllowed() { + val proto = """ + |edition = "2024"; + |syntax = "proto2"; + |message Foo {} + """.trimMargin() + try { + ProtoParser.parse(location, proto) + fail() + } catch (e: IllegalStateException) { + assertThat(e).hasMessage("Syntax error in file.proto:2:1: too many syntax or edition definitions") + } + } + + @Test + fun rejectUnknownEditions() { + for (edition in listOf("something", "EDITION_2023", "2025")) { + val proto = """ + |edition = "2025"; + |syntax = "proto2"; + |message Foo {} + """.trimMargin() + try { + ProtoParser.parse(location, proto) + fail("Excepted to fail for edition $edition") + } catch (e: IllegalStateException) { + assertThat(e).hasMessage("Syntax error in file.proto:1:1: unknown edition: 2025") + } + } + } + @Test fun syntaxSpecified() { val proto = """ @@ -671,7 +714,7 @@ class ProtoParserTest { fail() } catch (expected: IllegalStateException) { assertThat(expected).hasMessage( - "Syntax error in file.proto:2:1: 'syntax' element must be the first declaration in a file", + "Syntax error in file.proto:2:1: 'syntax' or 'edition' element must be the first declaration in a file", ) } } @@ -3174,7 +3217,7 @@ class ProtoParserTest { fail() } catch (expected: IllegalStateException) { assertThat(expected) - .messageContains("Syntax error in file.proto:2:3: too many syntax definitions") + .messageContains("Syntax error in file.proto:2:3: too many syntax or edition definitions") } } diff --git a/wire-swift-generator/src/main/java/com/squareup/wire/swift/SwiftGenerator.kt b/wire-swift-generator/src/main/java/com/squareup/wire/swift/SwiftGenerator.kt index ebd9dfa81a..cb7aa30b86 100644 --- a/wire-swift-generator/src/main/java/com/squareup/wire/swift/SwiftGenerator.kt +++ b/wire-swift-generator/src/main/java/com/squareup/wire/swift/SwiftGenerator.kt @@ -15,6 +15,7 @@ */ package com.squareup.wire.swift +import com.squareup.wire.Syntax.Edition import com.squareup.wire.Syntax.PROTO_2 import com.squareup.wire.Syntax.PROTO_3 import com.squareup.wire.internal.camelCase @@ -601,7 +602,9 @@ class SwiftGenerator private constructor( // Declare locals into which everything is written before promoting to members. type.declaredFields.forEach { field -> val localType = when (type.syntax) { - PROTO_2 -> if (field.isRepeated || field.isMap) { + is Edition, + PROTO_2, + -> if (field.isRepeated || field.isMap) { field.typeName } else { field.typeName.makeOptional() @@ -614,7 +617,9 @@ class SwiftGenerator private constructor( } val initializer = when (type.syntax) { - PROTO_2 -> when { + is Edition, + PROTO_2, + -> when { field.isMap -> "[:]" field.isRepeated -> "[]" else -> "nil" @@ -696,7 +701,9 @@ class SwiftGenerator private constructor( val hasPropertyWrapper = !isIndirect(type, field) && (field.defaultedValue != null || field.isProtoDefaulted) val initializer = when (type.syntax) { - PROTO_2 -> if (field.isOptional || field.isRepeated || field.isMap) { + is Edition, + PROTO_2, + -> if (field.isOptional || field.isRepeated || field.isMap) { CodeBlock.of("%N", field.safeName) } else { CodeBlock.of("try %1T.checkIfMissing(%2N, %3S)", structType, field.safeName, field.name) @@ -780,13 +787,17 @@ class SwiftGenerator private constructor( private val MessageType.protoCodableType: DeclaredTypeName get() = when (syntax) { - PROTO_2 -> proto2Codable + is Edition, + PROTO_2, + -> proto2Codable PROTO_3 -> proto3Codable } private val EnumType.protoCodableType: DeclaredTypeName get() = when (syntax) { - PROTO_2 -> proto2Enum + is Edition, + PROTO_2, + -> proto2Enum PROTO_3 -> proto3Enum }