Fix issue 17803 std.typecons.Tuple: opAssign should return ref Tuple

[Biotronic]

Issue 8494 is a two-part issue, with the other part requiring changes in DMD. This takes care of the Phobos side by having Tuple.opAssign return ref Tuple.

This allows Tuple to act like built-in types when assigning to it, e.g. return myTuple = myOtherTuple;.

[dlang-bot]

Thanks for your pull request, @Biotronic! 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

Auto-close	Bugzilla	Description
✓	17803	std.typecons.Tuple: opAssign should return ref Tuple

[MetaLang]

This should be written as assert(__traits(compiles, { a = (a = a); })); Generally, the unit tests for phobos are checkable predicates.

Also, there should be a test case for aliasing, e.g.:

Tuple!(int[]) a, b, c;
a = tuple([0, 1, 2]);
c = b = a;
assert(a.length == b.length && b.length == c.length);
assert(a.ptr == b.ptr && b.ptr == c.ptr);

[PetarKirov]

This should be written as assert(__traits(compiles, { a = (a = a); }));

No, your later example is much better. __traits(compiles, ...) is an anti-pattern in unittest, because it doesn't check that the result is actually correct. There can also be subtle differences between CT and RT execution, which can also skew the result of this check. There was an effort a couple of months ago to remove many instances of __traits(compiles, ...) from phobos.

[MetaLang]

Sorry, I went to reply and accidentally edited your comment instead.

---

No, your later example is much better. __traits(compiles, ...) is an anti-pattern in unittest...

That's exactly what was being tested though, whether a = (a = a) can compile. If that wasn't the intention then I agree that the test should be changed.

There was an effort a couple of months ago to remove many instances of __traits(compiles, ...) from phobos.

Depending on what it was replaced with, the cure could be worse than the disease. is(typeof(...)) is the other construct that was heavily used for testing if something compiles, historically, but it is far worse than using __traits(compiles) because it returns false for a myriad of other cases than just if an expression doesn't compile.

[Biotronic]

Question: dlang-bot says this will auto-close issue 8494, which it shouldn't - this is just a partial fix, and the rest requires changes in DMD (either multiple alias this, in which case Phobos requires more changes, or making AliasSeq!17 implicitly castable to int). Should I or someone else do something to make sure the bug report is not closed?

[PetarKirov]

@Biotronic probably it would be better to split the issue in two so your PR can close the first one, and we can leave the dmd enhancement request open.

[RazvanN7]

@Biotronic You have 2 possibilities :
-> you can squash your commits under a commit message which does not include "Fix Issue 8494" (the dlang bot links the PR with the issues mentioned "Fix Issue X" in the commit message), that way the PR won't be linked with the bug report and the bug report will not be closed.
-> this PR will close the bug report and then you can add another bug report which states what should be done next in DMD (and phobos, if necessary).

In my opinion, the latter is the way to go

[Biotronic]

Added a new bug report for the issue solved here, to retain the discussion in the original bug.

Comments addressed

[MetaLang]

As far as I can tell the circleci failure is a false positive.

Also @Biotronic, can you please rebase this branch on top of stable? All bufixes must target the stable branch.

[PetarKirov]

No need to target stable as this an enhancement not a bug fix (no matter how it is logged in Bugzilla). Also it would be better to stick with master since changing the return type of public API can break code (especially heavily metaprogramming) in unforseen ways so it we should opt in to the lengthier release cycle of official in order to get more testing.

[Biotronic]

I'm confused by the style_lint line numbers - they don't seem to match the actual code. In this case the line dscanner complains about is far removed from anything I've touched, but in other cases it's been off by 10-20 lines. And seeing as it doesn't print the line that failed it's sometimes hard to figure out what the actual problem is.

It seems to be a false positive in this case, and I can't reproduce it locally. It should still be fixed, but having dscanner give better output is a better fix than randomly changing lines in this PR to appease it.

Also, forgot to update issue number in source. Fixed now.

[PetarKirov]

@Biotronic We're all good now, thanks for your work!

[wilzbach]

I'm confused by the style_lint line numbers - they don't seem to match the actual code. In this case the line dscanner complains about is far removed from anything I've touched, but in other cases it's been off by 10-20 lines. And seeing as it doesn't print the line that failed it's sometimes hard to figure out what the actual problem is.

This is due to the fact that we merge your PR into master on CircleCi. Unfortunately the only ways to get correct line numbers are to rebase a PR or to run DScanner locally.

[PetarKirov]

@wilzbach the obvious question that applies to all CIs is "Can we rebase all PRs before testing them?".

[wilzbach]

Do you mean sth like auto-rebasing them (dlang/dlang-bot#109) or simply which CIs rebase and which don't? IIRC Travis is the only one left which doesn't.

[PetarKirov]

I meant rebasing on the CIs end, but actually dlang/dlang-bot#109 looks much more appealing. I'm a bit busy atm to do a proper review. Ideally @CyberShadow and @MartinNowak should also take a close look, since this is quite important for our contribution process. Edit' I just saw that Vladimir already did a review, so sorry for the needless ping.