[std][BTree] Fix behavior of ::append to match documentation, ::insert, and ::extend

[tinnamchoi]

Resolves #145614

[rustbot]

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[Mark-Simulacrum]

BTreeMap::append is documented as:

Moves all elements from other into self, leaving other empty.

If a key from other is already present in self, the respective value from self will be overwritten with the respective value from other.

This (IMO) leaves ambiguous whether equal (by Eq/Ord) keys are updated or not in the implementation. Today, append will update the key as well. This PR is proposing that we align the behavior with insert's, which is documented as not updating the key:

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.

This is a breaking change, so nominating for libs-api. We can try running crater, but I'm not sure we'll find much signal given the specific conditions needed to trigger it (BTreeMap, using append, having keys with extra metadata, and writing a test reachable in Crater that exercises this case).

HashMap's extend impl uses insert internally, so it matches insert behavior. BTreeMap's extend also calls BTreeMap::insert in a loop and so does not match append today.

@tinnamchoi, can you also update the docs for append to reflect this change?

[joshtriplett]

@rfcbot merge

[rfcbot]

Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members:

• @Amanieu
• @BurntSushi
• @dtolnay
• @joshtriplett
• @m-ou-se
• @the8472

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[joshtriplett]

We discussed this in today's @rust-lang/libs-api meeting. We'd like to make this change, hence the FCP to confirm consensus.

People did express a desire to get whatever signal crater can give us, but it might not be worth a dedicated crater run; perhaps we should just rely on the beta crater run?

[Mark-Simulacrum]

Beta crater is probably too noisy (especially for tests) for this purpose, dedicated run then seems better. @bors try

[rust-bors]

☀️ Try build successful (CI)
Build commit: 16fbf98 (16fbf98f22730e073c09e7b03f0eafb87a295545, parent: 160e7623e8cbbf1feab2b6e2a24733a98c7bde9c)

[Mark-Simulacrum]

@craterbot run mode=build-and-test

[craterbot]

👌 Experiment pr-145628 created and queued.
🤖 Automatically detected try build 16fbf98
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more