Change `remove` to `unstable_remove`

[CastilloDel]

The actual name of remove isn't clear enough about the consequences of using it. As can be seen in rust-lang/rust#102674. It can lead to confusion, thinking that even when calling remove the order will still be kept.

It could be changed to unstable_remove or unstable_swap_remove to made clear that the insertion order will be disrupted.

shift_remove could also be renamed to remove if deemed desirable.

I find it is better to have worse performance, than a bug. The performance can be improved if needed, but a bug can be difficult to find.

[cuviper]

IMO, swap_remove is already giving a fair hint that it's disrupting order, and for a time we did have remove marked deprecated -- that was added in #99 / 0c24d10, and backed out in #107 / 268665b. I'd be in favor of adding the deprecations back, given your evidence of confusion.

The master branch is on 2.0, which still hasn't been published yet, so we could make real breaking changes here. However, given the goal of "drop-in replacement for map/set", I think it's important to have something here on the original name, but both swap and shift have their drawbacks. Deprecation serves to let you drop-in but then realize that a decision is needed, which isn't as noticeable just from docs.

[CastilloDel]

Okay, I see this has already been discussed. I had searched for issues, but I didn't think there would be a discussion about this directly in a PR 😥. I will give my opinion, just in case it is useful:

I agree with @/Mark-Simulacrum here and with you here. I feel like it is a bigger footgun not to have the expected behavior than to have worse performance.

I get the argument about the deprecation kinda defeating the purpose of indexmap being a drop-in replacement. IMO it would be acceptable to have a worse perf when adding it as a drop-in and being able to improve performance if necessary after (by changing remove to shift_remove if possible). But I still think the deprecation isn't a bad idea. The change from remove to one of the explicit variants can probably be done easily workspace wide with a single replace.

Anyway, if this has already been discussed, it will be okay if you just close this issue!

[cuviper]

Note that shift_remove is also disruptive: it preserves order, but it decrements all indexes of items after the removal point. I don't know if that's more or less surprising for someone using this as a drop-in, if they're coming from normal hash maps that are neither ordered nor indexable.

@bluss, has time altered your opinion about this?

[CastilloDel]

Hmmm I didn't know that. I guess that using it as a drop-in it would be more common to want to want to preserve the order than the indexes, but I may be biased in this opinion

[ClementNerma]

Upvoting for a (re-)deprecation of the .remove() method. Just bit my hand with this one and searched literally for hours as to why there was a bug in my application - it was caused by the de-ordering due to a call to .remove(). Replacing it with .shift_remove() resolved the issue.

Or .remove() should preserve order, and act like .shift_remove(). After all, BTreeMap keeps the elements ordered when using this method. But I don't think this would be a wise idea because it would break existing code's behaviour.