Skip to content

Commit

Permalink
Improve opOpAssign error messages (#20800)
Browse files Browse the repository at this point in the history
  • Loading branch information
@dkorpel
dkorpel authored Jan 30, 2025
1 parent 28bc5c6 commit 06e0096
Show file tree
Hide file tree
Showing 7 changed files with 267 additions and 196 deletions.
2 changes: 1 addition & 1 deletion changelog/dmd.error-messages.dd
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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:

---
Expand Down
36 changes: 25 additions & 11 deletions compiler/src/dmd/expressionsem.d
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3819,6 +3819,9 @@ private extern (C++) final class ExpressionSemanticVisitor : Visitor
// 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;
Expand Down Expand Up @@ -7583,15 +7586,8 @@ private extern (C++) final class ExpressionSemanticVisitor : Visitor
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();
Expand Down Expand Up @@ -10233,7 +10229,7 @@ private extern (C++) final class ExpressionSemanticVisitor : Visitor
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);
}

/*
Expand Down Expand Up @@ -11745,7 +11741,8 @@ private extern (C++) final class ExpressionSemanticVisitor : Visitor
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);
Expand Down Expand Up @@ -11824,6 +11821,9 @@ private extern (C++) final class ExpressionSemanticVisitor : Visitor
return;
}

if (exp.suggestOpOpAssign(sc, parent))
return setError();

if (SliceExp se = exp.e1.isSliceExp())
{
if (se.e1.type.toBasetype().ty == Tsarray)
Expand Down Expand Up @@ -13877,6 +13877,20 @@ private Expression expressionSemantic(Expression e, Scope* sc, Type[2] aliasThis
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));
Expand Down
117 changes: 70 additions & 47 deletions compiler/src/dmd/opover.d
View file
Original file line number Diff line number Diff line change
Expand Up @@ -94,58 +94,40 @@ private bool hasOpBinary(EXP op) pure @safe
}
}

/**
* 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);
Expand Down Expand Up @@ -617,6 +599,7 @@ bool suggestBinaryOverloads(BinExp e, Scope* sc)
{
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;
Expand All @@ -640,6 +623,46 @@ bool suggestBinaryOverloads(BinExp e, Scope* sc)
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...)
{
Expand Down
Loading

0 comments on commit 06e0096

Please sign in to comment.