add missing visit_enum

[frederik-uni]

fixes surrealdb/surrealdb#4844 and probably dtolnay/serde-yaml#344

[frederik-uni]

@dtolnay is there an issue with this request?

[sagarrabadiya]

I can confirm this PR fixes surrealdb/surrealdb#4844 currently stuck with untagged variant deserialize by adding this method code into crate it works as expected without this method untagged enum with input data doesn’t deserialize and throws an error

when will this PR be merged?

[frederik-uni]

@nicolube as it was mentioned above it doesn’t really make sense. It's only a fix that works for the specific cases mentioned above but won’t work for custom deserializers.

/// Called when deserializing a struct-like variant.
data.struct_variant(fields, visitor);
/// Called when deserializing a variant with a single value.
data.newtype_variant();
/// Called when deserializing a tuple-like variant.
data.tuple_variant(len, visitor);
/// Called when deserializing a variant with no values.
data.unit_variant();

I dont see a way to determine what variant the value is. newtype_variant works out for the issues we are encountering with surrealdb, but there could be cases where the other functions would be needed.

[frederik-uni]

tests for this will be hard because the default behavior doesnt change.

I tested it with a serde-content fork

fn hint(&self) -> Option<VariantHint> {
        Some(match &self.data {
            Some(Value::Map(_)) => VariantHint::Struct(&[]),
            Some(Value::Seq(seq)) => VariantHint::Tuple(seq.len()),
            Some(_) => VariantHint::Newtype,
            None => VariantHint::Unit,
        })
    }

@Mingun how would you write tests for it?

[oli-obk]

tests for this will be hard because the default behavior doesnt change.

if you change the deserializers in https://github.com/serde-rs/serde/blob/master/serde/src/de/value.rs#L264 to support this, is there anything else missing from https://github.com/serde-rs/serde/blob/master/test_suite/tests/test_de.rs that you'd need to write a test?

[frederik-uni]

it doesn’t really make sense. It's only a fix that works for the specific cases mentioned above but won’t work for custom deserializers.

can you elaborate on what doesn't make sense? I am missing context I think

the original code was

Ok(Content::Map(
                [(
                    Content::String(key),
                    tri!(data.newtype_variant::<Self::Value>()),
                )]
                .into(),

I added the type hint so the right function is called

let data = match variant_hint {
                de::VariantHint::Unit => Content::Unit,
                de::VariantHint::Newtype => tri!(data.newtype_variant()),
                de::VariantHint::Tuple(len) => tri!(data.tuple_variant(len, self)),
                de::VariantHint::Struct(fields) => tri!(data.struct_variant(fields, self)),
            };

So its fixed now

[frederik-uni]

tests for this will be hard because the default behavior doesnt change.

if you change the deserializers in https://github.com/serde-rs/serde/blob/master/serde/src/de/value.rs#L264 to support this, is there anything else missing from https://github.com/serde-rs/serde/blob/master/test_suite/tests/test_de.rs that you'd need to write a test?

I dont see me writing tests. I am not that familiar with how serde works internally and I just wanted to fix an issue I had with surrealdb. The implementation was missing completely and the use case of this function is a very specific case that seems to only happen when using flatten. If someone wants to pick it up and write the tests for it please do.

[Mingun]

@frederik-uni, the goal to is write tests to cover all possible code paths in your new code. So you need to figure out what conditions is needed for that and construct corresponding input.

[frederik-uni]

@Mingun the untagged feature has no existing tests. there is a folder in the test_suite labeld regression but there arent any tests. I am not familiar enough with the codebase to be able to create a deserializer in the test_suite that has the new hint. Thats why i said if someone wants to pick it up they are welcome to. I literally dont know how to write the test. the primitive_deserializer has no uses in the test_suite. Thats why i said i wont implement it. I just dont know how and its not worth the time investment.

BTW i dont get how a primitive_deserializer would help test the flatten feature but ok

[Mingun]

@Mingun the untagged feature has no existing tests

No, it has:
https://github.com/serde-rs/serde/blob/b9dbfcb4ac3b7a663d9efc6eb1387c62302a6fb4/test_suite/tests/test_enum_untagged.rs

Most tests define some serializable struct and by using serde_test (de)serializer asserts that:

• struct is serialized to the some list of tokens. Each token roughly represents the serde API call of a Serializer;
• struct is tried to deserialize from some list of tokens, which roughly represents the serde API call of a Deserializer / Visitor. Usually several different representations are allowed (for example, for tagged enums either tag or content can be first), so test tries to deserialize from several representations.

Each token also roughly represents some serialization construct, for example, string, number, or mapping in JSON. So if you aware of the structure that you want to deserialize, you may construct token list corresponding to it and check that serde is able to deserialize from it.

[frederik-uni]

@Mingun it seems like i cant reproduce the bug using assert_tokens. i guess the visit_enum function isnt used internally & serde-content visited the enum differently than it was meant to

[frederik-uni]

@Mingun is there a test that specifically tests the visit functions for Content? I couldn't reproduce the issue because its for a specific case where the enum type is unknown.

[Mingun]

What specific methods you mean? If you want to figure out which tests covers methods, just add panic in them and see what tests failed. Then you can examine those tests to understand how they can be modified to reach lines you want.

[frederik-uni]

@Mingun where is visit_enum in private::Content tested?

[Mingun]

As I say, you can just insert panic!(); here and see that there is no tests for this method:

serde/serde/src/private/de.rs

Lines 528 to 535 in a685dcf

	fn visit_enum<V>(self, _visitor: V) -> Result<Self::Value, V::Error>
	where
	V: EnumAccess<'de>,
	{
	Err(de::Error::custom(
	"untagged and internally tagged enums do not support enum input",
	))
	}

Because error message say us that this error is expected in context of untagged or internally tagged enums, start looking at tests for that representations. Search for untagged and tag, because this is the words used in serde attributes to annotate enum as internally tagged or untagged.

Also, you could try to panic inside another method of ContentVisitor, for example here:

serde/serde/src/private/de.rs

Lines 514 to 526 in a685dcf

	fn visit_map<V>(self, mut visitor: V) -> Result<Self::Value, V::Error>
	where
	V: MapAccess<'de>,
	{
	let mut vec =
	Vec::<(Content, Content)>::with_capacity(
	size_hint::cautious::<(Content, Content)>(visitor.size_hint()),
	);
	while let Some(kv) = tri!(visitor.next_entry()) {
	vec.push(kv);
	}
	Ok(Content::Map(vec))
	}

This gives you following failures (cargo +nightly test --features unstable in ./test_suite):

failures (in test_annotations):
    flatten::enum_::adjacently_tagged::newtype
    flatten::enum_::adjacently_tagged::struct_
    flatten::enum_::externally_tagged::newtype
    flatten::enum_::externally_tagged::struct_from_map
    flatten::enum_::untagged::struct_
    flatten::unknown_field
    test_partially_untagged_enum
    test_partially_untagged_enum_desugared
    test_partially_untagged_enum_generic
    test_partially_untagged_internally_tagged_enum

So look at those tests and think how they could be modified to reach visit_enum instead of visit_map. General considerations tell us that most likely somewhere in the deserialized type there should be reference to an enum type. You can also note, that this approach was more effective, because ContentVisitor used also for #[serde(flatten)] fields and that is another place for experiments.