Custom Options with more than one required field can't be used

[GoogleCodeExporter]

message_descriptor.proto:
import "google/protobuf/descriptor.proto";
package Messages;
message MessageVersion {
    required int32 main_version = 1;
    required int32 sub_version = 2;
}

message MessageOptions {
    optional MessageVersion message_version = 1;
}

extend google.protobuf.MessageOptions {
    optional MessageOptions message_options = 50000;
}

testmessage.proto
import "Messages/message_descriptor.proto";
package Messages.TestMessage;
message Test {
    option (Messages.message_options).message_version.main_version = 1;
    option (Messages.message_options).message_version.sub_version = 1;

    optional string testField = 1;
}

What is the expected output? What do you see instead?
I would expect the version to be initialized. But it will crash since the 
MessageVersion is initialized with main_version, still missing sub_version.

What version of the product are you using? On what operating system?
I'm using Revision d3c7c6a1e16e, the newest one.

Please provide any additional information below.
It fails at UnknownFieldSet.MergeFieldFrom on the call of:
builder[field] = subBuilder.WeakBuild();

Calling only WeakBuildPartial(); will resolve the problem since the 
initialization check get's delayed. If nothing else changed, it will delay it 
until DescriptorProtoFile.ParseFrom called on the FileDescriptor and fail at 
BuildParsed with a rather ugly message.

A way to solve this is getting the same exception as before is to check if a 
field was initialized after each message is generated by changing the code 
generator. I changed DescriptorProtoFile.MergeFrom for testing, adding 
following code directly before the last return of the method:

foreach (IMessage field in this.AllFields.Values)
{
  if (!field.IsInitialized)
    field.WeakToBuilder().WeakBuild();
}

This triggers the same exception as before. Alternativeley (and maybe even 
better) is throwing the exception directly to avoid creating a new builder:

foreach (IMessage field in this.AllFields.Values)
{
  if (!field.IsInitialized)
    throw new UninitializedMessageException(MessageBeingBuilt);
}

Have i forgot something or should this work?

Original issue reported on code.google.com by falco20019 on 7 Jan 2015 at 4:59

[GoogleCodeExporter]

Ok, the proposed method is not working since it messes with the static 
initialization... Looking forward to another solution. Replacing the required 
fields with optional since then.

Original comment by falco20019 on 7 Jan 2015 at 6:02

[GoogleCodeExporter]

I have solved it now with setting the call of WeakBuild() to WeakBuildPartial 
as described above and with improving readability of 
UninitializedMessageException. See applied patch.

Original comment by falco20019 on 9 Jan 2015 at 10:18

Attachments:

• patch.diff

[GoogleCodeExporter]

Hi Falco20019,

Thanks for looking into this diligently and providing a fix. I haven't had a 
chance to look at it closely myself yet (and I'm too tired to do so right now) 
but in the meantime, can I check whether you've already signed the Google CLA? 
We'll need a CLA before I can accept your patch, I'm afraid. See 
https://cla.developers.google.com/clas for details.

Original comment by jonathan.skeet on 10 Jan 2015 at 3:51

[GoogleCodeExporter]

Just signed the CLA. Please look into the code before applying the patch since 
I am not that deep into the whole code. I just checked if I can see any 
dependencies but haven't seen any other problems with the change of WeakBuild 
to WeakBuildPartial.

Original comment by falco20019 on 13 Jan 2015 at 3:51

[GoogleCodeExporter]

Hi Falco20019,

Just to say I haven't forgotten about this. I need to get back into the code 
myself, and it's proving tricky to find a good slot of time to do that - but I 
promise it's on my list!

Original comment by jonathan.skeet on 31 Jan 2015 at 9:01

[GoogleCodeExporter]

We've now had a look at this, and understood it better. Basically, it's working 
as intended - but that doesn't mean you can't use an option with two required 
fields. It just means you need to use the syntax which specifies the option 
value as a complete message, rather than one field of it at a time. So you want:

    message Test {

       option (Messages.message_options)).message_version = {
           main_version: 1,
            sub_version: 1
        };

        optional string testField = 1;
    }

Hopefully this will meet your requirements - it feels like a much cleaner way 
to specify it, to be honest.

Original comment by jonathan.skeet on 22 Feb 2015 at 8:23

• Changed state: WontFix