Remove deprecation warnings on the removal methods again

[bluss]

Note, these methods are never scheduled to be removed. The warning was
intended to guide the user towards being more explicit about which
removal type they wanted (swap_remove or shift_remove).

However, seeing how indexmap can be used as a drop in solution for
HashMap, I again am dubious that these warnings should be there.

Documentation for the methods has been improved.

[bluss]

I'd like to do this, I guess it's a change of course but it's what feels best for me. My impression is taken from how rustc is using IndexMap as a HashMap drop in, so then I also wonder what the preference would be from them @Centril, if you know? 🙂 Would it be best with the friendly "Deprecation" warnings on plain ".remove()" or not?

[Centril]

uhm... I've mostly approached IndexMap as an ordinary user in rustc so idk...

cc @nikomatsakis @Mark-Simulacrum

[Mark-Simulacrum]

I think rustc doesn't care much -- if we want to be specific about this we can always introduce our own lint around it or so.

It's true right that swap_remove is entirely deterministic for a given insertion/deletion order? i.e., these functions default to a reasonable thing already from the perspective that "IndexMap is deterministic in all ways if used deterministically"?

[cuviper]

Both styles of removal are deterministic -- swap_remove is exactly like Vec::swap_remove where the last item takes the position of the removed item, and shift_remove is like Vec::remove where all following items are shifted left one position. The performance tradeoffs are also the same.

I feel like it's a footgun that remove defaults to swap_remove behavior, which is why I wanted it deprecated. If you're using indexes or depending on the exact order, it's important to know what removal will do to this. But I guess if you only care about the order for build determinism / reproducibility, it doesn't really matter.

[Mark-Simulacrum]

Okay, yeah that's what I expected.

I do think it is a plausible foot gun, yes -- I don't think de-deprecating will help rustc much here fwiw, so if that's the sole reason, don't do it.

[nikomatsakis]

Hmm, speaking from the POV of "average index map user" (and not about rustc specifically), I would be fine with a clear warning in the docs on remove with respect to how it affects the order (but not an actual deprecation). My expectation coming to the crate was "as long as you only insert, insertion order; once you start removing, weird stuff happens and you have to be careful" (which seems to be accurate). I sort of consider this to be true for all indexed data structures, actually.

[cuviper]

I would be fine with a clear warning in the docs on remove with respect to how it affects the order (but not an actual deprecation).

In case this isn't clear, the deprecation doesn't discourage remove entirely -- it just recommends the methods with explicit removal style: "Deprecated: use swap_remove or shift_remove."

[cuviper]

If we do keep this after all, it should also recommend "or shift_remove".

[bluss]

Thanks everyone for the feedback!

The deprecation warnings get in they way for being a drop-in replacement for HashMap, which is still a goal.

Deprecation warnings are not signaling the right thing - we want to say 'caution', but not that these methods are going away.

I have removed the deprecations and improved the method docs to make it clear, the way it should have been from the start.