Skip to content

Commit

Permalink
Message literals allow ints for enums (#98)
Browse files Browse the repository at this point in the history 
Furthermore, open enums (e.g. those defined in a proto3 file) can even
have int values that do not correspond to known/named enum values.
  • Loading branch information
@jhump
jhump authored Feb 22, 2023
1 parent c1a7e84 commit 6df82ab
Show file tree
Hide file tree
Showing 2 changed files with 185 additions and 10 deletions.
145 changes: 145 additions & 0 deletions linker/linker_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1438,6 +1438,151 @@ func TestLinkerValidation(t *testing.T) {
expectedErr: `foo.proto:3:6: symbol "foo.bar.baz" already defined as a package at bar.proto:2:9` +
` || bar.proto:2:9: symbol "foo.bar.baz" already defined at foo.proto:3:6`,
},
"success_enum_in_msg_literal_using_number": {
input: map[string]string{
"foo.proto": `
syntax = "proto2";
import "google/protobuf/descriptor.proto";
enum Foo {
ZERO = 0;
ONE = 1;
}
message Bar {
optional Foo foo = 1;
}
extend google.protobuf.FileOptions {
optional Bar bar = 10101;
}
option (bar) = { foo: 1 };`,
},
},
"success_enum_in_msg_literal_using_negative_number": {
input: map[string]string{
"foo.proto": `
syntax = "proto2";
import "google/protobuf/descriptor.proto";
enum Foo {
ZERO = 0;
ONE = 1;
NEG_ONE = -1;
}
message Bar {
optional Foo foo = 1;
}
extend google.protobuf.FileOptions {
optional Bar bar = 10101;
}
option (bar) = { foo: -1 };`,
},
},
"success_open_enum_in_msg_literal_using_unknown_number": {
// enums in proto3 are "open", so unknown numbers are acceptable
input: map[string]string{
"foo.proto": `
syntax = "proto3";
import "google/protobuf/descriptor.proto";
enum Foo {
ZERO = 0;
ONE = 1;
}
message Bar {
Foo foo = 1;
}
extend google.protobuf.FileOptions {
Bar bar = 10101;
}
option (bar) = { foo: 5 };`,
},
},
"failure_enum_option_using_number": {
input: map[string]string{
"foo.proto": `
syntax = "proto2";
import "google/protobuf/descriptor.proto";
enum Foo {
ZERO = 0;
ONE = 1;
}
extend google.protobuf.FileOptions {
optional Foo foo = 10101;
}
option (foo) = 1;`,
},
expectedErr: `foo.proto:10:16: option (foo): expecting enum name, got integer`,
},
"failure_default_value_for_enum_using_number": {
input: map[string]string{
"foo.proto": `
syntax = "proto2";
enum Foo {
ZERO = 0;
ONE = 1;
}
message Bar {
optional Foo foo = 1 [default=1];
}`,
},
expectedErr: `foo.proto:7:39: field Bar.foo: option default: expecting enum name, got integer`,
},
"failure_closed_enum_in_msg_literal_using_unknown_number": {
input: map[string]string{
"foo.proto": `
syntax = "proto2";
import "google/protobuf/descriptor.proto";
enum Foo {
ZERO = 0;
ONE = 1;
}
message Bar {
optional Foo foo = 1;
}
extend google.protobuf.FileOptions {
optional Bar bar = 10101;
}
option (bar) = { foo: 5 };`,
},
expectedErr: `foo.proto:13:23: option (bar): closed enum Foo has no value with number 5`,
},
"failure_enum_in_msg_literal_using_out_of_range_number": {
input: map[string]string{
"foo.proto": `
syntax = "proto3";
import "google/protobuf/descriptor.proto";
enum Foo {
ZERO = 0;
ONE = 1;
NEG_ONE = -1;
}
message Bar {
Foo foo = 1;
}
extend google.protobuf.FileOptions {
Bar bar = 10101;
}
option (bar) = { foo: 2147483648 };`,
},
expectedErr: `foo.proto:14:23: option (bar): value 2147483648 is out of range for an enum`,
},
"failure_enum_in_msg_literal_using_out_of_range_negative_number": {
input: map[string]string{
"foo.proto": `
syntax = "proto3";
import "google/protobuf/descriptor.proto";
enum Foo {
ZERO = 0;
ONE = 1;
NEG_ONE = -1;
}
message Bar {
Foo foo = 1;
}
extend google.protobuf.FileOptions {
Bar bar = 10101;
}
option (bar) = { foo: -2147483649 };`,
},
expectedErr: `foo.proto:14:23: option (bar): value -2147483649 is out of range for an enum`,
},
}

for name, tc := range testCases {
Expand Down
50 changes: 40 additions & 10 deletions options/options.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -344,11 +344,11 @@ func (interp *interpreter) processDefaultOption(scope string, fqn string, fld *d
var v interface{}
if fld.GetType() == descriptorpb.FieldDescriptorProto_TYPE_ENUM {
ed := interp.file.ResolveEnumType(protoreflect.FullName(fld.GetTypeName()))
ev, err := interp.enumFieldValue(mc, ed, val)
_, name, err := interp.enumFieldValue(mc, ed, val, false)
if err != nil {
return -1, interp.reporter.HandleError(err)
}
v = string(ev.Name())
v = string(name)
} else {
v, err = interp.scalarFieldValue(mc, fld.GetType(), val, false)
if err != nil {
Expand Down Expand Up @@ -1191,11 +1191,11 @@ func (interp *interpreter) fieldValue(mc *internal.MessageContext, fld protorefl
k := fld.Kind()
switch k {
case protoreflect.EnumKind:
evd, err := interp.enumFieldValue(mc, fld.Enum(), val)
num, _, err := interp.enumFieldValue(mc, fld.Enum(), val, insideMsgLiteral)
if err != nil {
return interpretedFieldValue{}, err
}
return interpretedFieldValue{val: protoreflect.ValueOfEnum(evd.Number())}, nil
return interpretedFieldValue{val: protoreflect.ValueOfEnum(num)}, nil

case protoreflect.MessageKind, protoreflect.GroupKind:
v := val.Value()
Expand All @@ -1216,16 +1216,46 @@ func (interp *interpreter) fieldValue(mc *internal.MessageContext, fld protorefl

// enumFieldValue resolves the given AST node val as an enum value descriptor. If the given
// value is not a valid identifier, an error is returned instead.
func (interp *interpreter) enumFieldValue(mc *internal.MessageContext, ed protoreflect.EnumDescriptor, val ast.ValueNode) (protoreflect.EnumValueDescriptor, error) {
func (interp *interpreter) enumFieldValue(mc *internal.MessageContext, ed protoreflect.EnumDescriptor, val ast.ValueNode, allowNumber bool) (protoreflect.EnumNumber, protoreflect.Name, error) {
v := val.Value()
if id, ok := v.(ast.Identifier); ok {
ev := ed.Values().ByName(protoreflect.Name(id))
var num protoreflect.EnumNumber
switch v := v.(type) {
case ast.Identifier:
name := protoreflect.Name(v)
ev := ed.Values().ByName(name)
if ev == nil {
return nil, reporter.Errorf(interp.nodeInfo(val).Start(), "%venum %s has no value named %s", mc, ed.FullName(), id)
return 0, "", reporter.Errorf(interp.nodeInfo(val).Start(), "%venum %s has no value named %s", mc, ed.FullName(), v)
}
return ev.Number(), name, nil
case int64:
if !allowNumber {
return 0, "", reporter.Errorf(interp.nodeInfo(val).Start(), "%vexpecting enum name, got %s", mc, valueKind(v))
}
if v > math.MaxInt32 || v < math.MinInt32 {
return 0, "", reporter.Errorf(interp.nodeInfo(val).Start(), "%vvalue %d is out of range for an enum", mc, v)
}
num = protoreflect.EnumNumber(v)
case uint64:
if !allowNumber {
return 0, "", reporter.Errorf(interp.nodeInfo(val).Start(), "%vexpecting enum name, got %s", mc, valueKind(v))
}
if v > math.MaxInt32 {
return 0, "", reporter.Errorf(interp.nodeInfo(val).Start(), "%vvalue %d is out of range for an enum", mc, v)
}
return ev, nil
num = protoreflect.EnumNumber(v)
default:
return 0, "", reporter.Errorf(interp.nodeInfo(val).Start(), "%vexpecting enum, got %s", mc, valueKind(v))
}
isOpen := ed.ParentFile().Syntax() == protoreflect.Proto3
ev := ed.Values().ByNumber(num)
if ev != nil {
return num, ev.Name(), nil
}
if !isOpen {
return 0, "", reporter.Errorf(interp.nodeInfo(val).Start(), "%vclosed enum %s has no value with number %d", mc, ed.FullName(), num)
}
return nil, reporter.Errorf(interp.nodeInfo(val).Start(), "%vexpecting enum, got %s", mc, valueKind(v))
// unknown value, but enum is open, so we allow it and return blank name
return num, "", nil
}

// scalarFieldValue resolves the given AST node val as a value whose type is assignable to a
Expand Down

0 comments on commit 6df82ab

Please sign in to comment.