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: explicit map_entry option is sometimes allowed #13441

Closed
jhump opened this issue Aug 2, 2023 · 1 comment
Closed

protoc: explicit map_entry option is sometimes allowed #13441

jhump opened this issue Aug 2, 2023 · 1 comment
Labels

Comments

@jhump
Copy link
Contributor

jhump commented Aug 2, 2023

In the following example source file, protoc correctly produces an error:

// test.proto
message Foo { option map_entry = true; reserved 10 to 1; }
message Bar { optional Foo foo = 1; }
> protoc test.proto -o /dev/null
test.proto:3:24: map_entry should not be set explicitly. Use map<KeyType, ValueType> instead.

Interestingly, the reported position in the error message is where the message is used, not where it is defined.

However, if the file does not actually refer to the offending message type (Foo), the compiler accepts the invalid input:

// test.proto
message Foo { option map_entry = true; reserved 10 to 1; }

With the above, running protoc test.proto -o /dev/null produces no error.

This is inconsistent, but it also leads to "errors at a distance". If I define a file elsewhere that uses this message, I get an error in that other compile operation, instead of in the original:

// test2.proto
import "test.proto";
message Bar { optional Foo foo = 1; }
> protoc test2.proto -o /dev/null
test2.proto:3:24: map_entry should not be set explicitly. Use map<KeyType, ValueType> instead.
@jhump jhump added the untriaged auto added to all issues by default when created. label Aug 2, 2023
@fowles fowles added protoc and removed untriaged auto added to all issues by default when created. labels Aug 7, 2023
@fowles
Copy link
Member

fowles commented Aug 7, 2023

Also a bug, feel free to send a PR.

copybara-service bot pushed a commit that referenced this issue Aug 10, 2023
This addresses #13441.

This preserves the similar check at the _point of use_ of invalid messages in `DescriptorBuilder` (and there's an existing test that verifies that check still works).

But it adds another check in the parser, to catch this error at the _point of definition_ of an invalid message. And the corresponding test is updated: we no longer need a usage of the message to catch the error, and the reported position is the definition of the option, not the usage site of the message.

The way this works feels kinda gross, but I wasn't sure of a better way to do it. The only place we know for certain that it was an explicit option (vs. auto-added by the parser when synthesizing a map entry message) is when after processing the message body, we can look at the uninterpreted options. So that's what this does. If you have ideas on better/cleaner approaches, I'd be happy to revise.

Closes #13479

COPYBARA_INTEGRATE_REVIEW=#13479 from jhump:jh/map-option-not-allowed 4d95e9b
FUTURE_COPYBARA_INTEGRATE_REVIEW=#13479 from jhump:jh/map-option-not-allowed 4d95e9b
PiperOrigin-RevId: 555548662
copybara-service bot pushed a commit that referenced this issue Aug 10, 2023
This addresses #13441.

This preserves the similar check at the _point of use_ of invalid messages in `DescriptorBuilder` (and there's an existing test that verifies that check still works).

But it adds another check in the parser, to catch this error at the _point of definition_ of an invalid message. And the corresponding test is updated: we no longer need a usage of the message to catch the error, and the reported position is the definition of the option, not the usage site of the message.

The way this works feels kinda gross, but I wasn't sure of a better way to do it. The only place we know for certain that it was an explicit option (vs. auto-added by the parser when synthesizing a map entry message) is when after processing the message body, we can look at the uninterpreted options. So that's what this does. If you have ideas on better/cleaner approaches, I'd be happy to revise.

Closes #13479

COPYBARA_INTEGRATE_REVIEW=#13479 from jhump:jh/map-option-not-allowed 4d95e9b
FUTURE_COPYBARA_INTEGRATE_REVIEW=#13479 from jhump:jh/map-option-not-allowed 4d95e9b
PiperOrigin-RevId: 555548662
copybara-service bot pushed a commit that referenced this issue Aug 10, 2023
This addresses #13441.

This preserves the similar check at the _point of use_ of invalid messages in `DescriptorBuilder` (and there's an existing test that verifies that check still works).

But it adds another check in the parser, to catch this error at the _point of definition_ of an invalid message. And the corresponding test is updated: we no longer need a usage of the message to catch the error, and the reported position is the definition of the option, not the usage site of the message.

The way this works feels kinda gross, but I wasn't sure of a better way to do it. The only place we know for certain that it was an explicit option (vs. auto-added by the parser when synthesizing a map entry message) is when after processing the message body, we can look at the uninterpreted options. So that's what this does. If you have ideas on better/cleaner approaches, I'd be happy to revise.

Closes #13479

COPYBARA_INTEGRATE_REVIEW=#13479 from jhump:jh/map-option-not-allowed 4d95e9b
PiperOrigin-RevId: 555577734
copybara-service bot pushed a commit that referenced this issue Aug 12, 2023
This addresses #13441.  Second try, now with more internal fixes.

This preserves the similar check at the _point of use_ of invalid messages in `DescriptorBuilder` (and there's an existing test that verifies that check still works).

But it adds another check in the parser, to catch this error at the _point of definition_ of an invalid message. And the corresponding test is updated: we no longer need a usage of the message to catch the error, and the reported position is the definition of the option, not the usage site of the message.

The way this works feels kinda gross, but I wasn't sure of a better way to do it. The only place we know for certain that it was an explicit option (vs. auto-added by the parser when synthesizing a map entry message) is when after processing the message body, we can look at the uninterpreted options. So that's what this does. If you have ideas on better/cleaner approaches, I'd be happy to revise.

Closes #13479

PiperOrigin-RevId: 556335288
@fowles fowles closed this as completed Aug 15, 2023
copybara-service bot pushed a commit that referenced this issue Aug 15, 2023
This addresses #13441.  Second try, now with more internal fixes.

This preserves the similar check at the _point of use_ of invalid messages in `DescriptorBuilder` (and there's an existing test that verifies that check still works).

But it adds another check in the parser, to catch this error at the _point of definition_ of an invalid message. And the corresponding test is updated: we no longer need a usage of the message to catch the error, and the reported position is the definition of the option, not the usage site of the message.

The way this works feels kinda gross, but I wasn't sure of a better way to do it. The only place we know for certain that it was an explicit option (vs. auto-added by the parser when synthesizing a map entry message) is when after processing the message body, we can look at the uninterpreted options. So that's what this does. If you have ideas on better/cleaner approaches, I'd be happy to revise.

Closes #13479

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

No branches or pull requests

2 participants