Different max extension value if message_set_wire_format = true

[bufdev]

Very obscure:

syntax = "proto2";

package foo;

message Bar {
  option message_set_wire_format = true;
  extensions 10 to max;
}

With protoc:

{
  "file": [
    {
      "name": "foo.proto",
      "package": "foo",
      "messageType": [
        {
          "name": "Bar",
          "extensionRange": [
            {
              "start": 10,
              "end": 2147483647
            }
          ],
          "options": {
            "messageSetWireFormat": true
          }
        }
      ]
    }
  ]
}

With protoreflect:

{
  "file": [
    {
      "name": "foo.proto",
      "package": "foo",
      "messageType": [
        {
          "name": "Bar",
          "extensionRange": [
            {
              "start": 10,
              "end": 536870912
            }
          ],
          "options": {
            "messageSetWireFormat": true
          }
        }
      ]
    }
  ]
}

If you turn message_set_wire_format off, the protoc generated max extension value goes back to 536870912.

[bufdev]

Note that if message_set_wire_format = true, then the start of the range is allowed to be greater than 2^29-1 as well, i.e. this is valid with protoc:

syntax = "proto2";

package foo;

message Bar {
  option message_set_wire_format = true;
  extensions 2147483640 to max;
}

But this is not:

syntax = "proto2";

package foo;

message Bar {
  extensions 2147483640 to max;
}

The second one produces:

foo.proto:6:14: Extension range end number must be greater than start number.

[bufdev]

This may get really ugly - I'm not sure how you update the logic in

protoreflect/desc/protoparse/proto.y

Line 693 in c1f7119

tagRange : _INT_LIT {

The max tag validation might have to be done outside of the .y logic, and rangeNode might have to have a marker for max, with internal.MaxTag being applied after the fact unless message_set_wire_format = true, in which case you have a new MaxTagMessageSetWireFormat = 2147483647 // 2^31 - 1. Assuming you have that value, here's a test to reproduce (added to parser_test.go):

func TestParseMaxExtensionValue(t *testing.T) {
	accessor := FileContentsFromMap(map[string]string{
		"test.proto": `
syntax = "proto2";
message Foo {
	extensions 4 to max;
}
`,
	})

	p := Parser{
		Accessor: accessor,
	}
	fds, err := p.ParseFiles("test.proto")
	testutil.Ok(t, err)
	extensionRangeEnd := fds[0].GetMessageTypes()[0].GetExtensionRanges()[0].End
	testutil.Eq(t, int32(internal.MaxTag), extensionRangeEnd)
}

func TestParseMessageSetWireFormatMaxExtensionValue(t *testing.T) {
	accessor := FileContentsFromMap(map[string]string{
		"test.proto": `
syntax = "proto2";
message Foo {
	option message_set_wire_format = true;
	extensions 4 to max;
}
`,
	})

	p := Parser{
		Accessor: accessor,
	}
	fds, err := p.ParseFiles("test.proto")
	testutil.Ok(t, err)
	extensionRangeEnd := fds[0].GetMessageTypes()[0].GetExtensionRanges()[0].End
	testutil.Eq(t, int32(internal.MaxTagMessageSetWireFormat), extensionRangeEnd)
}

[jhump]

I believe this is basically the same underlying issue as #275, which are both around undocumented details of "message set" wire format, particularly the fact that the allowed tag range is different with this encoding format.

[jhump]

I think I have a fix ready. It's a large-ish change because I decided to tackle some hygiene issues while in there (like trying to add some helper methods to make some of the gnarly error-handling slightly less verbose; and breaking up the over-sized parser.go into multiple files). It was a fairly intrusive change that involved deferring a lot of the validation (for example, we can't validate tag values until after we've processed options, to determine if the message has message_set_wire_format). So, since I was already tearing it up heavily, I decide to make a fairly large refactor.

While in there, I just happened to find a few other problems (#312, #313, and #314) and am fixing those as well. This change will also fix #307 -- which involved non-trivial changes to the grammar (proto.y), but luckily in a way that I think simplifies a lot of it. Also, deferring some of the validation made proto.y less verbose, which is overall a good thing. (It's still a beast -- this whole package is; the error-handling is the bit that bothers me the most, especially in the options-interpretation code, which is also super-complicated...)

[jhump]

Fixed in #316