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

Support option aggregate syntax #68

Closed
perrydunn opened this issue Feb 15, 2022 · 9 comments
Closed

Support option aggregate syntax #68

perrydunn opened this issue Feb 15, 2022 · 9 comments
Assignees

Comments

@perrydunn
Copy link
Contributor

perrydunn commented Feb 15, 2022

Thanks for producing protolint!

Please could the aggregate syntax be supported for (custom) options?

message FooOptions {
  optional int32 opt1 = 1;
  optional string opt2 = 2;
}

extend google.protobuf.FieldOptions {
  optional FooOptions foo_options = 1234;
}

// usage:
message Bar {
  optional int32 a = 1 [(foo_options).opt1 = 123, (foo_options).opt2 = "baz"];
  // alternative aggregate syntax (uses TextFormat):
  optional int32 b = 2 [(foo_options) = { opt1: 123 opt2: "baz" }];
  // for completeness, split with (optional) commas:
  optional int32 c = 3 [(foo_options) = {
    opt1: 123,
    opt2: "baz",
  }];
}

Unfortunately this is not part of the language spec but is implemented in protoc...

@yoheimuta
Copy link
Owner

@perrydunn Thank you for reaching out!
I'm looking into the syntax.

@yoheimuta yoheimuta self-assigned this Feb 22, 2022
@yoheimuta
Copy link
Owner

@perrydunn Basically, the parser supports the aggregate syntax like https://github.com/yoheimuta/go-protoparser/blob/master/_testdata/grpc-gateway_a_bit_of_everything.proto#L16.

But, a trailing period in opt2: "baz". is the unexpected syntax.
I couldn't find this split with (optional) commas example in any documents.

image

Where did you pick it up?

@perrydunn
Copy link
Contributor Author

perrydunn commented Feb 22, 2022

Ah, sorry, that should have been a trailing comma. I've updated the original message with the comma.

Thanks for clarifying that it is supported. Unfortunately at work we use the comma-separated version. I appreciate that there is no such example in the docs (I couldn't find one either), but it is permitted by protoc. At work we use protobuf extensively and have many such examples which compile fine.

@perrydunn
Copy link
Contributor Author

perrydunn commented Feb 22, 2022

Just going to mention these issues here to link:
protocolbuffers/protobuf#3755
protocolbuffers/protobuf#1148
It's frustrating that the spec is not fully documented by Google.

@yoheimuta
Copy link
Owner

Please could the aggregate syntax be supported for (custom) options?

@perrydunn Have you experienced any errors?
Because I confirmed it parsed your example well locally.

(base) ~/w/p/g/s/g/y/go-protoparser ❯❯❯ gwd _testdata
diff --git a/_testdata/simple.proto b/_testdata/simple.proto
index f372a79..081f480 100644
--- a/_testdata/simple.proto
+++ b/_testdata/simple.proto
@@ -1,26 +1,22 @@
 syntax = "proto3";
-// An example of the official reference
-// See https://developers.google.com/protocol-buffers/docs/reference/proto3-spec#proto_file
-package examplepb;
-import public "other.proto";
-option java_package = "com.example.foo";
-enum EnumAllowingAlias {
-    option allow_alias = true;
-    UNKNOWN = 0;
-    STARTED = 1;
-    RUNNING = 2 [(custom_option) = "this is a "
-                                   "string on two lines"
-                ];
+
+message FooOptions {
+  optional int32 opt1 = 1;
+  optional string opt2 = 2;
 }
-message outer {
-    option (my_option).a = true;
-    message inner {   // Level 2
-      int64 ival = 1;
-    }
-    repeated inner inner_message = 2;
-    EnumAllowingAlias enum_field =3;
-    map<int32, string> my_map = 4;
+
+extend google.protobuf.FieldOptions {
+  optional FooOptions foo_options = 1234;
+}
+
+// usage:
+message Bar {
+  optional int32 a = 1 [(foo_options).opt1 = 123, (foo_options).opt2 = "baz"];
+  // alternative aggregate syntax (uses TextFormat):
+  optional int32 b = 2 [(foo_options) = { opt1: 123 opt2: "baz" }];
+  // for completeness, split with (optional) commas:
+  optional int32 c = 3 [(foo_options) = {
+    opt1: 123,
+    opt2: "baz",
+  }];
 }
-service HelloService {
-  rpc SayHello (HelloRequest) returns (HelloResponse) {};
-}
\ No newline at end of file
(base) ~/w/p/g/s/g/y/go-protoparser ❯❯❯ make run/dump/example
go run _example/dump/main.go -debug=false -permissive=true -unordered=false
{
  "Syntax": {
    "ProtobufVersion": "proto3",
    "ProtobufVersionQuote": "\"proto3\"",
    "Comments": null,
    "InlineComment": null,
    "Meta": {
      "Pos": {
        "Filename": "simple.proto",
        "Offset": 0,
        "Line": 1,
        "Column": 1
      },
      "LastPos": {
        "Filename": "simple.proto",
        "Offset": 17,
        "Line": 1,
        "Column": 18
      }
    }
  },
  "ProtoBody": [
    {
      "MessageName": "FooOptions",
      "MessageBody": [
        {
          "IsRepeated": false,
          "IsRequired": false,
          "IsOptional": true,
          "Type": "int32",
          "FieldName": "opt1",
          "FieldNumber": "1",
          "FieldOptions": null,
          "Comments": null,
          "InlineComment": null,
          "Meta": {
            "Pos": {
              "Filename": "simple.proto",
              "Offset": 43,
              "Line": 4,
              "Column": 3
            },
            "LastPos": {
              "Filename": "",
              "Offset": 0,
              "Line": 0,
              "Column": 0
            }
          }
        },
        {
          "IsRepeated": false,
          "IsRequired": false,
          "IsOptional": true,
          "Type": "string",
          "FieldName": "opt2",
          "FieldNumber": "2",
          "FieldOptions": null,
          "Comments": null,
          "InlineComment": null,
          "Meta": {
            "Pos": {
              "Filename": "simple.proto",
              "Offset": 70,
              "Line": 5,
              "Column": 3
            },
            "LastPos": {
              "Filename": "",
              "Offset": 0,
              "Line": 0,
              "Column": 0
            }
          }
        }
      ],
      "Comments": null,
      "InlineComment": null,
      "InlineCommentBehindLeftCurly": null,
      "Meta": {
        "Pos": {
          "Filename": "simple.proto",
          "Offset": 20,
          "Line": 3,
          "Column": 1
        },
        "LastPos": {
          "Filename": "simple.proto",
          "Offset": 96,
          "Line": 6,
          "Column": 1
        }
      }
    },
    {
      "MessageType": "google.protobuf.FieldOptions",
      "ExtendBody": [
        {
          "IsRepeated": false,
          "IsRequired": false,
          "IsOptional": true,
          "Type": "FooOptions",
          "FieldName": "foo_options",
          "FieldNumber": "1234",
          "FieldOptions": null,
          "Comments": null,
          "InlineComment": null,
          "Meta": {
            "Pos": {
              "Filename": "simple.proto",
              "Offset": 139,
              "Line": 9,
              "Column": 3
            },
            "LastPos": {
              "Filename": "",
              "Offset": 0,
              "Line": 0,
              "Column": 0
            }
          }
        }
      ],
      "Comments": null,
      "InlineComment": null,
      "InlineCommentBehindLeftCurly": null,
      "Meta": {
        "Pos": {
          "Filename": "simple.proto",
          "Offset": 99,
          "Line": 8,
          "Column": 1
        },
        "LastPos": {
          "Filename": "simple.proto",
          "Offset": 179,
          "Line": 10,
          "Column": 1
        }
      }
    },
    {
      "MessageName": "Bar",
      "MessageBody": [
        {
          "IsRepeated": false,
          "IsRequired": false,
          "IsOptional": true,
          "Type": "int32",
          "FieldName": "a",
          "FieldNumber": "1",
          "FieldOptions": [
            {
              "OptionName": "(foo_options).opt1",
              "Constant": "123"
            },
            {
              "OptionName": "(foo_options).opt2",
              "Constant": "\"baz\""
            }
          ],
          "Comments": null,
          "InlineComment": null,
          "Meta": {
            "Pos": {
              "Filename": "simple.proto",
              "Offset": 208,
              "Line": 14,
              "Column": 3
            },
            "LastPos": {
              "Filename": "",
              "Offset": 0,
              "Line": 0,
              "Column": 0
            }
          }
        },
        {
          "IsRepeated": false,
          "IsRequired": false,
          "IsOptional": true,
          "Type": "int32",
          "FieldName": "b",
          "FieldNumber": "2",
          "FieldOptions": [
            {
              "OptionName": "(foo_options)",
              "Constant": "{opt1:123\nopt2:\"baz\"}"
            }
          ],
          "Comments": [
            {
              "Raw": "// alternative aggregate syntax (uses TextFormat):",
              "Meta": {
                "Pos": {
                  "Filename": "simple.proto",
                  "Offset": 287,
                  "Line": 15,
                  "Column": 3
                },
                "LastPos": {
                  "Filename": "simple.proto",
                  "Offset": 336,
                  "Line": 15,
                  "Column": 52
                }
              }
            }
          ],
          "InlineComment": null,
          "Meta": {
            "Pos": {
              "Filename": "simple.proto",
              "Offset": 340,
              "Line": 16,
              "Column": 3
            },
            "LastPos": {
              "Filename": "",
              "Offset": 0,
              "Line": 0,
              "Column": 0
            }
          }
        },
        {
          "IsRepeated": false,
          "IsRequired": false,
          "IsOptional": true,
          "Type": "int32",
          "FieldName": "c",
          "FieldNumber": "3",
          "FieldOptions": [
            {
              "OptionName": "(foo_options)",
              "Constant": "{opt1:123,opt2:\"baz\",}"
            }
          ],
          "Comments": [
            {
              "Raw": "// for completeness, split with (optional) commas:",
              "Meta": {
                "Pos": {
                  "Filename": "simple.proto",
                  "Offset": 408,
                  "Line": 17,
                  "Column": 3
                },
                "LastPos": {
                  "Filename": "simple.proto",
                  "Offset": 457,
                  "Line": 17,
                  "Column": 52
                }
              }
            }
          ],
          "InlineComment": null,
          "Meta": {
            "Pos": {
              "Filename": "simple.proto",
              "Offset": 461,
              "Line": 18,
              "Column": 3
            },
            "LastPos": {
              "Filename": "",
              "Offset": 0,
              "Line": 0,
              "Column": 0
            }
          }
        }
      ],
      "Comments": [
        {
          "Raw": "// usage:",
          "Meta": {
            "Pos": {
              "Filename": "simple.proto",
              "Offset": 182,
              "Line": 12,
              "Column": 1
            },
            "LastPos": {
              "Filename": "simple.proto",
              "Offset": 190,
              "Line": 12,
              "Column": 9
            }
          }
        }
      ],
      "InlineComment": null,
      "InlineCommentBehindLeftCurly": null,
      "Meta": {
        "Pos": {
          "Filename": "simple.proto",
          "Offset": 192,
          "Line": 13,
          "Column": 1
        },
        "LastPos": {
          "Filename": "simple.proto",
          "Offset": 539,
          "Line": 22,
          "Column": 1
        }
      }
    }
  ],
  "Meta": {
    "Filename": "simple.proto"
  }

@perrydunn
Copy link
Contributor Author

perrydunn commented Feb 23, 2022

Thanks for the response. FieldOptions look happy, but at least EnumValueOptions are not (I haven't checked the others yet).

e.g. with simple.proto as

syntax = "proto3";

message FooOptions {
  optional int32 opt1 = 1;
  optional string opt2 = 2;
}

extend google.protobuf.EnumValueOptions {
  FooOptions foo_options = 1235;
}

enum Status {
  UNKNOWN = 0;
  COMPLETE = 1 [(foo_options) = {
    opt1: 123,
    opt2: "baz",
  }];
}

make run/dump/example results in

go run _example/dump/main.go -debug=false -permissive=true -unordered=false
failed to parse, err found "\"{\"(Token=14, Pos=simple.proto:14:33)" but expected [enumValueOption] at /home/peregrine/go-protoparser/parser/enum.go:280:found "{" but expected [;]
exit status 1
Makefile:47: recipe for target 'run/dump/example' failed
make: *** [run/dump/example] Error 1

@yoheimuta
Copy link
Owner

@perrydunn
Copy link
Contributor Author

FYI I opened yoheimuta/protolint#221 to upgrade protolint's go-protoparser version to the one you've cut.

I'll run protolint on further protobuf files in our repo at work to see if there are any further issues wrt aggregate syntax and//or options before closing this issue.

Thanks!

@perrydunn
Copy link
Contributor Author

I've run on our repo and all is well!

Thanks @yoheimuta

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants