Make Serializer and Deserializer pub and expose incomplete_enum mechanism

[jordens]

Summary

miniconf is a library we use to access (get/set over serial, mqtt, store in flash) settings on embedded devices. It is a serde-based hierarchical heterogeneous key-value access mechanism.
We'd like to generate a schema for the settings trees and found serde-reflection very useful for this (thanks!).

We implement functionality similar to trace_value/trace_type from Tracer using our intermediate miniconf::TreeSerialize/TreeDeserialize traits. See below for what that looks like.

For trace_value and trace_type_once we would like Serializer and Deserializer to be pub. Happy to split that out of this PR if it's uncontroversial.

For trace_type we also need a way to mark the top level enum in incomplete_enums before tracing again (see trace_type).

Downstream use case:
https://github.com/quartiq/miniconf/blob/c741c31b8fcd5e8769861c16081f0d6895e019a2/miniconf/src/trace.rs#L13-L63

Test Plan

CI

[ma2bd]

Interesting. We may want to mark the newly public function(s) as #[doc(hidden)].

[jordens]

I think I found a decent way to keep up the invariant that only if incomplete_enums is empty, a complete registry can be returned and I left out the doc hiding. AFAICT it would not have made a difference w.r.t. semver rules anyway.
Let me know if you still prefer to hide the new API.

[jordens]

@ma2bd anything I can do to advance this PR?

[ma2bd]

@ma2bd anything I can do to advance this PR?

Hi @jordens, Thanks for your patience. Couple of questions:

• Why do we need EnumProgress::Pending? I can't see the difference between removing an entry from incomplete_enums and marking it as EnumProgress::Pending. If there is a difference, would you be able to provide a unit test demonstrating the new functionality?
• The debug_assert! is also puzzling me. Are we worried about an infinite loop? But then why not assert! or better return an Error instead?

[jordens]

@ma2bd thanks for looking at this!

• To make use of an exposed Deserializer the user also needs access to incomplete_enums to signal that further variants should be traced. If the user then removes an enum from incomplete_enums the tracer state is inconsistent (until and unless it is re-traced): tracer.registry() at that point would succeed but be wrong. Before this PR that inconsistency only existed internally and for a short time:

  serde-reflection/serde-reflection/src/trace.rs

  Lines 255 to 260 in df819c4

  	let (format, value) = self.trace_type_once::<T>(samples)?;
  	values.push(value);
  	if let Format::TypeName(name) = &format {
  	if let Some(&progress) = self.incomplete_enums.get(name) {
  	// Restart the analysis to find more variants of T.
  	self.incomplete_enums.remove(name);

  during the interval from L260 to the next trace in L255. To make it always consistent for a downstream Deserializer user I need a way to distinguish the state "incomplete but marked for re-tracing" (i.e. EnumProgress::Pending) from the state "unknown or complete" (not present in incomplete_enums). I added a unittest showing the functionality and where the inconsistent state would show up. I tried to keep it readable. If you want, I can juggle the commits so that the test will actually show the wrong behavior before a subsequent commit fixes it.

• The infinite loop at that point was undetectable before. With the new EnumProcess::Pending it can now be detected. Since an Infinite loop here appears to imply wrong behavior in serde_reflection, I choose assert over returning an error. But I agree that the performance penalty is negligible and it should not be debug_assert. Changed.

[ma2bd]

@ma2bd thanks for looking at this!

• To make use of an exposed Deserializer the user also needs access to incomplete_enums to signal that further variants should be traced. If the user then removes an enum from incomplete_enums the tracer state is inconsistent (until and unless it is re-traced): tracer.registry() at that point would succeed but be wrong. Before this PR that inconsistency only existed internally and for a short time:

  serde-reflection/serde-reflection/src/trace.rs

  Lines 255 to 260 in df819c4

  	let (format, value) = self.trace_type_once::<T>(samples)?;
  	values.push(value);
  	if let Format::TypeName(name) = &format {
  	if let Some(&progress) = self.incomplete_enums.get(name) {
  	// Restart the analysis to find more variants of T.
  	self.incomplete_enums.remove(name);

  
  during the interval from L260 to the next trace in L255. To make it always consistent for a downstream Deserializer user I need a way to distinguish the state "incomplete but marked for re-tracing" (i.e. EnumProgress::Pending) from the state "unknown or complete" (not present in incomplete_enums). I added a unittest showing the functionality and where the inconsistent state would show up. I tried to keep it readable. If you want, I can juggle the commits so that the test will actually show the wrong behavior before a subsequent commit fixes it.
• The infinite loop at that point was undetectable before. With the new EnumProcess::Pending it can now be detected. Since an Infinite loop here appears to imply wrong behavior in serde_reflection, I choose assert over returning an error. But I agree that the performance penalty is negligible and it should not be debug_assert. Changed.

Thanks for your answer. I tried your unit test and it doesn't seem to require the new value Pending to pass. So I'm proposing #85