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

protoc: strange inconsistency in message literals in custom options for when a colon is needed #9551

Closed
jhump opened this issue Feb 25, 2022 · 4 comments

Comments

@jhump
Copy link
Contributor

jhump commented Feb 25, 2022

What version of protobuf and what language are you using?
3.19.0

What operating system (Linux, Windows, ...) and version?
OS X

What runtime / compiler are you using (e.g., python version or gcc version)
N/A, just using protoc to compile descriptors

What did you do?
I tried to compile the following file, via protoc -o /dev/null test.proto:

// test.proto
syntax = "proto3";

import "google/protobuf/descriptor.proto";

message Foo {
  Foo f = 1;
  string s = 2;
  repeated Foo fs = 3;
  repeated string ss = 4;
}

extend google.protobuf.MessageOptions {
  Foo foo = 10101;
}

message Test1 {
  option (foo) = {
    // using a colon to separate field and value always works
    f: {s:"a"}
    s: "a"
    fs: [{s:"a"}, {s:"b"}, {s:"c"}]
    ss: ["a", "b", "c"]
  };
}

message Test2 {
  option (foo) = {
    // but it's inconsistent when a colon can be omitted
   
    // safe to omit when value is a message because '<' or '{'
    // unambiguously indicates start of value
    f {s: "a"}
    
    // also safe to omit for repeated field of messages using
    // array literal notation
    fs [{s:"a"}, {s:"b"}, {s:"c"}]

    // but NOT safe to omit for repeated scalar in array literal
    // notation, even though '[' unambiguously indicates start of value
    ss ["a", "b", "c"]
  };
}

What did you expect to see
Success

What did you see instead?
An error that a colon is required on that last example value:

test.proto:28:18: Error while parsing option value for "foo": Expected ":", found "[".

If I add a colon just to that last one (ss) or comment out that line, the file is accepted.

Anything else we should know about your project / environment
Nope

@elharo
Copy link
Contributor

elharo commented Feb 28, 2022

I expect to find this in the EBNF but I don't:

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

If we follow that spec, none of this is allowed. We probably need to update that spec.

@jhump
Copy link
Contributor Author

jhump commented Feb 28, 2022

Yeah, it does not go into the format for message literals. The implementation actually relies on the libprotoc implementation of the text format -- so it just provides everything between the { and } to the text format parser.

Also relevant:
protocolbuffers/protobuf-grammar#2
https://github.com/jhump/protocompile/tree/master/grammar

@perezd
Copy link
Contributor

perezd commented Apr 18, 2022

Realistically, this language blemish is something we have to live with. Fixing this is a significant breaking change that we're unlikely to have staffing to drive.

@perezd perezd closed this as completed Apr 18, 2022
@jhump
Copy link
Contributor Author

jhump commented Aug 22, 2022

While I understand that changing the text format could be a significant effort, I think it might be simpler to just patch protoc. Also, it would not be a breaking change as it would make protoc strictly more permissive, not less (i.e. programs that omit a colon that are rejected today would suddenly be accepted by the compiler).

As far as this only effecting protoc, not the text format, this could be achieved by letting the descriptor database or options interpreter (whichever class parses the data when interpreting options) be a "friend" class to the C++ text format implementation. The text format could then have a private method that allows toggling off the requirement that list literals be preceded by a colon, and only the option interpretation step of the compiler actually uses it.

This alternate way to address might be small enough for a contributor to look into a pull request, instead of marshaling the resources of Google's protobuf team.

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

No branches or pull requests

3 participants