Revert "findSplit: return named tuples instead of an anonymous tuple"

[jacob-carlborg]

Reverts #5968. As mentioned in #5968 (comment), this broke DStep and Phobos. Meaning it might break other projects as well.

[dlang-bot]

Thanks for your pull request, @jacob-carlborg! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.

Some tips to help speed things up:

• smaller, focused PRs are easier to review than big ones

• try not to mix up refactoring or style changes with bug fixes or feature enhancements

• provide helpful commit messages explaining the rationale behind each change

Bear in mind that large or tricky changes may require multiple rounds of review and revision.

Please see CONTRIBUTING.md for more information.

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.

[jmdavis]

I suspect that this comes from how Tuple implements indexing. At first glance, the error that gets fixed in the commit that you linked to in the other PR (8becc70) seems like it shouldn't be necessary, but looking at the implementation of Tuple, it's not entirely clear to me how indexing works at all. It is complicated. But definitely, if it's not guaranteed that t[0] and t.name are going to work exactly the same, then changing to named tuples should not be happening.

It's already questionable to make the change given that any code that uses the type explicitly will break (or which uses tuple to make an assignment to it) - particularly when the documentation states what the type being returned is - but t[0] and t.name must have identical semantics, or we can't be switching to named tuples. Maybe with some adjustment to Tuple's implementation, it would work, but as it stands, that commit is a huge red flag that we can't be making changes like this as things stand.

[jacob-carlborg]

According to the examples of in the documentation of Tuple, it looks like indexing and using a named access is interchangeable for named tuples.

[andralex]

I'd like to build consensus on this with @wilzbach. First, let's get a better understanding of the breakage and if it can be addressed or at least improved upon with library code - e.g. by defining opAssign to accept structurally equivalent tuples. Can someone enumerate a few breakages in current code?

[jacob-carlborg]

I'd like to build consensus on this with @wilzbach. First, let's get a better understanding of the breakage and if it can be addressed or at least improved upon with library code - e.g. by defining opAssign to accept structurally equivalent tuples.

May I suggest we revert this first and then figure out if it's possible to make this change without breaking existing code.

Can someone enumerate a few breakages in current code?

It broke DStep [1] and Phobos [2]. It was fixed in Phobos by the same PR that introduced the change.

[1] https://travis-ci.org/jacob-carlborg/dstep/jobs/333759750
[2] 8becc70

[wilzbach]

Ugh. I didn't expect that to happen. Yeah reverting and regrouping is the best course of action - even though this shouldn't have happened :/
@jacob-carlborg could you please add dstep to Jenkins? Thanks!

[andralex]

Thx @wilzbach. Regarding the dstep error:

std/typecons.d(648,47): Error: reinterpreting cast from `inout(string)*` to `inout(Tuple!(string, string))*` is not supported in CTFE

That sounds entirely addressable by us.

[andralex]

And the other seems just the same.

[jmdavis]

We should be able to make it so that t[0] has identical semantics regardless of whether it's a named tuple or not, and right now, that does not seem to be the case. So, that should be totally fixable, but I think that it definitely should be fixed before we go about changing unnamed tuples to named tuples.

What we can't fix and makes changing to named tuples somewhat debatable depending on the context is that unnamed tuples are not compatible with named tuples type-wise. So, any time that someone has explicitly used Tuple or tuple in their code, and we go and change the type that we're providing to a named tuple, that code will break.

In the case of a function like findSplit, we have explicitly stated that it returns a tuple, so someone could easily be using Tuple or tuple explicitly in their code. e.g.

Tuple!(string, string, string) result;
if(condition)
    result = range.findSplit(needle);

// do something with result

Outside of strings, it's not particularly likely that someone would use Tuple explicitly like that, since you have to list the range types explicitly, and that gets ugly outside of strings, but they might. Regardless, it can be argued that risking such breakage is worth it, since in the vast majority of cases, folks are just going to use auto, and the use of named tuples is arguably better. But whether the change is worth risking the breakage is certainly a point that's up for debate.

Either way, I think that it's clear that we need to make sure that t[0] has the same semantics with and without named tuples if we want to switch to named tuples, since if that breaks, then existing code will break. Once we're sure that the semantics are the same, then we can look at changes like findSplit on a case-by-case basis.

[JackStouffer]

Regardless, it can be argued that risking such breakage is worth it, since in the vast majority of cases, folks are just going to use auto, and the use of named tuples is arguably better. But whether the change is worth risking the breakage is certainly a point that's up for debate.

Yes, I agree that the chances of someone using an explicit type for the return value is low, so the breakage once the semantics are fixed should be to a very small area.

However, it can also be argued that using anything other than typeof with an auto returning function is just asking for trouble and should have been avoided in the first place.

[jacob-carlborg]

jacob-carlborg could you please add dstep to Jenkins? Thanks!

@wilzbach done: dlang/ci#145.