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

Spec for options in protobuf language syntax is incomplete #8436

Closed
jroper opened this issue Mar 30, 2021 · 5 comments
Closed

Spec for options in protobuf language syntax is incomplete #8436

jroper opened this issue Mar 30, 2021 · 5 comments

Comments

@jroper
Copy link
Contributor

jroper commented Mar 30, 2021

I'm not sure where this documentation is hosted, but the following specifications for the protobuf language:

https://developers.google.com/protocol-buffers/docs/reference/proto2-spec
https://developers.google.com/protocol-buffers/docs/reference/proto3-spec

Are incomplete, they say nothing about the syntax of message literals when used in options. For example, this example from the developer guide for options:

optional int32 b = 2 [(foo_options) = { opt1: 123 opt2: "baz" }];

Is, according to the specs referenced above, not valid protobuf syntax, which specifies options as:

option = "option" optionName  "=" constant ";"
optionName = ( ident | "(" fullIdent ")" ) { "." ident }
constant = fullIdent | ( [ "-" | "+" ] intLit ) | ( [ "-" | "+" ] floatLit ) | strLit | boolLit 

This matters because right now I've encountered a problem in the JavaScript protocol buffers support where it differs in its opinion of what is valid syntax for message literals in options from protoc, but where does the bug lie? In protoc, or in the JavaScript protobuf parser? And even if we establish which one the bug lies in, what is the correct behavior? Without a spec that specifies what the correct syntax is, neither has a bug, and there's no way to resolve it because there's no correct behavior defined.

@jeffory-orrok
Copy link

fwiw, I asked the same thing on SO and in the Protocol Buffers group

I don't know how "official" the answer that was posted on Groups is, but it said essentially: the source code is the spec :-/

@jeffory-orrok
Copy link

jeffory-orrok commented Apr 1, 2021

You might have a look at the pull request I submitted to protobufjs to accept optional semicolons along with the optional commas. If you need optional colons too, you might add a second argument of true to the line that reads skip(":"); in parseOptionValue() of parse.js. I held off making that change because a) I didn't need it, and b) they seem to be optional only in certain circumstances based on what I saw in text_format.cc and protobufjs seems to have a more simplistic approach to parsing. [disclaimer: my analysis was hardly exhaustive]

@alkasm
Copy link
Contributor

alkasm commented Apr 3, 2021

See also: #6188

@dlj-NaN
Copy link
Contributor

dlj-NaN commented Apr 23, 2021

The original question refers to "Options [that] can be used in proto files, messages, enums and services."

Options for fields, however, are different. That syntax is here: https://developers.google.com/protocol-buffers/docs/reference/proto2-spec#normal_field

Message-typed custom option extensions are not actually included in that specification, but are covered in the language guide: https://developers.google.com/protocol-buffers/docs/proto#customoptions

(Basically, message-typed custom option extensions are specified in TextFormat syntax.)

@alkasm
Copy link
Contributor

alkasm commented May 25, 2021

(Basically, message-typed custom option extensions are specified in TextFormat syntax.)

For future readers, "TextFormat syntax" is referring to protobuf's TextFormat: https://developers.google.com/protocol-buffers/docs/reference/cpp/google.protobuf.text_format#TextFormat

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

5 participants