protobuf icon indicating copy to clipboard operation
protobuf copied to clipboard

[docs] Issues with proto2 and proto3 language specifications

Open jhugard opened this issue 9 years ago • 3 comments

I would like to humbly request that the Protocol Buffers Reference pages be updated to reflect the current language definition(s): specifically, to correct the following issues with the Proto2 and Proto3 language specifications.

Issues identified include the following.

Proto2 Service definition section indicates that "stream <streamName> ( <messageType>, <messageType> )" is a valid construct. However, this construct is supported by neither protoc nor the FileDescriptor definition (nor has it ever been going back to the first GITHUB commit):

Original:

service = "service" serviceName "{" { option | rpc | stream | emptyStatement } "}"
rpc = "rpc" rpcName "(" ["stream"] messageType ")" "returns" "(" ["stream"]
messageType ")" (("{" {option | emptyStatement } "}") | ";")
stream = "stream" streamName "(" messageType "," messageType ")" (("{"
{option | emptyStatement} "}") | ";")

Recommended Edit:

service = "service" serviceName "{" { option | rpc | emptyStatement } "}"
rpc = "rpc" rpcName "(" ["stream"] messageType ")" "returns" "(" ["stream"]
messageType ")" (("{" {option | emptyStatement } "}") | ";")

Proto3 Service definition section references the "stream" production described above in the definition of "service", but deletes the definition of "stream". Looks like the section was copied from Proto2 and while "stream" definition was deleted, the reference on the "service" line was not updated.

Original:

service = "service" serviceName "{" { option | rpc | stream | emptyStatement } "}"
rpc = "rpc" rpcName "(" ["stream"] messageType ")" "returns" "(" ["stream"]
messageType ")" (("{" {option | emptyStatement } "}") | ";")

Recommended Edit:

service = "service" serviceName "{" { option | rpc | emptyStatement } "}"
rpc = "rpc" rpcName "(" ["stream"] messageType ")" "returns" "(" ["stream"]
messageType ")" (("{" {option | emptyStatement } "}") | ";")

"constant" is not defined for either proto2 or proto3.

The value 'constant' is used in several places, but is not defined. This appears to be a literal value (int, float, bool, or string), but that is an assumption because it is never explicitly defined

For example:

option = "option" optionName  "=" constant ";"
optionName = (ident | "(" fullIdent ")") {"." ident}

Recommended Edit on both proto2 and proto3 language specification pages:

Between "String literals" and "EmptyStatement", add the following:

constant = intLit | floatLit | boolLit | strLit

Option "Aggregate Syntax" is neither documented nor defined.

Search for Aggregate Syntax on the Proto2 Language Guide page. The example usage defines an aggregate syntax for option definition that is supported by the protoc compiler, but which is neither well documented in the language guide nor defined on the proto2/proto3 language specification pages.

Original for both proto2 and proto3:

option = "option" optionName  "=" constant ";"
optionName = (ident | "(" fullIdent ")") {"." ident}

Recommended Edit for both:

option = "option" optionName  "=" ( constant | altOptSyntax ) ";"
optionName = (ident | "(" fullIdent ")") {"." ident}
altOptSyntax = "{" ident ":" constant [ { "," ident ":" constant } ] "}"

Also, update the example referenced above to include a comma separator between values.

NOTE: have not verified if the altOptSyntax is actually supported by the current protoc; to my recollection it is.

Parser appears to support negative integer and and floats literals in an option definition, but this is not reflected in the language specification.

According to the language specification, all intLit and floatLit values must be positive values (minus sign is not supported):

decimalDigit = "0" … "9"
octalDigit   = "0" … "7"
hexDigit     = "0" … "9" | "A" … "F" | "a" … "f"
intLit     = decimalLit | octalLit | hexLit
decimalLit = ( "1" … "9" ) { decimalDigit }
octalLit   = "0" { octalDigit }
hexLit     = "0" ( "x" | "X" ) hexDigit { hexDigit } 

Integer definitions in general should get a review pass. E.g., intLit is is defined for all integer literals, including the definition of fieldNumber. The former allows 0, but the latter appears to require a positive value >= 1.

Example at the end of proto2 language spec is incorrect:

message foo {
  optional group GroupMessage {
    optional a = 1;
  }
}

Should read:

message foo {
  optional group GroupMessage = 1 {
    optional int32 a = 2;
  }
}

jhugard avatar Jan 17 '16 13:01 jhugard