Reintroduce const violation bug to fix vibe.d build failure

[TurkeyMan]

This should fix the vibe.d build failure.

[dlang-bot]

Thanks for your pull request and interest in making D better, @TurkeyMan! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

• My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
• My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
• I have provided a detailed rationale explaining my changes
• New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

---

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

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.

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 + druntime#2753"

[CyberShadow]

This has fewer failures on BuildKite than master, so, I'm going to merge this.

[TurkeyMan]

The vibe.d issue that this was intended to resolve is resolved by this PR. That other one is unrelated to anything I've done.

[n8sh]

Noting that the title of this PR is incorrect. It is not a bug to return int rather than const int and is to be preferred dlang/phobos#6682 (comment):

That ensures no surprise with functions that initialize an auto with the result of the call.

[CyberShadow]

Thanks for pointing that out! So, this is actually a regression fix affecting value types.

[TurkeyMan]

@n8sh Supplying a const ref to a function, and returning it with const stripped away is a bug, and it's a bad one. Andrei's example shows a bool, and you talk about int. The API in question is a T, which is a very different thing. T may have indirections.

This code is NOT a fix, it reintroduced the bug.

[CyberShadow]

Supplying a const ref to a function, and returning it with const stripped away is a bug

If it returns by non-ref, then it should be perfectly fine to strip head const.

T may have indirections.

So, don't return T, return T with head const stripped. That should both fix the bug and prevent a regression for pure value types.

[TurkeyMan]

If it returns by non-ref, then it should be perfectly fine to strip head const.

Stripping head-const is not valid for struct's or classes.
This code strips const completely.

It was showed elsewhere numerous times:

struct S { int* p; }
immutable S s;
S r = atomicLoad(s);
*r.p = 10;

[CyberShadow]

Stripping head-const is not valid for struct's or classes.

Yes, it is impossible to have tail-const in these cases. So, for structs or classes, it should return T as-is.

[TurkeyMan]

That's possible... but my general feeling is that so many 'cases' are bad. It makes meta way more complicated when it decides to do a special thing in one case and not some other...
Someone will write their own meta that assumes atomicLoad strips const from the return, and then we just cascade the issue into their meta when they encounter T == struct, and they are forced to propagate the same logic.
I personally think that's a really bad habit that is prolific in D code, and we should avoid undue complexity in the std library, and certainly druntime.

[CyberShadow]

That's possible... but my general feeling is that so many 'cases' are bad.

That really isn't anything new, it's a consequence of D's type system being generally very complicated. Have a look at the implementations of some templates in std.traits and std.typecons. This is why we encapsulate the complexity into a template which does a single thing and is documented to do this certain thing, then use it throughout. (Actually a bit surprised there isn't already something like that in Phobos, but maybe I missed it.)

Someone will write their own meta that assumes atomicLoad strips const from the return

I don't really see why this is an issue. It will result in a compilation error, and they will have to fix the code if they want it to support more kinds of types.

The bottom line is, the code in vibe.d was correct, so we need to avoid breaking it and other such code, even if that means adding a little bit of complexity here.

[CyberShadow]

BTW, perhaps leveraging the implementation in the compiler will make this simpler. Does Unqual!T v = *(T*).init; compile? Then, Unqual!T is a safely head-const-stripped version of T. Then there is no need to check for every kind of D type.

Edit: demo: https://run.dlang.io/is/34UqHd

[TurkeyMan]

I can make a rule like that. It still feels pretty complex though.
Here's a thought; the argument is that it interacts poorly with auto, but I wonder why auto works the way it does?

const(int) foo();
auto x = foo();
pragma(msg, typeof(x));

> const(int)

This seems unnecessary... why doesn't auto strip away pointless qualifiers naturally?
I feel like that would have a simplifying effect on a lot of code. It may also reduce instantiation bloat; since functions that receive auto args would coalesce various qualified permutations into fewer instantiations.