Field presence #247

snproj · 2023-02-27T15:37:08Z

Continuation of #243

Since I no longer have access to the ghpr-asia fork that I made that PR with.

Some minor formatting changes rolled back to reduce the diff
Custom default impls should now only be generated when there are custom default fields
Added --generate-getters to generate getters for Proto2 fields with custom defaults

Minor notes:

Forgot to mention in previous PR message, but custom default fields for strings and bytes must now begin and end with either single or double inverted commas (previously not enforced). I had to enforce this to predictably generate default const fields from user input.
The minor change from #[derive(Debug, Default, PartialEq, Clone)] to #[derive(Default, Debug, PartialEq, Clone)] is there because Default is now being inserted programmatically into the list of derives, so it was best to simply append to the start

Future action

Proto2 and Proto3 parsing will be made stricter; originally this was part of this PR (since the whole parsing system got restructured anyway), but it's probably better to separate it out. The changes are:
- Proto3 enums must begin with index 0
- Proto3 cannot have required fields
- Proto3 cannot have custom defaults

thomaseizinger

Nice work!

I only reviewed the generated code as I don't have any knowledge on how the parsing system works.

thomaseizinger · 2023-02-27T22:08:50Z

quick-protobuf/examples/pb_rs/data_types.rs

+impl<'a> Default for FooMessage<'a> {
+    fn default() -> Self {
+        Self {
+            f_int32: None,
+            f_int64: None,
+            f_uint32: None,
+            f_uint64: None,
+            f_sint32: None,
+            f_sint64: None,
+            f_bool: None,
+            f_FooEnum: None,
+            f_fixed64: None,
+            f_sfixed64: None,
+            f_fixed32: None,
+            f_sfixed32: None,
+            f_double: None,
+            f_float: None,
+            f_bytes: None,
+            f_string: None,
+            f_self_message: None,
+            f_bar_message: None,
+            f_repeated_int32: Vec::new(),
+            f_repeated_packed_int32: Vec::new(),
+            f_repeated_packed_float: PackedFixed::from(Vec::new()),
+            f_imported: None,
+            f_baz: None,
+            f_nested: None,
+            f_nested_enum: None,
+            f_map: HashMap::new(),
+            test_oneof: mod_FooMessage::OneOftest_oneof::None,
+        }
+    }
+}


Why is this one not derived?

Oversight in the impl Default system; have fixed it😅 thanks for catching

thomaseizinger · 2023-02-27T22:09:39Z

quick-protobuf/examples/pb_rs_nostd/protos/no_std.rs

+                w.write_packed_fixed_with_tag(18, &self.nums)?;
+        w.write_with_tag(26, |w| w.write_message(&self.message))?;
+                for s in &self.messages { w.write_with_tag(34, |w| w.write_message(s))?; }


Unnecessary indentation?

thomaseizinger · 2023-02-27T22:10:14Z

quick-protobuf/examples/pb_rs_v3/data_types.rs

+                w.write_packed_with_tag(154, &self.f_repeated_int32, |w, &m| w.write_int32(*&m), &|&m| sizeof_varint((m) as u64))?;
+                w.write_packed_with_tag(162, &self.f_repeated_packed_int32, |w, &m| w.write_int32(*&m), &|&m| sizeof_varint((m) as u64))?;
+                w.write_packed_fixed_with_tag(170, &self.f_repeated_packed_float)?;


Here too? Can we avoid this indentation?

thomaseizinger · 2023-02-27T22:11:56Z

quick-protobuf/tests/rust_protobuf/v2/test_default_values_test.rs

-    assert_eq!(b"cde\n33".to_vec(), d.bytes_field.to_vec());
-    assert!(EnumForDefaultValue::TWO.eq(&d.enum_field));
+    let d = TestDefaultValuesOptional::from_reader(&mut reader, bytes).unwrap();
+    assert_eq!(1.0, d.double_field.unwrap_or(TestDefaultValuesOptional::DEFAULT_double_field));


Shouldn't this assert be that it is None and a separate assert that DEFAULT_double_field == 1.0?

Yep, I think I originally wanted to test exactly what the user would write (i.e. obj.field.unwrap_or(Class::DEFAULT_field)) so I just wrote it in verbatim

Have changed it though since your version makes more sense haha

snproj · 2023-03-05T23:35:29Z

Made the changes as per above, also removed some extra parentheses in the generated get_size()es that previous versions of this PR added in.

Edit: cleaning up diff

…tidy up diff

snproj · 2023-03-06T02:08:53Z

I've spent a few hours trying to get the diff as small as possible for the get_size() and write_message() parts, but the way that the code generation has changed in this PR means that eliminating all the differences is hard without a bunch of error-prone rewriting (that will eventually be reverted anyway when I make the PR to clean it up, since I already have the code for that). I don't know that it's impossible, but I didn't want to delay too long haha

If you're curious, the previous code had control flow in various parts that determine when to add a dereference into the code, which had built up over time into generating a bunch of &&*&**&. This is the main source of diff, since I decided to rewrite the control flow to avoid these chains of references and dereferences; which makes it hard to artificially add them back in and shrink the diff.

Hopefully it's ok 🙏

thomaseizinger

LGTM! (Only reviewed generated code)

I'll ping a PR on our repo that needs these changes and have them try them out!

thomaseizinger · 2023-03-08T12:24:31Z

I've spent a few hours trying to get the diff as small as possible for the get_size() and write_message() parts, but the way that the code generation has changed in this PR means that eliminating all the differences is hard without a bunch of error-prone rewriting (that will eventually be reverted anyway when I make the PR to clean it up, since I already have the code for that). I don't know that it's impossible, but I didn't want to delay too long haha

If you're curious, the previous code had control flow in various parts that determine when to add a dereference into the code, which had built up over time into generating a bunch of &&*&**&. This is the main source of diff, since I decided to rewrite the control flow to avoid these chains of references and dereferences; which makes it hard to artificially add them back in and shrink the diff.

Hopefully it's ok pray

Yeah that is fine. There is a means to an end when it comes to reducing diff :)

thomaseizinger · 2023-03-08T12:24:45Z

libp2p/rust-libp2p#3455 uses this branch and it works well, green light from our end!

d-roak · 2023-05-01T03:37:58Z

Hi @snproj!

Can you do a version bump in the near future to include this change?

Thanks 🙏

mxinden · 2023-05-15T02:17:47Z

Friendly ping. Could you cut a new release with this patch?

Add field presence

c351684

snproj mentioned this pull request Feb 27, 2023

Add field presence #243

Closed

Merge branch 'ghpr-field-presence' into field-presence

cae3446

snproj force-pushed the field-presence branch from 37a0a16 to cae3446 Compare February 27, 2023 16:00

thomaseizinger reviewed Feb 27, 2023

View reviewed changes

thomaseizinger mentioned this pull request Mar 3, 2023

feat(relay): upgrade circuit relay v2 to proto3 according the spec libp2p/rust-libp2p#3455

Closed

4 tasks

fix default derive, tidy up tests and generated code

f237972

snproj marked this pull request as draft March 5, 2023 23:46

fix custom default not showing up in get_size() and write_message(), …

d5f0f04

…tidy up diff

snproj marked this pull request as ready for review March 6, 2023 01:56

thomaseizinger reviewed Mar 6, 2023

View reviewed changes

snproj mentioned this pull request Mar 6, 2023

Default values not respected #216

Open

snproj merged commit 2f37d5a into tafia:master Mar 20, 2023

thomaseizinger mentioned this pull request Oct 1, 2023

Release a new version #252

Open

thog92 mentioned this pull request Jan 25, 2024

perftest: missing field in Config #257

Merged

jxs mentioned this pull request Aug 3, 2024

Status and future maintenance of the project #263

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Field presence #247

Field presence #247

snproj commented Feb 27, 2023 •

edited

Loading

thomaseizinger left a comment

thomaseizinger Feb 27, 2023

snproj Mar 5, 2023

thomaseizinger Feb 27, 2023

thomaseizinger Feb 27, 2023

thomaseizinger Feb 27, 2023

snproj Mar 5, 2023 •

edited

Loading

snproj commented Mar 5, 2023 •

edited

Loading

snproj commented Mar 6, 2023

thomaseizinger left a comment

thomaseizinger commented Mar 8, 2023

thomaseizinger commented Mar 8, 2023

d-roak commented May 1, 2023

mxinden commented May 15, 2023

Field presence #247

Field presence #247

Conversation

snproj commented Feb 27, 2023 • edited Loading

Continuation of #243

Minor notes:

Future action

thomaseizinger left a comment

Choose a reason for hiding this comment

thomaseizinger Feb 27, 2023

Choose a reason for hiding this comment

snproj Mar 5, 2023

Choose a reason for hiding this comment

thomaseizinger Feb 27, 2023

Choose a reason for hiding this comment

thomaseizinger Feb 27, 2023

Choose a reason for hiding this comment

thomaseizinger Feb 27, 2023

Choose a reason for hiding this comment

snproj Mar 5, 2023 • edited Loading

Choose a reason for hiding this comment

snproj commented Mar 5, 2023 • edited Loading

snproj commented Mar 6, 2023

thomaseizinger left a comment

Choose a reason for hiding this comment

thomaseizinger commented Mar 8, 2023

thomaseizinger commented Mar 8, 2023

d-roak commented May 1, 2023

mxinden commented May 15, 2023

snproj commented Feb 27, 2023 •

edited

Loading

snproj Mar 5, 2023 •

edited

Loading

snproj commented Mar 5, 2023 •

edited

Loading