Fix Issue 12086 - std.algorithm.remove + range of indices produces wrong results

[wilzbach]

See the Bugzilla message or the changelog entry for a motivation.

---

Previously, std.algorithm.remove used to accept any values as offset.
This behavior was very error-prone:

import std.algorithm, std.stdio;
[0, 1, 2, 3, 4].remove(1, 3).writeln; // 0, 2, 4  -- correct
[0, 1, 2, 3, 4].remove([1, 3]).writeln; // 0, 3, 4  -- incorrect

With this release, using arrays as individual element is deprecated.
tuple can be used to explicitly indicate that a range
from start to stop (non-enclosing) should be removed:

import std.algorithm, std.stdio, std.typecons;
[0, 1, 2, 3, 4].remove(tuple(1, 3)).writeln; // 0, 3, 4

However, only 2-tuples are allowed to avoid this un-intuitive scenario:

import std.algorithm, std.stdio, std.typecons;
[0, 1, 2, 3, 4].remove(tuple(1, 3, 4)).writeln; // 0, 3, 4

[dlang-bot]

Thanks for your pull request, @wilzbach!

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✓	12086	normal	std.algorithm.remove + range of indices produces wrong results

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + phobos#6154"

[wilzbach]

There's also a similar discussion about renaming remove here: https://issues.dlang.org/show_bug.cgi?id=10959

Quotes:

Currently std.algorithm.remove is a landmine
..
It's a landmine, and it's bit me a few times in the past too.

Note that this PR just deprecates the "landmine" input. Ideally renaming to removeIndex would be best, but that's not something this PR proposes. Let's fix the landmines first!

[dukc]

On the bottom of this function you use single-statement curly brace block for static else. But at line 1713 you newline after it without curly braces, a style considered error-prone by some. Should you change these to be consistent with each other? On the other hand, this might be an over-pedantic suggestion.

[wilzbach]

The current convention is that curly braces only need to be used for more than one line, i.e. when they are required.
I don't mind changing but to my knowledge always requiring curly braces isn't part of the DStyle (as of now).

[jmdavis]

And please don't make it part of the style. That would be incredibly annoying. Personally, I always use them when the condition is longer than one line, since it gets hard to read otherwise, but other than that, I never use braces when their contents are just one line long, and many other contributors to Phobos don't use the pointless braces either. They just add unnecessary vertical space to the file.

[dukc]

I did not mean that lines like that should have the curly braces always, but that they should (maybe) be consistent with each other in the same function. Meaning, either use or don't use curly braces for if or else clause at one line and one statement at another line, but don't do both in the same "mother block".

[wilzbach]

Looks like no one wants to approve a deprecation without @andralex's approval :/

[andralex]

The documentation at https://dlang.org/library/std/algorithm/mutation/remove.html does not even list the use with arrays. I think we should simply change the code, not deprecate. We shouldn't commit to deprecate undocumented behavior.

Anyway I'm requesting changes on behalf of the code comments.

[andralex]

s/element/elements/

[andralex]

That's a lot of work! Just do this inline:

    static if (is(T == Tuple!(U1, U2), U1, U2))
        enum isIntegralPair = isIntegral!U1 && isIntegral!U2;
    else
        enum isIntegralPair = false;
}

We don't want to support static arrays.

No need even to define a separate template, just do this in the body:

static assert(is(T == Tuple!(U1, U2), U1, U2) && isIntegral!(T[0]) && isIntegral!(T[1]), "Each offset must be an integral or a tuple of two integrals");

[andralex]

No private symbols in a public constraint - how would the user ever figure what isIntegralPair does?

[wilzbach]

I think we should simply change the code, not deprecate. We shouldn't commit to deprecate undocumented behavior.

Cool. I wasn't aware that this is an option - even better then :)
I also improved the documentation and examples, s.t. the usage of tuple offsets gets clearer.

[MartinNowak]

We shouldn't commit to deprecate undocumented behavior.
Cool. I wasn't aware that this is an option - even better then :)

An idiosyncratic option, in particular given the bad state of documentation a few years ago.
Can we please commit to raise quality instead of constantly deteriorating sensible principles.
deprecate is a keyword in D so that the cost of deprecations is very low.

Complex rules are also unscalable, as we either need an authority to decide on every deprecation or risk a "let's break everything, I'm certain nobody uses it" atavism.

In particular considering that half of our CI projects are currently disabled, I don't think it's a good idea to merge this as is.

Thanks for your work though, it was a weird idea to use an array to remove a range of indices, and it's in our interest to get rid of this.

Not quite on topic, would be interesting if we had a dedicated range primitive for a till b (e.g. iota) that's recognized by remove, and similar use-cases like opSlice.

Added the deprecation warnings back in.

[wilzbach]

Okay I added the deprecation warnings back in & rebased this to master. Should be good to go now.
Maybe it can still be part of 2.081?