Skip to content

Commit

Permalink
Improve opBinary error messages (#20789)
Browse files Browse the repository at this point in the history
  • Loading branch information
@dkorpel
dkorpel authored Jan 27, 2025
1 parent 2ca7960 commit ea9ead9
Show file tree
Hide file tree
Showing 10 changed files with 299 additions and 105 deletions.
45 changes: 45 additions & 0 deletions changelog/dmd.error-messages.dd
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,51 @@ app.d(6): `app.U` declared here
*/
---

When overloading binary operators, and `opBinary` or `opBinaryRight` is missing or doesn't match,
the error message now points out the problem:

---
struct Str {}

struct Number
{
int x;
int opBinary(string op : "+")(int rhs) => this.x + x;
}

void f(Str str, Number number)
{
const s = str ~ "hey";
const n = number + "oops";
}

/*
Before:

app.d(12): Error: incompatible types for `(str) ~ ("hey")`: `Str` and `string`
const s = str ~ "hey";
^
app.d(13): Error: incompatible types for `(number) + ("oops")`: `Number` and `string`
const n = number + "oops";

After:

app.d(12): Error: operator `~` is not defined for type `Str`
const s = str ~ "hey";
^
app.d(2): perhaps overload the operator with `auto opBinary(string op : "~")(string rhs) {}`
struct Str {}
^
app.d(13): Error: function `test_.Number.opBinary!"+".opBinary(int rhs)` is not callable using argument types `(string)`
const n = number + "oops";
^
app.d(13): cannot pass argument `"oops"` of type `string` to parameter `int rhs`
app.d(7): `opBinary` defined here
int opBinary(string op : "+")(int rhs) => this.x + x;
^
*/
---

Furthermore:

- D1 operator overloading functions (`opAdd`, `opDot`) are completely removed and no longer mentioned in error messages specifically.
Expand Down
2 changes: 1 addition & 1 deletion compiler/src/dmd/dcast.d
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4138,7 +4138,7 @@ Expression typeCombine(BinExp be, Scope* sc)
{
Expression errorReturn()
{
Expression ex = be.incompatibleTypes();
Expression ex = be.incompatibleTypes(sc);
if (ex.op == EXP.error)
return ex;
return ErrorExp.get();
Expand Down
29 changes: 26 additions & 3 deletions compiler/src/dmd/expressionsem.d
View file
Original file line number Diff line number Diff line change
Expand Up @@ -377,7 +377,7 @@ private Expression incompatibleTypes(UnaExp e)
* Returns:
* ErrorExp
*/
extern (D) Expression incompatibleTypes(BinExp e)
extern (D) Expression incompatibleTypes(BinExp e, Scope* sc = null)
{
if (e.e1.type.toBasetype() == Type.terror)
return e.e1;
Expand All @@ -386,6 +386,10 @@ extern (D) Expression incompatibleTypes(BinExp e)

// CondExp uses 'a ? b : c' but we're comparing 'b : c'
const(char)* thisOp = (e.op == EXP.question) ? ":" : EXPtoString(e.op).ptr;

if (sc && suggestBinaryOverloads(e, sc))
return ErrorExp.get();

if (e.e1.op == EXP.type || e.e2.op == EXP.type)
{
error(e.loc, "incompatible types for `(%s) %s (%s)`: cannot use `%s` with types",
Expand Down Expand Up @@ -12092,6 +12096,9 @@ private extern (C++) final class ExpressionSemanticVisitor : Visitor
return;
}

if (exp.suggestBinaryOverloads(sc))
return setError();

if (exp.checkArithmeticBin())
return setError();

Expand Down Expand Up @@ -12245,6 +12252,9 @@ private extern (C++) final class ExpressionSemanticVisitor : Visitor
return;
}

if (exp.suggestBinaryOverloads(sc))
return setError();

if (exp.checkArithmeticBin())
return setError();

Expand Down Expand Up @@ -12578,6 +12588,12 @@ private extern (C++) final class ExpressionSemanticVisitor : Visitor
return true;
}

if (exp.suggestBinaryOverloads(sc))
{
setError();
return true;
}

if (exp.checkArithmeticBin() || exp.checkSharedAccessBin(sc))
{
setError();
Expand Down Expand Up @@ -12792,6 +12808,9 @@ private extern (C++) final class ExpressionSemanticVisitor : Visitor
return;
}

if (exp.suggestBinaryOverloads(sc))
return setError();

if (exp.checkIntegralBin() || exp.checkSharedAccessBin(sc))
return setError();

Expand Down Expand Up @@ -12862,6 +12881,10 @@ private extern (C++) final class ExpressionSemanticVisitor : Visitor
result = exp.incompatibleTypes();
return;
}

if (exp.suggestBinaryOverloads(sc))
return setError();

if (exp.checkIntegralBin() || exp.checkSharedAccessBin(sc))
return setError();

Expand Down Expand Up @@ -13119,15 +13142,15 @@ private extern (C++) final class ExpressionSemanticVisitor : Visitor
return setError();

case Tarray, Tsarray:
result = exp.incompatibleTypes();
result = exp.incompatibleTypes(sc);
errorSupplemental(exp.loc, "`in` is only allowed on associative arrays");
const(char)* slice = (t2b.ty == Tsarray) ? "[]" : "";
errorSupplemental(exp.loc, "perhaps use `std.algorithm.find(%s, %s%s)` instead",
exp.e1.toChars(), exp.e2.toChars(), slice);
return;

default:
result = exp.incompatibleTypes();
result = exp.incompatibleTypes(sc);
return;
}
result = exp;
Expand Down
82 changes: 78 additions & 4 deletions compiler/src/dmd/opover.d
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,29 @@ bool isCommutative(EXP op) @safe
return false;
}

/// Returns: whether `op` can be overloaded with `opBinary`
private bool hasOpBinary(EXP op) pure @safe
{
switch (op)
{
case EXP.add: return true;
case EXP.min: return true;
case EXP.mul: return true;
case EXP.div: return true;
case EXP.mod: return true;
case EXP.and: return true;
case EXP.or: return true;
case EXP.xor: return true;
case EXP.leftShift: return true;
case EXP.rightShift: return true;
case EXP.unsignedRightShift: return true;
case EXP.concatenate: return true;
case EXP.pow: return true;
case EXP.in_: return true;
default: return false;
}
}

/*******************************************
* Helper function to turn operator into template argument list
*/
Expand Down Expand Up @@ -591,12 +614,63 @@ Expression opOverloadBinary(BinExp e, Scope* sc, Type[2] aliasThisStop)
s_r = null;

bool choseReverse;
if (auto res = pickBestBinaryOverload(sc, opToArg(sc, e.op), s, s_r, e, choseReverse))
return res;
if (auto result = pickBestBinaryOverload(sc, opToArg(sc, e.op), s, s_r, e, choseReverse))
return result;

return binAliasThis(e, sc, aliasThisStop);
}

/**
* If applicable, print an error relating to implementing / fixing `opBinary` functions.
* Params:
* e = binary operation
* sc = scope to try `opBinary!""` semantic in for error messages
* Returns: `true` when an error related to `opBinary` was printed
*/
bool suggestBinaryOverloads(BinExp e, Scope* sc)
{
if (!e.op.hasOpBinary)
return false;

AggregateDeclaration ad1 = isAggregate(e.e1.type);
AggregateDeclaration ad2 = isAggregate(e.e2.type);

if (ad1)
{
if (Dsymbol s = search_function(ad1, Id.opBinary))
{
dotTemplate(e.e1, Id.opBinary, opToArg(sc, e.op), e.e2).expressionSemantic(sc);
errorSupplemental(s.loc, "`opBinary` defined here");
return true;
}
error(e.loc, "operator `%s` is not defined for type `%s`", EXPtoString(e.op).ptr, e.e1.type.toChars);
errorSupplemental(ad1.loc, "perhaps overload the operator with `auto opBinary(string op : \"%s\")(%s rhs) {}`", EXPtoString(e.op).ptr, e.e2.type.toChars);
return true;
}
else if (ad2)
{
if (Dsymbol s_r = search_function(ad1, Id.opBinaryRight))
{
dotTemplate(e.e2, Id.opBinaryRight, opToArg(sc, e.op), e.e1).expressionSemantic(sc);
errorSupplemental(s_r.loc, "`opBinaryRight` defined here");
return true;
}
error(e.loc, "operator `%s` is not defined for type `%s`", EXPtoString(e.op).ptr, e.e2.type.toChars);
errorSupplemental(ad2.loc, "perhaps overload the operator with `auto opBinaryRight(string op : \"%s\")(%s rhs) {}`", EXPtoString(e.op).ptr, e.e1.type.toChars);
return true;
}
return false;
}

// Helper to construct e.id!tiargs(arg)
private Expression dotTemplate(Expression e, Identifier id, Objects* tiargs, Expression arg)
{
auto ti = new DotTemplateInstanceExp(e.loc, e, id, tiargs);
auto args = new Expressions();
args.push(arg);
return new CallExp(e.loc, ti, args);
}

Expression opOverloadEqual(EqualExp e, Scope* sc, Type[2] aliasThisStop)
{
Type t1 = e.e1.type.toBasetype();
Expand Down Expand Up @@ -990,15 +1064,15 @@ private Expression pickBestBinaryOverload(Scope* sc, Objects* tiargs, Dsymbol s,

if (s)
{
functionResolve(m, s, e.loc, sc, tiargs, e.e1.type, ArgumentList(args2));
functionResolve(m, s, e.loc, sc, tiargs, e.e1.type, ArgumentList(args2), null);
if (m.lastf && (m.lastf.errors || m.lastf.hasSemantic3Errors()))
return ErrorExp.get();
}
FuncDeclaration lastf = m.lastf;
int count = m.count;
if (s_r)
{
functionResolve(m, s_r, e.loc, sc, tiargs, e.e2.type, ArgumentList(args1));
functionResolve(m, s_r, e.loc, sc, tiargs, e.e2.type, ArgumentList(args1), null);
if (m.lastf && (m.lastf.errors || m.lastf.hasSemantic3Errors()))
return ErrorExp.get();
}
Expand Down
Loading

0 comments on commit ea9ead9

Please sign in to comment.