Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix tags in codegen for large field numbers #213

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,10 @@ allprojects {
"examples/protos/src/main/proto/io/grpc/examples/route_guide.proto",
"testing/conformance/driver/src/main/proto/conformance/conformance.proto",
"testing/conformance/driver/src/main/proto/proto3/test_messages_proto3.proto",
"testing/interop/src/main/proto/tutorial/addressbook.proto"
"testing/interop/src/main/proto/tutorial/addressbook.proto",
"testing/interop/src/main/proto/google/protobuf/unittest_import.proto",
"testing/interop/src/main/proto/google/protobuf/unittest_import_public.proto",
"testing/interop/src/main/proto/google/protobuf/unittest_proto3.proto",
).map(rootProject::file) +
"node_modules/**" +
"**/build/extracted-include-protos/**" +
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,11 +35,7 @@ object CodeGenerator {

fun generate(contents: ProtoFileContents) =
contents.types.flatMap {
generate(
it,
Context(emptyList(), contents.info)
)
.map { type -> GeneratedType(it, type) }
generate(it, Context(emptyList(), contents.info)).map(::GeneratedType)
}

fun generate(type: TopLevelType, ctx: Context): Iterable<TypeSpec> =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ private class DeserializerGenerator(
beginControlFlow("when (deserializer.readTag())")
val constructor =
buildCodeBlock {
add("0·->·return·%T(\n", msg.className)
add("0u·->·return·%T(\n", msg.className)
withIndent {
constructorLines(properties).forEach(::add)
}
Expand All @@ -92,7 +92,7 @@ private class DeserializerGenerator(
addStatement("%L", constructor)
deserializerInfo.forEach {
addStatement(
"%L -> %N = %L",
"%Lu -> %N = %L",
it.tag,
it.fieldName,
it.value
Expand Down Expand Up @@ -156,7 +156,7 @@ private class DeserializerGenerator(
CodeBlock.of("%T.from(unknownFields)", UnknownFieldSet::class)

private class DeserializerInfo(
val tag: Int,
val tag: UInt,
val fieldName: String,
val value: CodeBlock
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -135,11 +135,11 @@ private class MapEntryGenerator(
beginControlFlow("when (deserializer.readTag())")
addStatement("%L", constructOnZero(value))
addStatement(
"${key.tag.value} -> key = %L",
"${key.tag.value}u -> key = %L",
deserialize(key, ctx)
)
addStatement(
"${value.tag.value} -> value = %L",
"${value.tag.value}u -> value = %L",
deserialize(value, ctx)
)
endControlFlow()
Expand Down Expand Up @@ -170,7 +170,7 @@ private class MapEntryGenerator(

private fun constructOnZero(f: StandardField) =
buildCodeBlock {
add("0 -> return %T(key, value", msg.className)
add("0u -> return %T(key, value", msg.className)
if (f.type == FieldType.Message) {
add(" ?: %T{}", value.className)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -166,11 +166,10 @@ class ProtoFileContents(
)

class GeneratedType(
val rawType: TopLevelType,
val typeSpec: TypeSpec
)

sealed class Tag(val value: Int) : Comparable<Tag> {
sealed class Tag(val value: UInt) : Comparable<Tag> {
class Packed(
number: Int
) : Tag(computeTag(number, 2))
Expand All @@ -188,4 +187,4 @@ sealed class Tag(val value: Int) : Comparable<Tag> {
}

private fun computeTag(fieldNumber: Int, wireType: Int) =
(fieldNumber shl 3) or wireType
(fieldNumber shl 3).toUInt() or wireType.toUInt()
2 changes: 1 addition & 1 deletion protokt-runtime/api/protokt-runtime.api
Original file line number Diff line number Diff line change
Expand Up @@ -595,7 +595,7 @@ public abstract interface class protokt/v1/KtMessageDeserializer {
public abstract fun readSInt32 ()I
public abstract fun readSInt64 ()J
public abstract fun readString ()Ljava/lang/String;
public abstract fun readTag ()I
public abstract fun readTag-pVg5ArA ()I
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UInt nonsense.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this ABI going to be stable?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know. We already do this below - can we merge and revisit pre-1.0?

public fun readUInt32-pVg5ArA ()I
public abstract fun readUInt64-s-VKNKU ()J
public abstract fun readUnknown ()Lprotokt/v1/UnknownField;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ interface KtMessageDeserializer {
fun readSInt64(): Long
fun readString(): String
fun readUInt64(): ULong
fun readTag(): Int
fun readTag(): UInt
fun readUnknown(): UnknownField
fun readRepeated(packed: Boolean, acc: KtMessageDeserializer.() -> Unit)
fun <T : KtMessage> readMessage(m: KtDeserializer<T>): T
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ package protokt.v1

internal fun deserializer(reader: Reader): KtMessageDeserializer {
return object : KtMessageDeserializer {
var lastTag = 0
var lastTag = 0u
var endPosition = reader.len

override fun readDouble() =
Expand Down Expand Up @@ -53,14 +53,14 @@ internal fun deserializer(reader: Reader): KtMessageDeserializer {
override fun readUInt64() =
Long.fromProtobufJsLong(reader.uint64()).toULong()

override fun readTag(): Int {
override fun readTag(): UInt {
lastTag =
if (reader.pos == endPosition) {
0
0u
} else {
val tag = readInt32()
check(tag ushr 3 != 0) { "Invalid tag" }
tag
tag.toUInt()
}
return lastTag
}
Expand All @@ -73,7 +73,7 @@ internal fun deserializer(reader: Reader): KtMessageDeserializer {
readBytes().toBytesSlice()

override fun readUnknown(): UnknownField {
val fieldNumber = (lastTag ushr 3).toUInt()
val fieldNumber = (lastTag.toInt() ushr 3).toUInt()

return when (tagWireType(lastTag)) {
0 -> UnknownField.varint(fieldNumber, readInt64())
Expand Down Expand Up @@ -115,5 +115,5 @@ internal fun deserializer(reader: Reader): KtMessageDeserializer {
}
}

private fun tagWireType(tag: Int) =
tag and ((1 shl 3) - 1)
private fun tagWireType(tag: UInt) =
tag.toInt() and ((1 shl 3) - 1)
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@

package com.toasttab.protokt.rt

@Deprecated("use v1")
@Target(AnnotationTarget.CLASS)
annotation class KtGeneratedMessage(
/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ internal fun deserializer(
stream.readUInt64().toULong()

override fun readTag() =
stream.readTag()
stream.readTag().toUInt()

override fun readBytes() =
Bytes(stream.readByteArray())
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
// Protocol Buffers - Google's data interchange format
// Copyright 2008 Google Inc. All rights reserved.
//
// Use of this source code is governed by a BSD-style
// license that can be found in the LICENSE file or at
// https://developers.google.com/open-source/licenses/bsd

// Author: kenton@google.com (Kenton Varda)
// Based on original Protocol Buffers design by
// Sanjay Ghemawat, Jeff Dean, and others.
//
// A proto file which is imported by unittest.proto to test importing.

syntax = "proto2";

// We don't put this in a package within proto2 because we need to make sure
// that the generated code doesn't depend on being in the proto2 namespace.
// In test_util.h we do
// "using namespace unittest_import = protobuf_unittest_import".
package protobuf_unittest_import;

option optimize_for = SPEED;
option cc_enable_arenas = true;

// Exercise the java_package option.
option java_package = "com.google.protobuf.test";

// Do not set a java_outer_classname here to verify that Proto2 works without
// one.

// Test public import
import public "google/protobuf/unittest_import_public.proto";

message ImportMessage {
optional int32 d = 1;
}

enum ImportEnum {
IMPORT_FOO = 7;
IMPORT_BAR = 8;
IMPORT_BAZ = 9;
}

// To use an enum in a map, it must has the first value as 0.
enum ImportEnumForMap {
UNKNOWN = 0;
FOO = 1;
BAR = 2;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
// Protocol Buffers - Google's data interchange format
// Copyright 2008 Google Inc. All rights reserved.
//
// Use of this source code is governed by a BSD-style
// license that can be found in the LICENSE file or at
// https://developers.google.com/open-source/licenses/bsd

// Author: liujisi@google.com (Pherl Liu)

syntax = "proto2";

package protobuf_unittest_import;

option java_package = "com.google.protobuf.test";

message PublicImportMessage {
optional int32 e = 1;
}
Loading