"cannot infer type" from Deserialize derive macro with simple variants and untagged variants

[ahl]

This produces an error:

    #[derive(Deserialize)]
    #[serde(tag = "t")]
    enum Data {
        A,
        #[serde(untagged)]
        Var(u32),
    }

error[[E0282]](https://doc.rust-lang.org/stable/error_codes/E0282.html): type annotations needed
 --> src/lib.rs:3:10
  |
3 | #[derive(Deserialize)]
  |          ^^^^^^^^^^^ cannot infer type
  |
  = note: this error originates in the derive macro `Deserialize` (in Nightly builds, run with -Z macro-backtrace for more info)

For more information about this error, try `rustc --explain E0282`.
error: could not compile `playground` (lib) due to previous error

My proposed fix is intended to minimize changes to code unrelated to the changes from #2403.

[oli-obk]

if the type annotation is added here, we can keep sharing the logic between first_attempt and attempts

[ahl]

Indeed! As I noted, I was trying to minimize code change for code paths unrelated to the new untagged variant semantics, but I'm very happy to add the type annotation in all cases if that's preferable. Please let me know.

[dtolnay]

Thanks for the PR!

From skimming the generated code for the enum in your PR description, after this fix, it ends up looking like:

if let _serde::__private::Result::<_, __D::Error>::Ok(__ok) = {
    let (__tag, __content) = _serde::Deserializer::deserialize_any(...)?;
    let __deserializer = _serde::__private::de::ContentDeserializer::<__D::Error>::new(__content);
    match __tag {
        __Field::__field0 => {
            _serde::Deserializer::deserialize_any(__deserializer, ...)?;
            _serde::__private::Ok(Data::A)
        }
    }
} {
    return _serde::__private::Ok(__ok);
}

if let _serde::__private::Ok(__ok) = _serde::__private::Result::map(
    <u32 as _serde::Deserialize>::deserialize(__deserializer),
    Data::Var,
) {
    return _serde::__private::Ok(__ok);
}

_serde::__private::Err(_serde::de::Error::custom(...))

That's probably a bad sign if the first if let can only ever be Ok or early return. I don't think just adding a type fixes that bug.

For an externally tagged enum (which is the only case that #2403 tested) untagged variants work like this:

#[derive(Deserialize, Debug)]
pub enum Data {
    A,
    #[serde(untagged)]
    Var(u32),
}

fn main() {
    let a = r#" "A" "#;
    println!("{:?}", serde_json::from_str::<Data>(a));  // Ok(A)

    let var = r#" 1 "#;
    println!("{:?}", serde_json::from_str::<Data>(var));  // Ok(Var(1))
}

For an internally tagged enum, it fails to compile before this PR, and I think it has incorrect behavior after this PR.

#[derive(Deserialize, Debug)]
#[serde(tag = "t")]
pub enum Data {
    A,
    #[serde(untagged)]
    Var(u32),
}

fn main() {
    let a = r#" {"t":"A"} "#;
    println!("{:?}", serde_json::from_str::<Data>(a));  // Ok(A)

    let var = r#" 1 "#;
    println!("{:?}", serde_json::from_str::<Data>(var));  // Err
}

Could you or @dewert99 look into making this deserialize Ok(Var(1))?

Otherwise we should add an intentional error message that this case is not supported (untagged variant in internally tagged enum).

[ahl]

Thanks for taking a look @dtolnay

That's probably a bad sign if the first if let can only ever be Ok or early return. I don't think just adding a type fixes that bug.

That doesn't seem intrinsically problematic to me (i.e. someone new to this code). It seems like #2403 intended to reuse as much existing logic as possible--which seems like a reasonable goal.

Could you or @dewert99 look into making this deserialize Ok(Var(1))?

Oof. Nice catch. This is more problematic than I had realized. I think I've found a relatively simple fix, but there are some weird semantics here. First, here's the gist of the fix I've been testing out (wrapping first_attempt in a closure):

    // Ignore any error associated with non-untagged deserialization so that we
    // can fall through to the untagged variants. This may be infallible so we
    // need to provide the error type.
    let first_attempt = first_attempt.map(|expr| {
        quote! {
            if let _serde::__private::Result::<_, __D::Error>::Ok(__ok) = (|| {
                #expr
            })() {
                return _serde::__private::Ok(__ok);
            }
        }
    });

That is: ignore any early returns (in addition to terminal returns) from first_attempt so that we fall back to trying the untagged variants even if, say, the tag is missing.

There is (at least) one more problem: what to do if all variants fail to deserialize? With the change I proposed we get:

"data did not match any variant of untagged enum Data"

That's clearly not right since the enum isn't untagged... at least not in whole. Note that's also the error for an externally tagged enum with untagged variants, which seems like a different, but related, bug.

Without the untagged variants we get:

"missing field `tag`"

That clearly made sense before #2403 but I'm not sure it's sufficient now; also consider an (annoying) enum like this:

#[derive(Serialize)]
#[serde(tag = "potato")]
enum X {
    #[serde(untagged)]
    A { x: u32 },
    #[serde(untagged)]
    B { y: y32},
}

It's internally tagged... sort of? Or is it untagged since all the variants are untagged? How should we talk about it in error messages?

Perhaps the error message for an internal or adjacently tagged enum that contains untagged variants should instead be a little more explicit:

"missing field `tag` and data did not match any untagged variants"

For an externally tagged enum perhaps:

if the data is either a string or an object with a single:

unknown variant `XXX`, expected one of `YYY` ... and data did not match any untagged variants

otherwise:

expected value or data matching an untagged variant

I'm happy to pursue this, but would appreciate guidance regarding the proposed fix, error messages, and any tests you'd suggest I crib to improve the robustness of testing here.

I think the genie is already out of the bottle, but I do want to ask if untagged variants is a feature you want to commit to. I personally love it, but I can see that it may have added unanticipated complexity (along the lines of #1646 (comment)).

[ahl]

@dtolnay could you take another look? I think I've got the other enum tagging types covered; I would appreciate input about error messages in particular.

[ahl]

Howdy! Any advice for moving this PR forward?

[dtolnay]

Thank you!

I think the "data did not match any variant of untagged enum Data" error message is not a big deal — I wouldn't block releasing this fix on improving the message. But your suggested messages in #2613 (comment) make sense to me. I would be glad if someone wants to pursue those improvements.

Regarding robust testing, @Mingun can perhaps give guidance since they have been reorganizing and fleshing out the tests recently.