Skip to content

Commit

Permalink
improvements to protoparse's handling of message literals in custom o…
Browse files Browse the repository at this point in the history 
…ptions (#483)

* fix interpretation of field values in message literals in custom options when:
  1. bool field and value is just "t", "f", "True", "False" (supported by protoc, not previously supported by protoparse)
  2. enum field and enum value name is "true", "false", "t", "f", "True", "False", "inf", or "nan"
* extensions inside message literals use protobuf text format, which does not allow fully-qualified names starting with a dot
  • Loading branch information
@jhump
jhump authored Feb 3, 2022
1 parent d02a936 commit d4949d2
Show file tree
Hide file tree
Showing 14 changed files with 490 additions and 299 deletions.
10 changes: 8 additions & 2 deletions desc/protoparse/ast/values.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -366,6 +366,11 @@ func (n *SignedFloatLiteralNode) AsFloat() float64 {
}

// BoolLiteralNode represents a boolean literal.
//
// Deprecated: The AST uses IdentNode for boolean literals, where the
// identifier value is "true" or "false". This is required because an
// identifier "true" is not necessarily a boolean value as it could also
// be an enum value named "true" (ditto for "false").
type BoolLiteralNode struct {
*KeywordNode
Val bool
Expand Down Expand Up @@ -526,8 +531,9 @@ type MessageFieldNode struct {
compositeNode
Name *FieldReferenceNode
// Sep represents the ':' separator between the name and value. If
// the value is a message literal (and thus starts with '<' or '{'),
// then the separator is optional, and thus may be nil.
// the value is a message literal (and thus starts with '<' or '{')
// or an array literal (starting with '[') then the separator is
// optional, and thus may be nil.
Sep *RuneNode
Val ValueNode
}
Expand Down
59 changes: 59 additions & 0 deletions desc/protoparse/linker_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -601,6 +601,65 @@ func TestLinkerValidation(t *testing.T) {
},
`foo.proto:7:34: message Baz: option (foo).baz.options.(foo).buzz.name: oneof "bar" already has field "baz" set`,
},
{
map[string]string{
"foo.proto": "syntax = \"proto3\";\n" +
"import \"google/protobuf/descriptor.proto\";\n" +
"enum Foo { true = 0; false = 1; t = 2; f = 3; True = 4; False = 5; inf = 6; nan = 7; }\n" +
"extend google.protobuf.MessageOptions { repeated Foo foo = 10001; }\n" +
"message Baz {\n" +
" option (foo) = true; option (foo) = false;\n" +
" option (foo) = t; option (foo) = f;\n" +
" option (foo) = True; option (foo) = False;\n" +
" option (foo) = inf; option (foo) = nan;\n" +
"}\n",
},
"", // should succeed
},
{
map[string]string{
"foo.proto": "syntax = \"proto3\";\n" +
"import \"google/protobuf/descriptor.proto\";\n" +
"extend google.protobuf.MessageOptions { repeated bool foo = 10001; }\n" +
"message Baz {\n" +
" option (foo) = true; option (foo) = false;\n" +
" option (foo) = t; option (foo) = f;\n" +
" option (foo) = True; option (foo) = False;\n" +
"}\n",
},
"foo.proto:6:18: message Baz: option (foo): expecting bool, got identifier",
},
{
map[string]string{
"foo.proto": "syntax = \"proto3\";\n" +
"import \"google/protobuf/descriptor.proto\";\n" +
"message Foo { repeated bool b = 1; }\n" +
"extend google.protobuf.MessageOptions { Foo foo = 10001; }\n" +
"message Baz {\n" +
" option (foo) = {\n" +
" b: t b: f\n" +
" b: true b: false\n" +
" b: True b: False\n" +
" };\n" +
"}\n",
},
"", // should succeed
},
{
map[string]string{
"foo.proto": "syntax = \"proto2\";\n" +
"import \"google/protobuf/descriptor.proto\";\n" +
"message Foo { extensions 1 to 10; }\n" +
"extend Foo { optional bool b = 10; }\n" +
"extend google.protobuf.MessageOptions { optional Foo foo = 10001; }\n" +
"message Baz {\n" +
" option (foo) = {\n" +
" [.b]: true\n" +
" };\n" +
"}\n",
},
"foo.proto:8:6: syntax error: unexpected '.'",
},
}
for i, tc := range testCases {
acc := func(filename string) (io.ReadCloser, error) {
Expand Down
50 changes: 43 additions & 7 deletions desc/protoparse/options.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -858,7 +858,7 @@ func processDefaultOption(res *parseResult, scope string, fld fldDescriptorish,
elementType: descriptorType(fld.AsProto()),
option: opt,
}
v, err := fieldValue(res, mc, fld, val, true)
v, err := fieldValue(res, mc, fld, val, true, false)
if err != nil {
return -1, res.errs.handleError(err)
}
Expand Down Expand Up @@ -1106,7 +1106,7 @@ func interpretField(res *parseResult, mc *messageContext, element descriptorish,
}

optNode := res.getOptionNode(opt)
if err := setOptionField(res, mc, dm, fld, node, optNode.GetValue()); err != nil {
if err := setOptionField(res, mc, dm, fld, node, optNode.GetValue(), false); err != nil {
return nil, res.errs.handleError(err)
}
if fld.IsRepeated() {
Expand Down Expand Up @@ -1148,7 +1148,7 @@ func findExtension(fd fileDescriptorish, name string, public bool, checked map[f
return nil
}

func setOptionField(res *parseResult, mc *messageContext, dm *dynamic.Message, fld *desc.FieldDescriptor, name ast.Node, val ast.ValueNode) error {
func setOptionField(res *parseResult, mc *messageContext, dm *dynamic.Message, fld *desc.FieldDescriptor, name ast.Node, val ast.ValueNode, insideMsgLiteral bool) error {
v := val.Value()
if sl, ok := v.([]ast.ValueNode); ok {
// handle slices a little differently than the others
Expand All @@ -1161,7 +1161,7 @@ func setOptionField(res *parseResult, mc *messageContext, dm *dynamic.Message, f
}()
for index, item := range sl {
mc.optAggPath = fmt.Sprintf("%s[%d]", origPath, index)
if v, err := fieldValue(res, mc, richFldDescriptorish{FieldDescriptor: fld}, item, false); err != nil {
if v, err := fieldValue(res, mc, richFldDescriptorish{FieldDescriptor: fld}, item, false, insideMsgLiteral); err != nil {
return err
} else if err = dm.TryAddRepeatedField(fld, v); err != nil {
return errorWithPos(val.Start(), "%verror setting value: %s", mc, err)
Expand All @@ -1170,7 +1170,7 @@ func setOptionField(res *parseResult, mc *messageContext, dm *dynamic.Message, f
return nil
}

v, err := fieldValue(res, mc, richFldDescriptorish{FieldDescriptor: fld}, val, false)
v, err := fieldValue(res, mc, richFldDescriptorish{FieldDescriptor: fld}, val, false, insideMsgLiteral)
if err != nil {
return err
}
Expand Down Expand Up @@ -1315,7 +1315,7 @@ func valueKind(val interface{}) string {
}
}

func fieldValue(res *parseResult, mc *messageContext, fld fldDescriptorish, val ast.ValueNode, enumAsString bool) (interface{}, error) {
func fieldValue(res *parseResult, mc *messageContext, fld fldDescriptorish, val ast.ValueNode, enumAsString, insideMsgLiteral bool) (interface{}, error) {
v := val.Value()
t := fld.AsFieldDescriptorProto().GetType()
switch t {
Expand Down Expand Up @@ -1380,7 +1380,7 @@ func fieldValue(res *parseResult, mc *messageContext, fld fldDescriptorish, val
if ffld == nil {
return nil, errorWithPos(val.Start(), "%vfield %s not found", mc, string(a.Name.Name.AsIdentifier()))
}
if err := setOptionField(res, mc, fdm, ffld, a.Name, a.Val); err != nil {
if err := setOptionField(res, mc, fdm, ffld, a.Name, a.Val, true); err != nil {
return nil, err
}
}
Expand All @@ -1391,6 +1391,26 @@ func fieldValue(res *parseResult, mc *messageContext, fld fldDescriptorish, val
if b, ok := v.(bool); ok {
return b, nil
}
if id, ok := v.(ast.Identifier); ok {
if insideMsgLiteral {
// inside a message literal, values use the protobuf text format,
// which is lenient in that it accepts "t" and "f" or "True" and "False"
switch id {
case "t", "true", "True":
return true, nil
case "f", "false", "False":
return false, nil
}
} else {
// options with simple scalar values (no message literal) are stricter
switch id {
case "true":
return true, nil
case "false":
return false, nil
}
}
}
return nil, errorWithPos(val.Start(), "%vexpecting bool, got %s", mc, valueKind(v))
case dpb.FieldDescriptorProto_TYPE_BYTES:
if str, ok := v.(string); ok {
Expand Down Expand Up @@ -1453,6 +1473,14 @@ func fieldValue(res *parseResult, mc *messageContext, fld fldDescriptorish, val
}
return nil, errorWithPos(val.Start(), "%vexpecting uint64, got %s", mc, valueKind(v))
case dpb.FieldDescriptorProto_TYPE_DOUBLE:
if id, ok := v.(ast.Identifier); ok {
switch id {
case "inf":
return math.Inf(1), nil
case "nan":
return math.NaN(), nil
}
}
if d, ok := v.(float64); ok {
return d, nil
}
Expand All @@ -1464,6 +1492,14 @@ func fieldValue(res *parseResult, mc *messageContext, fld fldDescriptorish, val
}
return nil, errorWithPos(val.Start(), "%vexpecting double, got %s", mc, valueKind(v))
case dpb.FieldDescriptorProto_TYPE_FLOAT:
if id, ok := v.(ast.Identifier); ok {
switch id {
case "inf":
return float32(math.Inf(1)), nil
case "nan":
return float32(math.NaN()), nil
}
}
if d, ok := v.(float64); ok {
if (d > math.MaxFloat32 || d < -math.MaxFloat32) && !math.IsInf(d, 1) && !math.IsInf(d, -1) && !math.IsNaN(d) {
return nil, errorWithPos(val.Start(), "%vvalue %f is out of range for float", mc, d)
Expand Down
12 changes: 3 additions & 9 deletions desc/protoparse/proto.y
View file
Original file line number Diff line number Diff line change
Expand Up @@ -279,13 +279,7 @@ scalarConstant : stringLit {
}
| numLit
| name {
if $1.Val == "true" || $1.Val == "false" {
$$ = ast.NewBoolLiteralNode($1.ToKeyword())
} else if $1.Val == "inf" || $1.Val == "nan" {
$$ = ast.NewSpecialFloatLiteralNode($1.ToKeyword())
} else {
$$ = $1
}
$$ = $1
}

numLit : _FLOAT_LIT {
Expand Down Expand Up @@ -467,8 +461,8 @@ aggFieldEntry : aggName ':' scalarConstant {
aggName : name {
$$ = ast.NewFieldReferenceNode($1)
}
| '[' typeIdent ']' {
$$ = ast.NewExtensionFieldReferenceNode($1, $2, $3)
| '[' ident ']' {
$$ = ast.NewExtensionFieldReferenceNode($1, $2.toIdentValueNode(nil), $3)
}
| '[' error ']' {
$$ = nil
Expand Down
Loading

0 comments on commit d4949d2

Please sign in to comment.