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.

Sign up for GitHub

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

Jump to bottom

add: custom types #45

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

add: custom types #45

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
191b3c8
add: init custom types
fdymylja Dec 2, 2021
7f0ad2e
change: custom type interface
fdymylja Dec 2, 2021
d4ffa4c
add: range for custom types
fdymylja Dec 2, 2021
23fcf9c
add: finalize protoreflect
fdymylja Dec 2, 2021
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
2 changes: 1 addition & 1 deletion Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,4 +4,4 @@ pulsar:
docker build -t dev:proto-build -f Dockerfile .
docker run -v "$(CURDIR):/genproto" -w /genproto dev:proto-build ./scripts/fastreflect.sh "$(DIRECTORIES_TO_BUILD)"

.PHONY: proto fastreflection
.PHONY: pulsar
50 changes: 34 additions & 16 deletions cosmos_proto/cosmos.pulsar.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 6 additions & 1 deletion features/fastreflection/clear.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,12 @@ func (g *clearGen) genField(field *protogen.Field) {
return
}

g.P("x.", field.GoName, " = ", zeroValueForField(nil, field))
switch {
case isCustomType(field):
g.P("x.", field.GoName, ".Clear()")
default:
g.P("x.", field.GoName, " = ", zeroValueForField(nil, field))
}
}

func (g *clearGen) genNullable(field *protogen.Field) {
Expand Down
5 changes: 5 additions & 0 deletions features/fastreflection/get.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,11 @@ func (g *getGen) genFieldGetter(field *protogen.Field) {
return
}

if isCustomType(field) {
g.P("return x.", field.GoName, ".Get()")
return
}

fieldRef := "x." + field.GoName
g.P("value := ", fieldRef)
switch field.Desc.Kind() {
Expand Down
5 changes: 5 additions & 0 deletions features/fastreflection/has.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,11 @@ func (g *hasGen) genField(field *protogen.Field) {
return
}

if isCustomType(field) {
g.P("return x.", field.GoName, ".IsSet()")
return
}

switch field.Desc.Kind() {
case protoreflect.FloatKind:
g.P("return x.", field.GoName, " != ", zeroValueForField(nil, field), " || ", mathPkg.Ident("Signbit"), "(float64(x.", field.GoName, "))")
Expand Down
13 changes: 13 additions & 0 deletions features/fastreflection/range.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
package fastreflection

import (
"github.com/cosmos/cosmos-proto/cosmos_proto"
"github.com/cosmos/cosmos-proto/generator"
"google.golang.org/protobuf/compiler/protogen"
"google.golang.org/protobuf/proto"
"google.golang.org/protobuf/reflect/protoreflect"
)

Expand Down Expand Up @@ -40,6 +42,13 @@ func (g *rangeGen) genField(field *protogen.Field) {
}

switch {
case isCustomType(field):
g.P("if x.", field.GoName, ".IsSet() {")
g.P("value := x.", field.GoName, ".Get()")
g.P("if !f(", fieldDescriptorName(field), ", value) {")
g.P("return")
g.P("}")
g.P("}")
case field.Desc.IsMap():
g.P("if len(x.", field.GoName, ") != 0 {")
g.P("value := ", protoreflectPkg.Ident("ValueOfMap"), "(&", mapTypeName(field), "{m: &x.", field.GoName, "})")
Expand Down Expand Up @@ -97,6 +106,10 @@ func (g *rangeGen) genField(field *protogen.Field) {
}
}

func isCustomType(field *protogen.Field) bool {
return proto.GetExtension(field.Desc.Options(), cosmos_proto.E_GoType).(string) != ""
}

func (g *rangeGen) genOneof(field *protogen.Field) {
// we check if it was processed or not
if _, ok := g.processedOneofs[field.Oneof.GoIdent.String()]; ok {
Expand Down
10 changes: 6 additions & 4 deletions features/fastreflection/set.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,18 +41,20 @@ func (g *setGen) genComment() {
}

func (g *setGen) genField(field *protogen.Field) {
if field.Oneof != nil {
g.genOneof(field)
return
}

switch {
case field.Oneof != nil:
g.genOneof(field)
return
case field.Desc.IsMap():
g.genMap(field)
return
case field.Desc.IsList():
g.genList(field)
return
case isCustomType(field):
g.P("x.", field.GoName, ".Set(value)")
return
}

fieldRef := "x." + field.GoName
Expand Down
32 changes: 32 additions & 0 deletions features/protoc/custom_type.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
package protoc

import (
"github.com/cosmos/cosmos-proto/cosmos_proto"
"github.com/cosmos/cosmos-proto/generator"
"google.golang.org/protobuf/compiler/protogen"
"google.golang.org/protobuf/proto"
"strings"
)

func isCustomType(field *protogen.Field) bool {
fd := field.Desc
t := proto.GetExtension(fd.Options(), cosmos_proto.E_GoType).(string)
return t != ""
}

func customFieldType(g *generator.GeneratedFile, field *protogen.Field) string {
if field.Desc.IsList() || field.Desc.IsMap() {
panic("invalid custom type") // TODO(fdymylja): better err msg
}
t := proto.GetExtension(field.Desc.Options(), cosmos_proto.E_GoType).(string)
switch {
case !strings.Contains(t, "."):
return "*" + t
default:
s := strings.Split(t, ".")
typeName := s[len(s)-1]
pkg := protogen.GoImportPath(strings.Join(s[0:len(s)-1], "."))
g.Import(pkg)
return "*" + g.QualifiedGoIdent(pkg.Ident(typeName))
}
}
8 changes: 8 additions & 0 deletions features/protoc/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1688,6 +1688,11 @@ func fieldGoType(g *generator.GeneratedFile, f *fileInfo, field *protogen.Field)
}

pointer = field.Desc.HasPresence()
if isCustomType(field) {
goType = customFieldType(g, field)
return
}

switch field.Desc.Kind() {
case protoreflect.BoolKind:
goType = "bool"
Expand Down Expand Up @@ -1744,6 +1749,9 @@ func fieldDefaultValue(g *generator.GeneratedFile, f *fileInfo, m *messageInfo,
}
return defVarName
}
if isCustomType(field) {
return "nil"
}
switch field.Desc.Kind() {
case protoreflect.BoolKind:
return "false"
Expand Down
2 changes: 1 addition & 1 deletion internal/testprotos/test3/test.pulsar.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11221,7 +11221,7 @@ func (x *fastReflection_ForeignMessage) ProtoMethods() *protoiface.Methods {
// Code generated by protoc-gen-go. DO NOT EDIT.
// versions:
// protoc-gen-go v1.27.0
// protoc v3.15.7
// protoc v3.18.1
// source: internal/testprotos/test3/test.proto

const (
Expand Down
2 changes: 1 addition & 1 deletion internal/testprotos/test3/test_import.pulsar.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -374,7 +374,7 @@ func (x *fastReflection_ImportMessage) ProtoMethods() *protoiface.Methods {
// Code generated by protoc-gen-go. DO NOT EDIT.
// versions:
// protoc-gen-go v1.27.0
// protoc v3.15.7
// protoc v3.18.1
// source: internal/testprotos/test3/test_import.proto

const (
Expand Down
4 changes: 4 additions & 0 deletions proto/cosmos_proto/cosmos.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,4 +15,8 @@ extend google.protobuf.FieldOptions {
string accepts_interface = 93001;
// scalar is used to define scalar types
string scalar = 93002;

// go_type defines a custom go type for the field.
// NOTE: it is not valid for repeated and map fields.
string go_type = 93003;
}
12 changes: 12 additions & 0 deletions runtime/runtime.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,23 @@ package runtime
import (
"fmt"
"google.golang.org/protobuf/proto"
"google.golang.org/protobuf/reflect/protoreflect"
"google.golang.org/protobuf/runtime/protoiface"
"io"
"math/bits"
)

type CustomType interface {
UnmarshalBytes(in protoiface.UnmarshalInput, b []byte) (out protoiface.UnmarshalOutput, err error)
MarshalBytes(in protoiface.MarshalOutput) (out protoiface.MarshalOutput, err error)
Size(in protoiface.SizeInput) (out protoiface.SizeOutput)
Set(value protoreflect.Value)
Get() protoreflect.Value
Clear()
IsSet() bool
Mutable() protoreflect.Value
}
Comment on lines +12 to +21
Copy link
Member

Choose a reason for hiding this comment

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

Not sure we need to make it this complex. On one hand this allows caching of the protoreflect.Value + custom type, but feels like it mixes too many concerns. Can you say a little bit more about the use case for messages and Mutable?

My thinking was that we only handle string or bytes types for now because those are the only cases we have with a very simple two method Unmarshal/Marshal interface, and that a scalar can only work with one proto type (string or bytes)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can see it in this way:
Do we want a custom type to be capable of overriding a message (ex we want to override anypb.Any with pulsar.Any) ?
If this is the intention then this is the only API which does not require multiple implementations of custom type logic that can serve all purposes.
If not then we can simplify by removing unmarshal/marshal/size inputs and Mutable

Copy link
Member

@aaronc aaronc Dec 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

So as we discussed I don't think we want to extend this for full Message type overriding (for Any's we can just replace the type name).

For simple scalars, we could use something like I described in #2 for CustomStringType or CustomBytesType or even something as simple as:

type CustomType interface {
  Set(value protoreflect.Value) error
  Get() protoreflect.Value
}

Copy link
Contributor Author

@fdymylja fdymylja Dec 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

doing one custom type for each primitive is going to be a big chunk of work codegen wise (basically we need one extra check for each kind and for each method), this is why I was going for a more generalized interface. Plus custom types are going to be rare and I think they're an advanced level, so I suppose someone creating one is fully aware of protobuf semantics (like if i am customizing a string then i know bytes that are being passed are those of a string).

What if we simplified the current custom type interface to this:

type CustomType interface {
	UnmarshalBytes(b []byte) error
	MarshalBytes() ([]byte, error)
	Size() int
	Set(value protoreflect.Value)
	Get() protoreflect.Value
	Clear()
	IsSet() bool
}

Copy link
Member

@aaronc aaronc Dec 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

which approach from #2 are you thinking of going with? why do we need both Unmarshal/Marshal + Set/Get? I'm not clear as to why we couldn't deal everything with Set/Get? With Unmarshal/Marshal the type doesn't know if it was passed a string, bytes or int64 say so it's less type safe

Copy link
Contributor Author

@fdymylja fdymylja Dec 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Those methods are needed for Protoreflect.Clear/Has/Range, I need to signal to custom types that they need to be unset/cleared and I need to have a way to know if they're set or not.

Size is needed for Marshal to compute the actual size.

I wouldn't need those if custom types were simply aliases of primitives, but since we plan to substitute string with a struct (case of sdk.Int for example), we need those methods otherwise the code wouldn't know how to unset them.

Using nil is not viable (in case of fields substituted by structs) because they would break proto presence (if a type is not: optional, repeated, map or message then we cannot determine presence) - and also if we wanted to use nil then we'd need custom logic for struct custom types and for example non struct ones (Such as address being a string converted into []byte)

Copy link
Member

Choose a reason for hiding this comment

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

right, but if we go with the second approach in #2 we don't need those.

I guess I'm thinking with whichever approach we take we do want the underlying proto value to get cached either by the proto message itself or the custom type

Copy link
Contributor Author

@fdymylja fdymylja Dec 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

using the 1st approach is simpler to implement (from codegen perspective).

second approach would need more time because generating that logic is a lot more complex, and needs to be handled with a case by case basis in every method.

Also I think both obtain the same result, with the custom type implementation being a little harder to implement (in case 1) - but I think that's fine, I don't expect every dev to want to implement its own custom type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also another reason I'd go with 1) is because in the sdk that's how we deal with custom types currently. I think the API is more dev friendly and less breaking.

Copy link
Member

Choose a reason for hiding this comment

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

do you think it's reasonable to expect custom types to cache the protoreflect.Value?

second approach would need more time because generating that logic is a lot more complex, and needs to be handled with a case by case basis in every method.

why is this the case? it just adds two fields plus a getter/setter pair without impacting any of the other marshaling/protoreflect code


func Sov(x uint64) (n int) {
return (bits.Len64(x|1) + 6) / 7
}
Expand Down
3 changes: 3 additions & 0 deletions testpb/1.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ syntax="proto3";
option go_package = "github.com/cosmos/cosmos-proto/testpb";

import "google/protobuf/descriptor.proto";
import "proto/cosmos_proto/cosmos.proto";

enum Enumeration {
One = 0;
Expand Down Expand Up @@ -35,6 +36,8 @@ message A {
};
repeated Enumeration LIST_ENUM = 22;
google.protobuf.FileDescriptorProto imported = 23;

string custom_type = 24 [(cosmos_proto.go_type) = "Int"];
}

message B {
Expand Down
Loading