find: return named tuples instead of anonymous

[wilzbach]

Motivation: Named tuples are a lot better to read.

There has been a precedent for this, see #5436

[dlang-bot]

Thanks for your pull request, @wilzbach!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

[quickfur]

I like this. A lot. Anonymous tuples are hard to remember (is the haystack t[0] or t[1]?) and make user code more obtuse.

However, the named fields really really need to be described thoroughly in the ddoc header of each respective function. It's not good enough to just stick that into the return type.

(On that note, I'm a bit sad that the return type spec is getting long and scary to read... but that's an issue for another day, and out of scope for this PR. :-P)

[MetaLang]

I'm not sure about this. On one hand, it's technically a breaking change because named Tuples don't implicitly convert to un-named Tuples:

void doSomethingWithFindResult(Tuple!(int[], size_t) t);
doSomethingWithFindResult([ 1, 4, 2, 3 ].find(2, 4)); //Error: cannot convert type Tuple!(int[], "haystack", size_t, "range") to Tuple!(int[], size_t)

On the other hand, I doubt this type of code is very common.

[JackStouffer]

This is also more dubious that the linked PR because in that case the return type was auto, which means you shouldn't expect the return type to be name-able in your code. However, the return type is explicitly documented here, and is a more serious breaking change.

Personally, I think the breakage will be at a minimum, but it will still exist.

Paging @andralex for his opinion.

[wilzbach]

First things first: I split this PR of into three parts:

• find (this PR)
• findSplit (findSplit: return named tuples instead of an anonymous tuple #5968)
• the typo + removed std.stdio logging ([Trivial] Small std.algorithm fixes: remove stdio + typo #5969)

On one hand, it's technically a breaking change because named Tuples don't implicitly convert to un-named Tuples:

Yep, but note that this is small subset - even comparison works:

assert(find(a, [ 1, 3 ], 4) == tuple([ 4, 2, 3 ], 2));

However, the return type is explicitly documented here, and is a more serious breaking change.

For findSplit (#5968) we already used auto

However, the named fields really really need to be described thoroughly in the ddoc header of each respective function. It's not good enough to just stick that into the return type.

Done. Especially I like the improvement of the documentation for findSplit.

Personally, I think the breakage will be at a minimum, but it will still exist.
...
I like this. A lot. Anonymous tuples are hard to remember

Can't say more than this. I think in this case the gain outweighs the minimal cases where we might break code.

[quickfur]

I'm tempted to say that find should return auto. This PR is a living proof of why explicit return types is a bad idea in generic library code. Had we written find to return auto in the first place, we wouldn't have risked this kind of breakage in user code. Now our hands are tied.

[quickfur]

How about moving the changes of $(D...) to backticks to a different PR that we can merge immediately?

[wilzbach]

Let's better follow-up on the various requests to use backticks in the docs and do it for std.algorithm once and for all?
-> #5970

[n8sh]

On one hand, it's technically a breaking change because named Tuples don't implicitly convert to un-named Tuples

Don't they? This runs:
https://run.dlang.io/is/qTspmv

void main(string[] args)
{
    import std.typecons : Tuple;
    alias Point = Tuple!(int, int);
    alias PointNamed = Tuple!(int, "x", int, "y");

    import std.traits : isImplicitlyConvertible;
    static assert(isImplicitlyConvertible!(PointNamed, Point));
    static assert(!isImplicitlyConvertible!(Point, PointNamed));

    import std.stdio : writeln;
    writeln(PointNamed.stringof, " is implicitly convertible to ", Point.stringof,
            " but not vice versa.");
}

[MetaLang]

Hmm... you're right, but now I don't understand why. Maybe it's because they're structs that have the same layout, so the compiler allows them to implicitly convert to each other...?

import std.typecons;

Tuple!(int, "x", int, "y") test(int x, int y) { return tuple!("x", "y")(x, y); }
void takesTup(Tuple!(int, int)) {}

void main()
{
    auto t = test(1, 1);
    takesTup(t);
    takesTup(test(2, 3));
    takesTup(tuple!("x", "y")(2, 2));
}

[quickfur]

ping @andralex This is a potentially breaking change, need your review & approval.

[andralex]

I wish we just closed this. It created just work for everyone involved, and is liable to create more.

[quickfur]

Should we just close this then?

[wilzbach]

I wish we just closed this. It created just work for everyone involved, and is liable to create more.

Would you mind elaborating on this a bit?
find(a, 2, 4).needle is a lot easier to read than find(a, 2, 4)[1] and the breakage is more hypothetical / minimal.
When we did our last "Learn D session" at the Munich Meetup group, usage of anonymous tuples in the Phobos APIs was among the common complaints (hence this PR btw).

[andralex]

I don't deny it's nicer. But there's work in reviewing, work in fixing known breakages of existing code, and more future work on fixing other breakages. It doesn't seem on the right side of cost/reward.

[andralex]

Gave this a fresh look, I can't shed the feeling "why are we bothering with low impact matter when there's so many big rocks to move". Anyhow, what are the cases that break - aside from those that explicitly check for exact typeof. The answer to this will inform this PR and others like it,

[MetaLang]

what are the cases that break

@andralex I'm surprised at what the compiler allows; these are the cases I could think of OTOH:

import std.typecons;

Tuple!(int, "x", int, "y") test(int x, int y) { return tuple!("x", "y")(x, y); }
void takesTup(Tuple!(int, int)) {}
void takesTup2(Tuple!(int, "asdf", int, "fdsa"));

void main()
{
    auto t = test(1, 1);
    takesTup(t); //ok
    takesTup(test(2, 3)); //ok
    takesTup(tuple!("x", "y")(2, 2)); //ok
    
    Tuple!(int, int) t2 = t; //ok
    Tuple!(int, "asdf", int, "fdsa") t3 = t; //ok
    Tuple!(int, "asdf", int, "fdsa") t4 = Tuple!(int, "x", int, "y")(1, 1); //ok
    
    Tuple!(int, int) t5 = Tuple!(int, "x", int, "y")(1, 1); //ok
    takesTup2(t);  //error
    takesTup2(t2); //error
}

[andralex]

@MetaLang https://github.com/dlang/phobos/blob/master/std/typecons.d#L651 has a lot to do with this. Named tuples are subtypes of their unnamed counterparts.

With this realization I think this PR is a go. Any breakage anyone sees?

[andralex]

I'm for this. If it causes major issues we didn't get all aspects of alias this right.

[jmdavis]

As discussed in this PR, changing findSplit to return a named tuple broke code, because t[0] and t.name weren't actually equivalent like they should be. So, until we figure out why and fix Tuple, I don't see how we can reasonably change any anonymous tuples to named tuples.

Such a change should work as long as you ignore the issue of anyone who explicitly declared a Tuple based on the docs instead of using typeof having problems (and we can probably safely ignore that), but apparently, it does currently break some code that simply was indexing the tuple.

[andralex]

@jmdavis can you please explain why t[0] is not equivalent to t.name? That would indeed be a problem. The visible comments in #6078 discuss the aftermath of the matter, I can't seem to see it stated.

[jmdavis]

Well, that discussion linked to one of the changes that had to be made to make Phobos compile when findSplit was made to return named tuples, which was this:

8becc70

Based on the commit message, it seems to be a problem related to CTFE and casting. I did do some looking over Tuple to try and understand it when that PR was posted, but I didn't spend enough time at it to fully dissect what was going on or figure out how to fix it.

Given that t[0] works correctly when the tuple is not named, and t.name works when the tuple is named, I would assume that the problem is fixable, but Tuple's implementation is quite complicated.

[andralex]

Got it, thanks. That's actually an easy fix - we need to use a non-names Tuple as backend for the respective named Tuple. Then no need to cast in CTFE. I've submitted https://issues.dlang.org/show_bug.cgi?id=18426

[wilzbach]

Based on the commit message, it seems to be a problem related to CTFE and casting. I did do some looking over Tuple to try and understand it when that PR was posted,

I dived a bit deeper and reduced the problem to this example:

https://run.dlang.io/gist/9e497b0205031ee8370aa9fa1ea18399?compiler=dmd&args=-unittest%20-main%20-c

However, I'm not sure how we can workaround this. If just the alias expand this is used, this stops to work:

auto t1 = Tuple!(int, "x", string, "y")(1, "a");
void foo(Tuple!(int, string) t2) {}
foo(t1);

foo.d(149): Error: function foo.__unittest_L143_C7.foo(Tuple!(int, string) t2) is not callable using argument types (Tuple!(int, "x", string, "y"))
foo.d(149):        cannot pass argument t1 of type Tuple!(int, "x", string, "y") to parameter Tuple!(int, string) t2

So @property ref inout(Tuple!Types) _Tuple_super() inout @trusted is the magic enemy here :/

[andralex]

So @Property ref inout(Tuple!Types) _Tuple_super() inout @trusted is the magic enemy he

It's not the signature, it's the implementation. All we need is to get rid of that cast, and we do so by reusing the unnamed Tuple as layout for the named Tuple.

[wilzbach]

we do so by reusing the unnamed Tuple as layout for the named Tuple.

That's already the case today - even for named tuples the layout is as follows:

struct Tuple
{
    Types expand; // AliasSeq!(int, int)
}

[wilzbach]

we need to do this:
Tuple!(int, "x", int, "y") should have as its only state Tuple!(int, int). In the form of a data member that is. Call that field impl. Then:
alias impl this;

This runs into the same mentioned overload problems as just Types expand

https://run.dlang.io/gist/9041fb8eac5134ffd47f0acbb2f4fb26?compiler=dmd&args=-unittest%20-main%20-c

(updated link)

[andralex]

@wilzbach link doesn't work but at any rate we can make it work.

[LightBender]

PR closed as stalled. If you wish to resurrect it you are welcome to do so. @tgehr This may be of interest to you.