The `unreachable!()`s in `de.rs` are reachable

[nagisa]

It is possible to make this crate to panic when supplying malformed configuration options. We do have a fair amount of wrapper around this crate, so it is not trivial to minimize the reproducer, so here's a broad approximation of what we have and how this happens:

struct Config { 
    enum: EnumConfig,
}

enum EnumConfig {
    Foo,
    Bar { filename: std::path::PathBuf },
}

which then works fine when the following toml is supplied:

enum.bar.filename = "blah"

and will also work this way:

enum = "foo"

but will panic when the configuration is mutated to be more along the lines of:

enum = "bar"

---

The relevant portion of the stack trace is:

thread 'main' panicked at 'internal error: entered unreachable code', ~/.cargo/registry/src/github.com-1ecc6299db9ec823/config-0.9.3/src/de.rs:459:18

<snip>

  11: std::panicking::begin_panic
             at /rustc/5c5b8afd80e6fa1d24632153cb2257c686041d41/src/libstd/panicking.rs:400
  12: <config::de::EnumAccess as serde::de::VariantAccess>::struct_variant
             at ./<::std::macros::panic macros>:3
  13: <crate::builder::_IMPL_DESERIALIZE_FOR_EnumConfig::<impl serde::de::Deserialize for crate::builder::EnumConfig>::deserialize::__Visitor as serde::de::Visitor>::visit_enum
             at src/builder.rs:13
  14: config::de::<impl serde::de::Deserializer for config::value::Value>::deserialize_enum
             at ~/.cargo/registry/src/github.com-1ecc6299db9ec823/config-0.9.3/src/de.rs:254
  15: crate::builder::_IMPL_DESERIALIZE_FOR_EnumConfig::<impl serde::de::Deserialize for crate::builder::EnumConfig>::deserialize
             at src/builder.rs:13
  16: <core::marker::PhantomData<T> as serde::de::DeserializeSeed>::deserialize
             at ~/.cargo/registry/src/github.com-1ecc6299db9ec823/serde-1.0.102/src/de/mod.rs:786
  17: <config::de::MapAccess as serde::de::MapAccess>::next_value_seed
             at ~/.cargo/registry/src/github.com-1ecc6299db9ec823/config-0.9.3/src/de.rs:358
  18: serde::de::MapAccess::next_value
             at ~/.cargo/registry/src/github.com-1ecc6299db9ec823/serde-1.0.102/src/de/mod.rs:1847
  19: <crate::_IMPL_DESERIALIZE_FOR_Config::<impl serde::de::Deserialize for crate::Config>::deserialize::__Visitor as serde::de::Visitor>::visit_map
             at src/main.rs:13
  20: config::de::<impl serde::de::Deserializer for config::value::ValueWithKey>::deserialize_any
             at ~/.cargo/registry/src/github.com-1ecc6299db9ec823/config-0.9.3/src/de.rs:26
  21: config::de::<impl serde::de::Deserializer for config::value::ValueWithKey>::deserialize_struct
             at ./<::serde::macros::forward_to_deserialize_any_method macros>:7
  22: crate::_IMPL_DESERIALIZE_FOR_Config::<impl serde::de::Deserialize for crate::Config>::deserialize
             at src/main.rs:13
  23: config::config::Config::get
             at ~/.cargo/registry/src/github.com-1ecc6299db9ec823/config-0.9.3/src/config.rs:164

<snip>

---

I recommend replacing all of these unreachables with proper errors.

[matthiasbeyer]

If this is still valid, would you care to submit a PR for this or either close this issue?

[matthiasbeyer]

So this adds a test for your case here:

https://github.com/matthiasbeyer/config-rs/commit/fbbfdc2045bbdeccf5fbc5122acf0e7faf6e2dde

But I have not enough details. The not-to-be-failing tests here fail, the one that should panic does so, but I guess not in the way you'd expect.

Could you please provide more context or maybe a patch? I'd like to have tests for this case that can then be fixed (fixing this bug test-driven).

[Aetf]

I think I've also bitten by this bug. But I'm using the YAML format as input.

@matthiasbeyer I think the issue with your test cases is name casing. It should be

e.Bar.filename = "blah"
e = "Foo"
e = "Bar"

in respective cases.

Basically, when deserializing an enum variant struct, the input could be any of the following (written in JSON as it's more clear what the input structure is):

{
  "case1": "Bar",
  "case2": {
    "Bar": null
  },
  "case3": {
    "Bar": {}
  },
  "case4": {
    "Bar": { "filename": "blah" }
  }
}

and the library should not panic and should report error in case 1-3.

[matthiasbeyer]

I think the issue with your test cases is name casing. It should be e.Bar.filename = "blah" e = "Foo" e = "Bar"

Just tested, doesn't change anything.

I'd be really happy if you could help with this bug (implementing proper tests that actually fail and of course if possible a fix).

[Aetf]

I'm currently short of time due to an upcoming meeting. I can try to get a proper reproduction in a few days.

[Aetf]

@matthiasbeyer Here are the actually failing test cases: dfb1b337f311f8dbc4b80e7af3f0ecc0b6225511. They are the same cases with name casing changed to Foo and Bar.

The #[should_panic] on the third case actually masks the panic, removing it makes the test case fail as expected.