Perhaps Extend should not talk about updating an entry, but updating a value (if any)

The thing with Extend is that BTreeSet does NOT update entries (and we rely on this) but BTreeMap updates the value of an entry (but not the key).

Not really sure how you'd clarify the docs for this, but it gets confusing when your API relies on these properties and you're trying to describe them to other ppl, but the docs don't align with the implementation.

(Also should we be posting this here or opening an issue? Also thoughts on having BTreeSet.inserter and BTreeSet.replacer, where Inserter provides an Extend that doesn't update stuff, and Replacer does update stuff?)

By adding documentation comments on impl<..> Extend<...> for BTreeMap<...> explaining the current behavior. BTreeMap::insert has docs explaining that this is what happens when the querying key is for an existing entry.

/// If the map did have this key present, the value is updated, and the old
/// value is returned. The key is not updated, though; this matters for
/// types that can be `==` without being identical. See the [module-level
/// documentation] for more.

Also explained in the module-level documentation

//! If we have a more complex key, calls to `insert` will
//! not update the value of the key.```
1 Like

This sentence from Extend:

When extending a collection with an already existing key, that entry is updated or, in the case of collections that permit multiple entries with equal keys, that entry is inserted.

Should really be in a separate section that makes it clear it's talking about what the standard library types happen to do. (Or arguably removed from Extend and kept with each implementing type instead, to reduce the chance of future drift.) There's nothing inherent in the trait that says things have to be that way. Or that Self or its implementation of Extend have to have anything to do with keys at all.

5 Likes