Enable -preview=rvaluerefparam

[TurkeyMan]

This has been hanging for like 6-7 years.
Walter told me to raise it on the forum and if there was no objection, he agreed it was time.
I raised it on the forum, several affirmative's, no resistance... I think that's clean signal.
This just doesn't affect very many people which is why most people don't seem to have a particularly strong opinion either way I reckon.

Anyway, it's time. I've been building with this for the better part of a decade, and several other people confirmed they've been using it the whole time too.

Spec PR dlang/dlang.org#3924

[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 run digger -- build "master + dmd#17068"

[rikkimax]

Looks like there is something nasty hiding out:

src\core\lifetime.d(1958): Error: static assert: is(void function() pure nothrow @nogc @safe == void function() nothrow @system) is false

[thewilsonator]

This should also update the tests since that is now no longer required as an argument for them

[rikkimax]

My guess is there is something wrong with attribute inference with this turned on.

https://github.com/dlang/dmd/blob/master/druntime/src/core/internal/moving.d#L29

Either way, until that's fixed this isn't being merged.

[TurkeyMan]

@thewilsonator yeah, I didn't remember how to run the tests locally; it's been a long time since I've worked on dlang!
I figured I'd just yolo and see :P

[TurkeyMan]

@rikkimax yeah that's weird huh... how do I repro that?

[rikkimax]

Split it out I suppose.

They are low level modules, might be able to build them up to not need druntime and test locally.

[TurkeyMan]

A couple of these look like they need attention from a proper DMD dev... like the one where a forward-reference error appears out of nowhere which has nothing to do with this flag, and also the return ref ones; I think that rvalue should remain NOT accepted by return ref parameters, since they only receive a temporary which expires after the call.

[TurkeyMan]

I had to fix the implementation a bit to tighten the guardrails; it was also accepted for out and return ref, in some code paths.

[TurkeyMan]

Okay, it looks there's 2 implementation bugs left in here; would any competent DMD contributor be able to check out the last 2 things?

test18216.d looks like a surprise forward-reference issue; I have no idea how this code could interact with forward referencing, but the error seems to think so!

The other odd azure one looks kinda scary... it looks like it's supposed to test opPostMove is called, but then for the inference to fail that way, it looks suggestive that opPostMove is just not being called at all... :/

core.lifetime is such a disaster zone, which will hopefully be completely ejected into space before too much longer!

[TurkeyMan]

Turned out hasElaborateMove just tested if some expressions worked, rather than checking the method details directly. I changed it to explicitly confirm its specification.

[TurkeyMan]

Spec is here: dlang/dlang.org#3924

[TurkeyMan]

Okay, it's only that forward reference one left. I have no idea how to approach that...

The build kite things look spurious; I repro-ed them and couldn't find any way they relate to this change.

[bangbangsheshotmedown]

I approve this PR, i have been using this flag for YEARS, i couldn't use D without it

[rikkimax]

Okay, it's only that forward reference one left. I have no idea how to approach that...

Digging through the changelog I found one potential PR that may have fixed this issue in the past.

#12801

Perhaps that will shed some light on how to proceed.

[rikkimax]

Okay after looking at it, I can see that it won't help.

Looking at the AST dump wasn't helpful as well.

It'll need debugging internal AST information as to why other compiler analysis is failing.

[TurkeyMan]

Yeah I spent a while looking at it; I isolated the internal flow, but fixing that one is above my pay grade... :/

It's so contrived and higgledy-piggledy; I'm not even sure this code should work!

The test is this:

struct Node
{
    mixin Type!();
    Pointer left;
}

mixin template Type()
{
    alias Base = typeof(this);

    static struct Proxy
    {
        struct Node
        {
            Base m_payload;
        }
        static immutable default_value = Base.init; // just remove this will work
    }

    alias pNode = shared(Proxy.Node)*;

    static struct Pointer
    {
        Base*   _ptr;
        auto ptr()
        {
            return cast(pNode) _ptr;
        }

        void opAssign(ref Pointer other) {} // just remove this will work

        alias getThis this; // just remove this will work
        ref auto getThis() return
        {
            return ptr.m_payload;
        }
    }
}

It's that void opAssign(ref Pointer other) which leads to the error.
The assignment on this line that invokes opAssign: static immutable default_value = Base.init
That causes the Pointer left; member in Base (actually Node) to be copied.

While performing semantic analysis on the call to opAssign, it encounters this:

MATCH argumentMatchParameter (...)
{
    ...

        else if (global.params.rvalueRefParam != FeatureState.enabled ||
                 p.storageClass & STC.out_ ||
                 !arg.type.isCopyable())  // can't copy to temp for ref parameter
        {
            if (pMessage) *pMessage = tf.getParamError(arg, p);
            return MATCH.nomatch;
        }
        else
        {
            /* in functionParameters() we'll convert this
             * rvalue into a temporary
             */
            m = MATCH.convert;
        }

In that else if(...); we check if this flag is NOT enabled, then the those other 2 checks. Both those other 2 checks are false here, but global.params.rvalueRefParam != FeatureState.enabled was previously true, and so it entered this condition and return MATCH.nomatch.

NOW, the other 2 conditions are still false, but global.params.rvalueRefParam != FeatureState.enabled is ALSO false, and so it doesn't enter that case, and instead drops down to the else block, and m = MATCH.convert...

I guess that return MATCH.nomatch short-circuited some semantic evaluation, and so the cycle could resolve, but now that matchParameter is MATCH.convert, I guess the calling code will continue with more semantic analysis... and I guess that continuation fails to short-circuit the ref-cycle.

[TurkeyMan]

Hey @RazvanN7 @dkorpel, do either of you know how to approach this last error? I've fought through shit loads, but this last one has be stumped...

[dkorpel]

The first step is to make the test case as small as possible. I reduced it to this:

immutable default_value = Node.init;

struct Pointer
{
	void opAssign(ref Pointer other) {}
	alias getThis this;
	auto getThis() => Node.init;
}

struct Node
{
    Pointer left;
}

Then break on the moment the compiler emitted the "no size because of forward reference" and trace back to find what it was trying to do. It's indeed related to opAssign, but not from calling that function, but building it for Pointer. In dmd/clone.d:102 hasIdentityOpAssign, it checks if opAssign(__rvalue(Pointer())) already compiles, which -preview=rvaluerefparam affects.

I guess that return MATCH.nomatch short-circuited some semantic evaluation, and so the cycle could resolve, but now that matchParameter is MATCH.convert, I guess the calling code will continue with more semantic analysis... and I guess that continuation fails to short-circuit the ref-cycle.

That's right, if you look right below the code you quoted you find this part:

dmd/compiler/src/dmd/typesem.d

Line 1106 in 949cb11

/* If the match is not already perfect or if the arg

    /* If the match is not already perfect or if the arg
       is not a lvalue then try the `alias this` chain
       see  https://issues.dlang.org/show_bug.cgi?id=15674
       and https://issues.dlang.org/show_bug.cgi?id=21905
    */
    if (ta != tp || !arg.isLvalue())
    {
        Type firsttab = ta.toBasetype();
        while (1)
        {
            Type tab = ta.toBasetype();
            Type tat = tab.aliasthisOf();
            if (!tat || !tat.implicitConvTo(tprm))
                break;
            if (tat == tab || tat == firsttab)
                break;
            ta = tat;
        }
    }

It's the tab.aliasthisOf() that triggers semantic analysis of auto getThis() which results in the circular reference error.

I'm not sure what's the best fix for this yet, but looking at buildkite, it seems this PR causes more subtle errors, so I don't think a small opAssign related fix will be sufficient.

[TurkeyMan]

Do you reckon those buildkite errors are a symptom of this? Do they all reliably pass on all other PR's? I never looked at any other DMD PR's.
They look completely unrelated... :/

[dkorpel]

Buildkite is pretty reliable, unless you're really behind master maybe. This branch is currently 54 commits behind, but even after rebasing, I can locally reproduce the unit-threaded failure for one. I haven't tried the other failing projects yet.

[dkorpel]

The error in std_data_json can be reproduced with:

void put(char[] x, const char[] s) {}

void main()
{
    import std.bigint;
    char[] dst;
    BigInt num;
    num.toString(str => dst.put(str), null);
}

[dkorpel]

Reduced to remove Phobos dependency:

void toString(Writer)(ref Writer sink) {}

void toString(void delegate(int) sink)
{
	toString!(void delegate(int))(sink);
}

void put() {}

void main()
{
    toString(str => put());
}

[TurkeyMan]

Ahhh, yeah okay I see. So, there's a catch-all template overload which is in competition with another particular overload that's probably not an exact match. When called with an r-value, and the catch-all template by ref was previously a no-match, but now it can get on with catching-all over a broader set of calling situations.

This is essentially a problem with the code; catch-all templates are always a bad code practise, especially when they are overloaded against other more specific functions. This situation would have existed when calling with an l-value; just that the existing code either never did, or was worked properly to avoid this collision (with an explicit cast or whatever)... this particular situation may occur in the wild, and the resolution is for the calling code to be more specific; or coerce the proper selection with a cast or something... or you know, just NOT write catch-all templates overloaded with other functions in the first place!

So, we need to make a call if this breakage is accepted.
What it breaks is essentially a bad code-smell to begin with... and I never feel upset about breaking code that's already essentially broken. This is going to get peoples noses out of joint though :/

What do you reckon?

[rikkimax]

If we break, detect and explain what went wrong and how to fix it.

As long as we do that, and it is genuinely not what people are thinking it is doing, then IMO break away.

[TurkeyMan]

I mean, this is from phobos right? Writer should be constrained to something that looks like a Writer... D desperately needs concepts, but in this case, at least a proper constraint would address this.

[TurkeyMan]

All that said; that category of issues is easy to understand. The circular-reference issue is still a problem. I logged another one on the bug tracker last night too.
I've been running into circular reference issues a bit recently; I'm wondering if there's a small regression somewhere recently that makes it easier or more likely to encounter a circular reference...

[dkorpel]

This is essentially a problem with the code

There's two overloads, one matches, one doesn't, so it should just pick the one that matches. The unconstrained template is not a problem, you can add if (isOutputRange!(Writer, char) and the bug still occurs. Note that if Writer is auto ref or non-ref, it also just works. Also peculiar is that:

int toString(Writer)(ref Writer sink) => 3;
int toString(void delegate(scope const(char)[]) sink) => 4;
void put() {}
pragma(msg, toString(dst => put()));

Prints:

onlineapp.d(1): Error: cannot have parameter of type `void`
4

Despite the error, it succesfully calls the right overload. This is good news, it means we're simply dealing with a bug of a speculative error from trying overloads escaping into the main compilation. This is the offending branch that changes the 'deduce argument' oarg from the lambda to a void:

dmd/compiler/src/dmd/templatesem.d

Line 1255 in 519d388

if ((fparam.storageClass & STC.ref_) && (!(fparam.storageClass & STC.auto_) || farg.isLvalue()))

This used to result in a nomatch() anyway because lambda's aren't lvalues, but with -preview=rvaluerefparam this isn't the case anymore. I'm trying to fix it so it doesn't deduce void: #17096

[dkorpel]

Phobos unittests also fail to compile with this PR:

std/bigint.d(1375): Error: `@safe` function `std.bigint.BigInt.__unittest_L1368_C11` cannot call `@system` function `std.format.format!(char, BigInt).format`
std/format/write.d(527):        which calls `std.format.write.formattedWrite!(Appender!string, char, BigInt).formattedWrite`
std/format/write.d(1231):        which calls `std.format.write.formatValue!(Appender!string, BigInt, char).formatValue`
std/format/internal/write.d(2506):        which calls `std.format.internal.write.formatValueImpl!(Appender!string, BigInt, char).formatValueImpl`
std/format/internal/write.d(2141):        which calls `std.format.internal.write.formatObject!(Appender!string, BigInt, char).formatObject`
std/bigint.d(1390):        which calls `std.bigint.BigInt.toString`

I dustmited it and tracked it down to this part:

https://github.com/dlang/phobos/blob/0744ca67542b50dc5b2c36135a4a020517037974/std/format/internal/write.d#L1845

else static if (is(typeof(
        (T val) {
            const FormatSpec!Char f;
            static struct S {void put(scope Char s){}}
            S s;
            val.toString(s, f);
            static assert(!__traits(compiles, val.toString(s, FormatSpec!Char())),
                          "force toString to take parameters by ref");
            static assert(!__traits(compiles, val.toString(S(), f)),
                          "force toString to take parameters by ref");
        })))
    {
        enum hasToString = HasToStringResult.customPutWriterFormatSpec;
    }

"force toString to take parameters by ref" by asserting passing an rvalue doesn't compile obviously doesn't work anymore, so it now fails to select customPutWriterFormatSpec and selects the delegate overload instead, which isn't @safe.

[dkorpel]

This fails Phobos unittests:

../dmd/druntime/import/core/internal/traits.d(254): Error: sequence index `[0]` is outside bounds `[0 .. 0]`
std/traits.d(3830): Error: template instance `core.internal.traits.hasElaborateMove!(S5)` error instantiating
std/traits.d(3830):        while evaluating: `static assert(!hasElaborateMove!(S5))`