tafia / quick-protobuf

A rust implementation of protobuf parser
MIT License
446 stars 82 forks source link

fix deprecated lifetimes showing up in generated structs #221

Closed snproj closed 1 year ago

snproj commented 1 year ago

Problem

Message fields that had lifetimes, but which were deprecated, were still contributing their lifetime parameters into the system. For example, the following .proto file:

message ThisShouldNotHaveALifetimeParameter {
    optional string dep = 1 [deprecated=true]; // since deprecated, should not cause lifetime parameter
}

Would cause the following faulty rust struct to be generated:

  pub struct ThisShouldNotHaveALifetimeParameter<'a> {
  }

Solution

Adding an extra condition to the check in has_lifetime() in message.rs fixes this:

let res = self
             .all_fields()
-            .any(|f| f.typ.has_lifetime(desc, f.packed(), ignore));
+            .any(|f| f.typ.has_lifetime(desc, f.packed(), ignore) && (!f.deprecated || config.add_deprecated_fields));
         ignore.pop();
         res

This necessitated config: &Config to be passed as parameters to many functions within types.rs.

Test cases were also added that would check for the absence of lifetime parameter by using the trybuild crate to assert a compilation failure (fails when the lifetime parameter is added).

Duplication

Some test files (test_deprecated_lifetime.rs, test_deprecated_lifetime_pb.proto, etc.) have been duplicated for both v2 and v3. I couldn't find a way to combine them (e.g. under common) in a way that would meaningfully decrease duplication, while still keeping separate tests for v2 and v3 (do let me know if I'm missing something).

Hex

Small addition to hex.rs to help type inference.

Version number and Dependencies

pb-rs updated to 0.9.2 quick-protobuf not updated, since trybuild is a dev-dependency and should not affect public API