Improve opOpAssign error messages

changelog/dmd.error-messages.dd

@@ -53,7 +53,7 @@ app.d(1):        perhaps define `auto opSlice(int lower, string upper) {}` for `
 */
 ---
 
-When overloading binary operators, and `opBinary` or `opBinaryRight` is missing or doesn't match,
+When overloading binary operators, and `opBinary`, `opBinaryRight` or `opOpAssign` is missing / fails to instantiate,
 the error message now points out the problem:
 
 ---

compiler/src/dmd/expressionsem.d

@@ -3819,6 +3819,9 @@
     // See tryAliasThisSemantic
     Type[2] aliasThisStop;
 
+    // (Optional) the expression this was lowered from, for better error messages
+    Expression parent;
+
     this(Scope* sc) scope @safe
     {
         this.sc = sc;
@@ -7583,15 +7586,8 @@
         exp.e1 = exp.e1.optimize(WANTvalue, /*keepLvalue*/ true);
         exp.type = exp.e1.type;
 
-        if (auto ad = isAggregate(exp.e1.type))
-        {
-            if (const s = search_function(ad, Id.opOpAssign))
-            {
-                error(exp.loc, "none of the `opOpAssign` overloads of `%s` are callable for `%s` of type `%s`", ad.toChars(), exp.e1.toChars(), exp.e1.type.toChars());
-                return setError();
-            }
-        }
-        if (exp.e1.checkScalar() ||
+        if (exp.suggestOpOpAssign(sc, parent) ||
+            exp.e1.checkScalar() ||
             exp.e1.checkReadModifyWrite(exp.op, exp.e2) ||
             exp.e1.checkSharedAccess(sc))
             return setError();
@@ -10233,7 +10229,7 @@
             e = new AddAssignExp(exp.loc, exp.e1, IntegerExp.literal!1);
         else
             e = new MinAssignExp(exp.loc, exp.e1, IntegerExp.literal!1);
-        result = e.expressionSemantic(sc);
+        result = e.expressionSemanticWithParent(sc, exp);
     }
 
     /*
@@ -11745,7 +11741,8 @@
             return;
         }
 
-        if (exp.e1.checkReadModifyWrite(exp.op, exp.e2))
+        if (exp.suggestOpOpAssign(sc, parent) ||
+            exp.e1.checkReadModifyWrite(exp.op, exp.e2))
             return setError();
 
         assert(exp.e1.type && exp.e2.type);
@@ -11824,6 +11821,9 @@
             return;
         }
 
+        if (exp.suggestOpOpAssign(sc, parent))
+            return setError();
+
         if (SliceExp se = exp.e1.isSliceExp())
         {
             if (se.e1.type.toBasetype().ty == Tsarray)
@@ -13877,6 +13877,20 @@
     return v.result;
 }
 
+// ditto, but with `parent` parameter that represents the expression before rewriting.
+// This way, when lowering an expression (e.g. i++ to i+=1), error messages can still
+// refer to the original expression.
+private Expression expressionSemanticWithParent(Expression e, Scope* sc, Expression parent)
+{
+    if (e.expressionSemanticDone)
+        return e;
+
+    scope v = new ExpressionSemanticVisitor(sc);
+    v.parent = parent;
+    e.accept(v);
+    return v.result;
+}
+
 private Expression dotIdSemanticPropX(DotIdExp exp, Scope* sc)
 {
     //printf("dotIdSemanticPropX() %s\n", toChars(exp));

compiler/src/dmd/opover.d

@@ -94,58 +94,40 @@
     }
 }
 
+/**
+ * Remove the = from op=, e.g. += becomes +
+ *
+ * Params:
+ *     op = tag for a binary assign operator
+ * Returns: the corresponding binary operator, or `op` if it wasn't an assign operator
+*/
+private EXP stripAssignOp(EXP op)
+{
+    switch (op)
+    {
+    case EXP.addAssign: return EXP.add;
+    case EXP.minAssign: return EXP.min;
+    case EXP.mulAssign: return EXP.mul;
+    case EXP.divAssign: return EXP.div;
+    case EXP.modAssign: return EXP.mod;
+    case EXP.andAssign: return EXP.and;
+    case EXP.orAssign: return EXP.or;
+    case EXP.xorAssign: return EXP.xor;
+    case EXP.leftShiftAssign: return EXP.leftShift;
+    case EXP.rightShiftAssign: return EXP.rightShift;
+    case EXP.unsignedRightShiftAssign: return EXP.unsignedRightShift;
+    case EXP.concatenateAssign: return EXP.concatenate;
+    case EXP.powAssign: return EXP.pow;
+    default: return op;
+    }
+}
+
 /*******************************************
  * Helper function to turn operator into template argument list
  */
 Objects* opToArg(Scope* sc, EXP op)
 {
-    /* Remove the = from op=
-     */
-    switch (op)
-    {
-    case EXP.addAssign:
-        op = EXP.add;
-        break;
-    case EXP.minAssign:
-        op = EXP.min;
-        break;
-    case EXP.mulAssign:
-        op = EXP.mul;
-        break;
-    case EXP.divAssign:
-        op = EXP.div;
-        break;
-    case EXP.modAssign:
-        op = EXP.mod;
-        break;
-    case EXP.andAssign:
-        op = EXP.and;
-        break;
-    case EXP.orAssign:
-        op = EXP.or;
-        break;
-    case EXP.xorAssign:
-        op = EXP.xor;
-        break;
-    case EXP.leftShiftAssign:
-        op = EXP.leftShift;
-        break;
-    case EXP.rightShiftAssign:
-        op = EXP.rightShift;
-        break;
-    case EXP.unsignedRightShiftAssign:
-        op = EXP.unsignedRightShift;
-        break;
-    case EXP.concatenateAssign:
-        op = EXP.concatenate;
-        break;
-    case EXP.powAssign:
-        op = EXP.pow;
-        break;
-    default:
-        break;
-    }
-    Expression e = new StringExp(Loc.initial, EXPtoString(op));
+    Expression e = new StringExp(Loc.initial, EXPtoString(stripAssignOp(op)));
     e = e.expressionSemantic(sc);
     auto tiargs = new Objects();
     tiargs.push(e);
@@ -617,6 +599,7 @@
     {
         if (Dsymbol s = search_function(ad1, Id.opBinary))
         {
+            // This expressionSemantic will fail, otherwise operator overloading would have succeeded before
             dotTemplateCall(e.e1, Id.opBinary, opToArg(sc, e.op), e.e2).expressionSemantic(sc);
             errorSupplemental(s.loc, "`opBinary` defined here");
             return true;
@@ -640,6 +623,46 @@
     return false;
 }
 
+/**
+ * If applicable, print an error relating to implementing / fixing `opOpAssign` or `opUnary` functions.
+ * Params:
+ *   exp = binary operation
+ *   sc = scope to try `opOpAssign!""` semantic in for error messages
+ *   parent = if `exp` was lowered from this `PreExp` or `PostExp`, mention `opUnary` as well
+ * Returns: `true` when an error related to `opOpAssign` was printed
+ */
+bool suggestOpOpAssign(BinAssignExp exp, Scope* sc, Expression parent)
+{
+    auto ad = isAggregate(exp.e1.type);
+    if (!ad)
+        return false;
+
+    if (parent && (parent.isPreExp() || parent.isPostExp()))
+    {
+        error(exp.loc, "operator `%s` not supported for `%s` of type `%s`", EXPtoString(parent.op).ptr, exp.e1.toChars(), ad.toChars());
+        errorSupplemental(ad.loc,
+            "perhaps implement `auto opUnary(string op : \"%s\")() {}`"~
+            " or `auto opOpAssign(string op : \"%s\")(int) {}`",
+            EXPtoString(stripAssignOp(parent.op)).ptr,
+            EXPtoString(stripAssignOp(exp.op)).ptr
+            );
+        return true;
+    }
+
+    if (const s = search_function(ad, Id.opOpAssign))
+    {
+        // This expressionSemantic will fail, otherwise operator overloading would have succeeded before
+        dotTemplateCall(exp.e1, Id.opOpAssign, opToArg(sc, exp.op), exp.e2).expressionSemantic(sc);
+    }
+    else
+    {
+        error(exp.loc, "operator `%s` not supported for `%s` of type `%s`", EXPtoString(exp.op).ptr, exp.e1.toChars(), ad.toChars());
+        errorSupplemental(ad.loc, "perhaps implement `auto opOpAssign(string op : \"%s\")(%s) {}`",
+            EXPtoString(stripAssignOp(exp.op)).ptr, exp.e2.type.toChars());
+    }
+    return true;
+}
+
 // Helper to construct e.id!tiargs(args), e.g. `lhs.opBinary!"+"(rhs)`
 private Expression dotTemplateCall(Expression e, Identifier id, Objects* tiargs, Expression[] args...)
 {