The feature preserve_order is not "purely additive," which makes it impossible to use serde_yaml 0.5.0 and clap in the same program

[emk]

Thank you for writing a great YAML parser for Rust! I'm working on several programs which make heavy use of it.

I ran into a rather tricky problem this morning, when I tried to use two Rust crates in the same program. And I'd love to get your feedback on it:

• serde_yaml 0.5.0 requires that yaml-rust be configured with the feature preserve_order enabled.
• clap requires that yaml-rust be configured with preserve_order disabled.

Trying to use both crates in the same program will turn on preserve_order and break clap:

error[E0308]: if and else have incompatible types
   --> /home/emk/.cargo/registry/src/github.com-1ecc6299db9ec823/clap-2.18.0/src/args/group.rs:463:30
    |
463 |         let group_settings = if b.len() == 1 {
    |                              ^ expected struct `linked_hash_map::LinkedHashMap`, found struct `std::collections::BTreeMap`
    |
    = note: expected type `&linked_hash_map::LinkedHashMap<yaml_rust::Yaml, yaml_rust::Yaml>`
    = note:    found type `&'a std::collections::BTreeMap<yaml_rust::Yaml, yaml_rust::Yaml>

I asked the Cargo maintainers on IRC if there was any workaround for this, and they said:

<sfackler> ekidd: features are normally supposed to be purely additive because of problems like these
<sfackler> seems like something you'd have to bug the yaml-rust people about unfortunately

As I understand it, this means that if a feature is enabled, it shouldn't change any of the existing APIs of the crate. I'm not sure what the best solution is here.

I've filed related issues against the affected projects: clap-rs/clap#747 dtolnay/serde-yaml#33

Thank you for any suggestions you can provide, and for writing a great YAML parser for Rust!

[chyh1990]

I think making 'preserve-order` as a default feature (or a must) will solve the problem. Few users has any reasons to disable this feature.

What do you think? I would also like to hear from clap-rs maintainers.

[emk]

What do you think?

Sadly, I don't think this will be enough to avoid all problems (though it might help a little bit in some common cases).

If I understood sfackler correctly, then turning on a feature can add new APIs. But if it changes or removes APIs, then cargo can't handle it correctly, and it may be impossible to use the library successfully in some projects.

I wish I had a better answer here. Maybe it would be possible to offer both versions in the same API somehow? Or to always provide the ordered version, and to have a feature to turn on the unordered version without turning off the ordered version?

Another possibility would be to provide just the ordered version all the time. This would be a breaking API change, so it would require releasing an 0.4.0 version.

Those are the only ideas I can think of right now. There might be somebody on #cargo on the Mozilla IRC who could suggest other ideas.

Anyway, thank you very much for releasing this library. It's extremely useful, and a lot of other great libraries rely on it!

[dtolnay]

+1 for always using LinkedHashMap without an option to use BTreeMap. I used to provide a preserve_order feature flag in serde_yaml but dropped it in favor of always preserve_order. The behavior is more intuitive when serializing structs: fields are serialized in the same order they are defined in Rust, which matches the behavior of other Serde formats. Preserve order also allows a YAML document to be deserialized and re-serialized without affecting the order of map keys.

[dtolnay]

While we're considering the map type anyway, OrderMap seems promising. I opened indexmap-rs/indexmap#9 to ask whether there are any disadvantages compared to LinkedHashMap.

[emk]

CC @kbknapp There's a discussion that might be relevant for clap-rs/clap#747 happening on #44.

I would personally be OK with only supporting LinkedHashMap or OrderMap, even if it cost some performance. But I don't know everybody else's use cases.

[bluss]

OrderMap is immature (no reserve, shrink_to_fit, no .drain()) and obviously not a drop in for linkedhashmap, but it could be useful if you just want to preserve insertion order (just don't do any deletions, and it's preserved), not keep a double ended queue like linked hash map does.

What are the common operations on it? If you know the number of key-value pairs to insert up front, I think insertion is the same cost as HashMap, if you grow dynamically, OrderMap should win slightly. OrderMap's iterators are very efficient (compared to HashMap), so if that is the main workload, then it could be a huge win.

[kbknapp]

I'm not against changing clap to use the preserve_order feature, but unfortunately doing so is breaking change for me and something I can't take lightly. I've had a 3.x version on the backburner for a while, and could make that change then, but it probably won't be for another few months until I have enough time to iron out the 3.x branch.

The temporary work around is for clap users to pin serde_yaml to 0.4.1 until I can make the breaking change and bump the major version.

[ThetaSinner]

I've come across a conflict trying to use log4rs, which imports serde_yaml which in turn always has preserve_order enabled, and config-rs which hard-codes BTreeMap and doesn't expect preserve_order to be enabled. This means I can use one library and not the other.
Anyhow, I was wondering what the progress on the PR is, the required change appears to have been made but isn't building and hasn't been accepted?

[robinst]

This has been closed, but it doesn't seem to be in a release yet? So dependent crates can't use the fix yet. Any plans for when that's gonna happen?