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

protoparse: fix extension resolution in custom options #484

Merged
merged 1 commit into from
Feb 4, 2022

Conversation

jhump
Copy link
Owner

@jhump jhump commented Feb 3, 2022

The way protoparse resolved references to extension names in custom options (both in option names and also in field names inside message literal values) did not work exactly like protoc.

Example file:

syntax="proto2";
package foo.bar;
import "google/protobuf/descriptor.proto";

message a { extensions 1 to 100; }
extend google.protobuf.MessageOptions { optional a opt = 10000; }

message b {
  message c {
    extend a { repeated int32 i = 1; repeated float f = 2; }
  }

  // extension references inside a message literal:
  option (opt) = {
    [foo.bar.b.c.i]: 123
    [bar.b.c.i]: 234  // * this form is accepted by protoc but erroneously rejected by protoparse
    [b.c.i]: 345.     // * this form is accepted by protoc but erroneously rejected by protoparse

    // these forms are accepted by neither
    //[c.i]: 456.     
    //[i]: 567
  };

  // extension references inside the option name:
  option (opt).(foo.bar.b.c.f) = 1.23;
  option (opt).(bar.b.c.f) = 2.34;
  option (opt).(b.c.f) = 3.45;
  option (opt).(c.f) = 4.56;  // ** this form is rejected by protoc but erroneously accepted by protoparse

  // this form is accepted by neither
  //option (opt).(f) = 5.67;
}

As seen in comments above, there were some names that protoc resolves that cause errors in protoparse. And vice versa: there is one extension that protoparse was able to resolve, but should not have been if adhering to the same scoping rules that protoc implements.

Also visible above: protoc is consistent with how names are resolved, whether they are field names in message literals vs. in option names. But protoparse was quite inconsistent.

This PR fixes this and includes lots of fun tests to verify that all forms accepted by protoc are accepted by protoparse and also that forms rejected by protoc are also rejected by protoparse.

@jhump jhump merged commit 260eab9 into master Feb 4, 2022
@jhump jhump deleted the jh/extension-resolution branch February 4, 2022 17:05
pkwarren added a commit to bufbuild/protocompile that referenced this pull request Sep 15, 2022
pkwarren added a commit to bufbuild/protocompile that referenced this pull request Sep 15, 2022
Port jhump/protoreflect#484 to protocompile.

Co-authored-by: Joshua Humphries <jhumphries@buf.build>
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

Successfully merging this pull request may close these issues.

1 participant