The implementation of each in std.algorithm.iteration sets out a lofty goal: whatever can be iterated, must be iterated with each.

Problem is, there are way too many things that can be iterated in D and too many ways to iterate them. This leads to a veritable gallop of checks:

    alias BinaryArgs = AliasSeq!(fun, "i", "a");

    enum isRangeUnaryIterable(R) =
        is(typeof(unaryFun!fun(R.init.front)));

    enum isRangeBinaryIterable(R) =
        is(typeof(binaryFun!BinaryArgs(0, R.init.front)));

    enum isRangeIterable(R) =
        isInputRange!R &&
        (isRangeUnaryIterable!R || isRangeBinaryIterable!R);

    enum isForeachUnaryIterable(R) =
        is(typeof((R r) {
            foreach (ref a; r)
                cast(void) unaryFun!fun(a);
        }));

    enum isForeachUnaryWithIndexIterable(R) =
        is(typeof((R r) {
            foreach (i, ref a; r)
                cast(void) binaryFun!BinaryArgs(i, a);
        }));

    enum isForeachBinaryIterable(R) =
        is(typeof((R r) {
            foreach (ref a, ref b; r)
                cast(void) binaryFun!fun(a, b);
        }));

    enum isForeachIterable(R) =
        (!isForwardRange!R || isDynamicArray!R) &&
        (isForeachUnaryIterable!R || isForeachBinaryIterable!R ||
         isForeachUnaryWithIndexIterable!R);

Does this pass the laugh test? How can one explain a colleague "yep, here are the conditions under which you can iterate over an object in the D language".

I don't contest the correctness of this code. I contest its being sensible. There's just too much of a good thing.

For example: static arrays are supported. Why? This creates a dangerous precedent whereby we'd need to support static arrays with other primitives, such as map or reduce. (We don't, and we shouldn't. Let them add a "[]" to the array.) Where do you stop?

We support opApply with one, two, or more arguments. Built-in hashtables (when was the last time you were in a place in your life where hashtable.each was useful?). You name it - we support it.

This is in the same league with supporting enums that use string as a base, as "some kinda sorta string thing".

Too much!

I have no idea how to improve this code because it will break one of the suitably overspecialized unittests. But this kind of stuff has no place in stdv2021.

On 9/17/20 12:18 PM, Andrei Alexandrescu wrote:
[snip]

Can't believe I forgot to mention what would be the ultimate smoking gun: all of that crap spills into the user-visible documentation: each is constrained on symbols not documented and not available to the user!

https://dlang.org/library/std/algorithm/iteration/each.each.html

Sure, one may have a vague idea what isForeachIterable or isRangeIterable are, but these are neither documented, nor accessible to user code. The user's only chance is peruse the source code or "build and pray".

We really must draw the line at defining constraints in terms of private symbols. That's a no-no.

On Thursday, 17 September 2020 at 16:18:18 UTC, Andrei Alexandrescu wrote:
>
> We support opApply with one, two, or more arguments. Built-in hashtables (when was the last time you were in a place in your life where hashtable.each was useful?). You name it - we support it.

It's a standard library - catering for almost everything is in the job description ;-)

> This is in the same league with supporting enums that use string as a base, as "some kinda sorta string thing".
>
> Too much!

I don't think anyone argues with you here.

> I have no idea how to improve this code because it will break one of the suitably overspecialized unittests.

Well, more importantly there's a high chance it will break actual code.

> But this kind of stuff has no place in stdv2021.

The only way for this to ever happen is if we start with it. One function at a time.
I suggest simply creating a new repository and starting from zero.
Apart from the clear mental divide and separation, this has the added advantage that we can just keep druntime and phobos as is and work on a new combined "base" library. There are a few others like being able to immediately ship it via dub or being able to use GitHub's features (issues, projects, roadmaps, ...).

Also, as we're almost done with migrating off auto-tester, setting up the same CIs wouldn't be a big hassle and I would be happy to do so ;-)

On Thursday, 17 September 2020 at 16:18:18 UTC, Andrei Alexandrescu wrote:
> For example: static arrays are supported. Why? This creates a dangerous precedent whereby we'd need to support static arrays with other primitives, such as map or reduce. (We don't, and we shouldn't. Let them add a "[]" to the array.) Where do you stop?

It should support foreach(), which would support static arrays. The problem is there is so many ways to do something, and part of that is on Phobos the way ranges were defined and are used. They aren't very efficient, they create excessive copies, and if you have a type that can't be copied then you pretty much can't use it at all.

You have situations like writeln() that uses ranges and doesn't have a path for types that support foreach(), this ends up with bugs where writeln ends up modifying the const object it was passed. Because it has to cast away the constness to be able to iterate through a range.

If you only support phobos ranges your are going to be left with a gaping hole that's currently filled with a language feature (that should be supported).

`reduce` does support static arrays btw.

On 9/17/20 1:41 PM, Seb wrote:
> On Thursday, 17 September 2020 at 16:18:18 UTC, Andrei Alexandrescu wrote:
>>
>> We support opApply with one, two, or more arguments. Built-in hashtables (when was the last time you were in a place in your life where hashtable.each was useful?). You name it - we support it.
> 
> It's a standard library - catering for almost everything is in the job description ;-)

You know it doesn't quite work like that. By that argument most range algorithms should work on static arrays, and actually beginners ask for such on occasion. We have deemed that too much effort for no good outcome. In that context, each is both an outlier and a dangerous precedent.

I recall you and I discussed each in the past. Probably we agreed it should work with everything one could reasonably use a loop with. Maybe you worked on it, too. I didn't check because it's not important; we should dissociate the code from whoever created it, and indeed I found myself critical of code I later discovered I'd written myself. This particular code is finely done, it's the charter we need to revise.

The fact that constraints come in form not knowable or verifiable by the user is a deal-breaker as bad as accepts-invalid. That transcends backward compatibility.

(Reminds me with a discussion I had with a salesman: "You should really subscribe to X." "What is X?" <vague description> "I don't understand." "Well it's really good, you should really try it out regardless so you get an idea." "How can you advise me to subscribe to something I don't understand?")

I created https://issues.dlang.org/show_bug.cgi?id=21260.

On Thu, Sep 17, 2020 at 05:41:56PM +0000, Seb via Digitalmars-d wrote:
> On Thursday, 17 September 2020 at 16:18:18 UTC, Andrei Alexandrescu wrote:
[...]
> > But this kind of stuff has no place in stdv2021.
> 
> The only way for this to ever happen is if we start with it. One function at a time.

Indeed.  We've been talking about std.v${next} for what, almost (or already?) a year now?  Time to take action.


> I suggest simply creating a new repository and starting from zero.

Yes, yes, yes!


> Apart from the clear mental divide and separation, this has the added advantage that we can just keep druntime and phobos as is and work on a new combined "base" library. There are a few others like being able to immediately ship it via dub or being able to use GitHub's features (issues, projects, roadmaps, ...).
[...]

This is a good way to start from a clean slate.  Say we set it up such that you could use a compiler switch to switch between current-Phobos and new-Phobos.  Maybe with a path override or some such.  Make it easy to test the new library with existing code, but without the forced commitment and breakage of actually replacing current-Phobos.  This would allow us to test the state of the new library against existing code and gauge how well we're doing / determine what kinds of breakages might occur and what migration path(s) are available for people who want to upgrade their code.

Also, we should use a disjoint namespace so that when new-Phobos does become official, old code can still continue working as-is (maybe add a path or two so that the compiler can find legacy-Phobos).  I propose std.v2.* -- with the anticipation that, a decade or so from now, there will be a std.v3.* which can be added without breaking all prior code. As Andrei has said -- stop changing, start adding.


T

-- 
We are in class, we are supposed to be learning, we have a teacher... Is it too much that I expect him to teach me??? -- RL

On 9/17/20 1:56 PM, Avrina wrote:
> `reduce` does support static arrays btw.

Somebody shoot me.

On Thu, Sep 17, 2020 at 02:41:09PM -0400, Andrei Alexandrescu via Digitalmars-d wrote:
> On 9/17/20 1:56 PM, Avrina wrote:
> > `reduce` does support static arrays btw.
> 
> Somebody shoot me.

BANG.

:-P

IMO, we should deprecate static array support on .reduce. (Not that it really matters anymore to me; .reduce has the wrong parameter order from the days before UFCS, and has been supplanted by .fold.  I sure hope .fold isn't stupid enough to accept static arrays...)


T

-- 
Маленькие детки - маленькие бедки.

On Thursday, September 17, 2020 10:18:18 AM MDT Andrei Alexandrescu via Digitalmars-d wrote:
> This is in the same league with supporting enums that use string as a base, as "some kinda sorta string thing".
>
> Too much!
>
> I have no idea how to improve this code because it will break one of the suitably overspecialized unittests. But this kind of stuff has no place in stdv2021.

Agreed.

I think that it's yet another case of something being added, because someone thought that it would be useful - probably several things over several iterations of the code. Getting stray stuff into Phobos like that doesn't really have a particularly high bar. Pull requests either tend to either sit around for too long and/or just get merged because they don't do anything clearly bad. Sometimes, someone will object to a particular pull request due to something like adding support for static arrays, and the changes won't get in, but I think that it's more likely that it will get in simply because the work was done, and it passes the tests. We've never really had enough people looking at pull requests, and we haven't had clear guidelines for how some stuff should be handled. So, a number of things get handled inconsistently. On some level, I think that our attempts to not PRs sitting around forever have also led to more questionable code getting committed in a number of cases. I don't know what a good way to fix that is.

Of course, another part of the calculus here is how changing how we've done stuff over time has caused some fun problems - like templatizing a function that used to explicitly take string. You then get a function that shouldn't accept static arrays or enums with a base type of string but which has to in order to avoid breaking code. isConvertibleToString is one of the big mistakes that came out of that, and we still haven't purged it from Phobos.

Regardless, IMHO, we should be avoiding support for static arrays and opApply in range-based code. At most, maybe there should be a way to convert a type with opApply to a range, but static arrays are already solved just by slicing them. In general, by requiring that someone explicitly do a conversion or wrap something in another type in order to make it a range, we can really simplify code. And in general, supporting implicit conversions in templated code just adds complexity and landmines (e.g. you can easily get bugs by testing that a type is implicitly convertible in the constraint but then not explicitly converting the type within the function). IMHO, for the most part, the less we support implicit conversions the better. I honestly wish that static arrays didn't even implicitly convert to dynamic arrays.

If we're really going to do another version of Phobos and rework ranges, then I think that as part of that, we should make various rules about how Phobos will handle ranges (and how most code should handle ranges) clear. Even when there's agreement on some of that stuff right now among most of the key developers, it's not necessarily well-documented. We should probably have explicitly documented rules and guidelines about stuff like not supporting static ranges in range-based code. That would then at least allow us to avoid some of the problems like shown here with each - as well as better document how ranges work and are supposed to be used in general.

- Jonathan M Davis

On Thu, Sep 17, 2020 at 01:41:00PM -0600, Jonathan M Davis via Digitalmars-d wrote: [...]
> Regardless, IMHO, we should be avoiding support for static arrays and opApply in range-based code. At most, maybe there should be a way to convert a type with opApply to a range, but static arrays are already solved just by slicing them. In general, by requiring that someone explicitly do a conversion or wrap something in another type in order to make it a range, we can really simplify code.

Yes, yes, and yes!  Separate out the concerns by letting another function take care of adapting {static array, opApply iterable, what have you} to the range API, then delete the non-range code that the implementation currently has.  Result: cleaner code, more modular, and each piece is more reusable (the adaptor can be used for other things besides .each).


[...]
> I honestly wish that static arrays didn't even implicitly convert to dynamic arrays.

That bug has been filed for years, I wish somebody would take it up and push it in. :-/  That has been a bad idea for years and yet it still persists.

	https://issues.dlang.org/show_bug.cgi?id=15932


> If we're really going to do another version of Phobos and rework ranges, then I think that as part of that, we should make various rules about how Phobos will handle ranges (and how most code should handle ranges) clear.  Even when there's agreement on some of that stuff right now among most of the key developers, it's not necessarily well-documented. We should probably have explicitly documented rules and guidelines about stuff like not supporting static ranges in range-based code. That would then at least allow us to avoid some of the problems like shown here with each - as well as better document how ranges work and are supposed to be used in general.
[...]

Yes, and we need water-tight wording in the documentation to leave no ambiguities in things like how things get copied around (or not), what happens after passing a to a function and the function returns, transient .front, ref, const, etc..  All seemingly trivial things, but left unadddressed, they can really become thorns in our sides.


T

-- 
An elephant: A mouse built to government specifications. -- Robert Heinlein