Ref parameter overload not called when struct implements copy constructor

[ibuclaw]

When there is a move assignment implemented, the copy assignment is ignored, even though it is the correct function to call.

struct S
{
    int data;

    ~this()                     // I. destructor
    { assert(true); }
    this(ref return scope S)    // II. copy constructor
    { assert(false); }
    ref S opAssign(const ref S) // III. copy assignment
    { return this; }
    this(return scope S)        // IV. move constructor
    { assert(false); }
    ref S opAssign(const S)     // V. move assignment 
    { assert(false); }
}

void test()
{
    S o1, o2;
    o2 = o1;    // Should use III, instead uses II + V.
}

Remove the move assignment, and the copy assignment path is correctly used.

[ibuclaw]

FYI @TurkeyMan

[ibuclaw]

This picks the move opAssign overload because of this branch.

dmd/compiler/src/dmd/typesem.d

Lines 1072 to 1078 in cbbca2b

	// check if the copy constructor may be called to copy the argument
	if (arg.isLvalue() && !isRef && argStruct && argStruct == prmStruct && argStruct.hasCopyCtor)
	{
	if (!isCopyConstructorCallable(argStruct, arg, tprm, sc, pMessage))
	return MATCH.nomatch;
	m = MATCH.exact;
	}

We have an lvalue argument (o1) and a non-ref parameter (const S) and the existence of a copy constructor (this(ref return scope S)), so MATCH.exact is taken.

This can be confirmed by looked at the generated code.

o2.opAssign(((const const(S21478) __copytmp4 = __copytmp4 = 0 , __copytmp4.__ctor(o1);) , __copytmp4));

We can give the lvalue arg + ref param an exact match as well, but there's a few hurdles to jump through in order to ensure we are dealing with an identity assign operator (that isn't @disabled, etc).

+        bool isIdentityAssignArg()
+        {
+            if (fd is null || fd.ident != Id.opAssign)
+                return false;
+            if (fd.isDisabled())
+                return false;
+            if (argStruct is null || argStruct != prmStruct)
+                return false;
+            if (!argStruct.hasIdentityAssign)
+                return false;
+            if (argStruct != fd.isMember())
+                return false;
+
+            auto fparams = fd.getParameterList();
+            if (fparams.length != 1)
+                return false;
+            if (p != fparams[0] || p.type.toDsymbol(null) != argStruct)
+                return false;
+
+            return true;
+        }
+        if (arg.isLvalue() && isRef && isIdentityAssignArg())
+        {
+            m = MATCH.exact;
+        }
         // check if the copy constructor may be called to copy the argument
         else if (arg.isLvalue() && !isRef && argStruct && argStruct == prmStruct && argStruct.hasCopyCtor)

This works, because when there's two MATCH.exact functions, the ref always wins with lvalues latter somewhere around the leastAsSpecialized() comparison.

There's also what to do when there's

void opAssign(ref S, int = 0);
void opAssign(S);

Which triggers the matches multiple overloads exactly error with the above patch (if the fparams.length gate were to be removed).

I'd be more comfortable then if accepted fix did not look at the argument/parameter match in isolation, but to instead add new context to either StructDeclaration - eg: hasLvalueIdentityAssign/hasRvalueIdentityAssign or a reference to these member functions - or have a new node class OpAssignDeclaration : FuncDeclaration { bool isCopyAssign; bool isMoveAssign; } so that we can do the same as per copy/move constructor matches.

if (cfd.isCpCtor && !arg.isLvalue())
    return MATCH.nomatch;       // copy constructor is only for lvalues
if (cfd.isMoveCtor && arg.isLvalue())
    return MATCH.nomatch;       // move constructor is only for rvalues

[ibuclaw]

Turns out it's not just opAssign affected. This rule in the spec is not adhered to when there's a copy constructor defined.

https://dlang.org/spec/expression.html#RvalueExpression

11.19.16.1 Overloading

1. If both ref and non-ref parameter overloads are present, an rvalue is preferably matched to the non-ref parameters, and an lvalue is preferably matched to the ref parameter. An RvalueExpression will preferably match with the non-ref parameter.

struct S
{
    this(ref return scope S) { }
}

struct T
{
    int plain_old_data;
}

int overload(const S) { return 1; }
int overload(const ref S) { return 2; }

int overload(const T) { return 1; }
int overload(const ref T) { return 2; }

void main()
{
    S s;
    assert(overload(s) == 1);  // erroneously calls copy constructor
                               // to convert lvalue to rvalue 
    T t;
    assert(overload(t) == 2);
}