search

tafia / quick-protobuf

A rust implementation of protobuf parser
MIT License
452 stars 87 forks source link

Fix nested lifetime search #119

Closed tafia closed 6 years ago

tafia commented 6 years ago

Close #118

tafia commented 6 years ago

Thanks for the review. I have pushed another change which should work. I have also rebased it. Sorry for the buggy fix and thank you for your patience.

jimblandy commented 6 years ago

Sure thing! I actually had a good time trying to find the counter-example...

tafia commented 6 years ago

Do you think it is ok to merge or do you want to play a little with it first?

jimblandy commented 6 years ago

If the tests pass I'm sure it's an improvement on the status quo, but there's something about it I still don't get. I'll look at it more when I get into work this morning, either way.

jimblandy commented 6 years ago

Although, I think there's a much simpler and more useful approach to this problem, based on the limitations of the protobufs language itself. Filed #121 for that.

tafia commented 6 years ago

Ok I have followed your advice on #121. I have also written Tarjan's algorithm to detect all strongly connected component within which cycles are. All optional fields are boxed within a cycle.

tafia commented 6 years ago

Test are now failing because there are some cycles with required fields only. I personally think it is good that it fails (the proto file itself must be amended). Of course this is a big breaking change but it is because of unsound messages.

What do you think?

tafia commented 6 years ago

Ok so I have added a new config option so user can decide to error on wrong messages if needed. By default it does not which means it will eventually change a required field into an optional one.

jimblandy commented 6 years ago

Are these cycles of required fields only present in test input, or are they actually used in the real world? If it's just the tests, then it seems fine to fix the tests.

But if they're used in the real world, it's important for pb-rs to support existing usage. But, how is it even possible to send such a message? Are those message types all unused?

I noticed that protoc doesn't forbid cycles of required fields, so I guess I shouldn't be surprised that they exist in the wild.

I haven't looked closely at the rest of pb-rs; does it ever print warnings? Perhaps the most helpful thing would be to always warn about cycles of required fields, to help people developing new protocols. Then, we could have options to silence the warnings, or treat them as errors, the way other compilers do.

tafia commented 6 years ago

Are these cycles of required fields only present in test input, or are they actually used in the real world? If it's just the tests, then it seems fine to fix the tests.

This is hard to answer that. I believe that if protoc indeed is boxing required fields too, then there may be some wrong file in the wild.

Having a warning should probably be enough for the time being. pb-rs will output warnings per default with a config parameter to error out on invalid proto file (not the default).