protoc: reserved names are not validated

[dsnet]

Consider the following proto file:

message M {
    optional string foo = 1; 
    reserved "foo bar";
}

Here, "foo bar" is clearly an invalid identifier. The intention of the user is to specify two different identifiers "foo" and "bar" as reserved. Since the reserved name logic looks for exact matches, it does not warn the user that a reserved name is being used.

I expect protoc to fail if any reserved identifier is invalid.

[dsnet]

Related problem:

message M {
    optional string foo = 1; 
    reserved "foo" "bar";
}

Again, the intention is to specify "foo" and "bar" as two separate identifiers. However, since protoc seems to be applying a textproto-like grammar, "foo" "bar" is treated as a single concatenated string "foobar".

The distinction between:

• reserved "foo bar";
• reserved "foo" "bar";
• reserved "foo", "bar";

is too subtle and error prone.

[dsnet]

Also, the current behavior goes against the grammar specification, which says that the token should be a fieldName which is equivalent to the ident token.

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

[mkruskal-google]

I think validating the reserved field names it a reasonable feature request. For the related problems, I don't see any unambiguous solution there other than education/documentation.

[jhump]

For the related problems, I don't see any unambiguous solution there other than education/documentation.

The parser could be updated to allow identifiers:

// unambiguous, omitting the comma is a syntax error
reserved foo, bar;

The existing support for string literals could remain, for backwards compatibility. (Though hopefully not forever?)

[fowles]

Given the way that reserved works for field numbers, using it this way with identifiers seems like a clean parallel structure. It can even be implemented in a way that avoids modifying descriptor.proto. Probably we should have a small design doc that outlines the alternatives. @mkruskal-google can you share a template with @jhump ?

[mkruskal-google]

Our public template for change proposals can be found here: https://docs.google.com/document/d/e/2PACX-1vS9xfZ_0lnCdsMjqj8qf15F7IUNMjWB3ZT7aJuaerSbytFlkJkYshYlEyp020mqZo0MsvmAnUW-DPOk/pub

[jhump]

@mkruskal-google, I don't have permission to view that doc. Maybe it's Google-internal? Can you create a copy and make it globally readable?

[mkruskal-google]

Sorry about that, here's one that's actually public: https://docs.google.com/document/d/1RPgh6engDi319RhZ9cKmLQYLgyF7xscBHpqfHTpkQ9Q/edit#heading=h.f4bilzmezze2

[jhump]

@mkruskal-google, I don't think this issue is resolved. We still need another step to actually change the warnings (merged in #10586) into actual errors. Can you re-open?

I'll go ahead and open a new issue to track the rest of the proposal, for allowing bare identifiers to be used instead of string literals.

[github-actions[bot]]

We triage inactive PRs and issues in order to make it easier to find active work. If this issue should remain active or becomes active again, please add a comment.

This issue is labeled inactive because the last activity was over 90 days ago.

[github-actions[bot]]

We triage inactive PRs and issues in order to make it easier to find active work. If this issue should remain active or becomes active again, please reopen it.

This issue was closed and archived because there has been no new activity in the 14 days since the inactive label was added.