Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #20583 - Initialisation of @mustuse member variable causes spur… #20584

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
+37 −1
Open

Fix #20583 - Initialisation of @mustuse member variable causes spur… #20584

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
968996f
Fix #20583 - Initialisation of `@mustuse` member variable causes spur…
thewilsonator Dec 21, 2024
848fcc5
Update compiler/src/dmd/mustuse.d
thewilsonator Dec 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 23 additions & 1 deletion compiler/src/dmd/mustuse.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,10 @@ bool checkMustUse(Expression e, Scope* sc)
{
auto sd = sym.isStructDeclaration();
// isStructDeclaration returns non-null for both structs and unions
if (sd && hasMustUseAttribute(sd, sc) && !isAssignment(e) && !isIncrementOrDecrement(e))
if (sd && hasMustUseAttribute(sd, sc)
&& !isAssignment(e)
&& !isCtorAssignment(e)
&& !isIncrementOrDecrement(e))
{
error(e.loc, "ignored value of `@%s` type `%s`; prepend a `cast(void)` if intentional",
Id.udaMustUse.toChars(), e.type.toPrettyChars(true));
Expand Down Expand Up @@ -104,6 +107,25 @@ private bool isAssignment(Expression e)
return false;
}

/**
* Returns: true if `e` is
* `(this.field = typeof(this.field).init, this.field.__ctor(arg))`
*/
thewilsonator marked this conversation as resolved.
Show resolved Hide resolved
private bool isCtorAssignment(Expression e)
{
import dmd.id : Id;

auto comma = e.isCommaExp();
if (!comma)
return false;
if (!isAssignment(comma.e1))
return false;
auto call = comma.e2.isCallExp();
if (!call)
return false;
return call.f.ident == Id.ctor;
}

/**
* Returns: true if id is the identifier of an assignment operator overload.
*/
Expand Down
14 changes: 14 additions & 0 deletions compiler/test/compilable/must_use_assign.d
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at ExpressionSemanticVisitor.visit(AssignExp), there are several places where this kind of rewrite is done, and all of them are slightly different. The relevant code is here:

dmd/compiler/src/dmd/expressionsem.d

Lines 10808 to 11079 in 32a6dfe

if (exp.op == EXP.construct)
{
Type t2 = e2x.type.toBasetype();
if (t2.ty == Tstruct && sd == (cast(TypeStruct)t2).sym)
{
sd.size(exp.loc);
if (sd.sizeok != Sizeok.done)
return setError();
if (!sd.ctor)
sd.ctor = sd.searchCtor();
// https://issues.dlang.org/show_bug.cgi?id=15661
// Look for the form from last of comma chain.
auto e2y = lastComma(e2x);
CallExp ce = (e2y.op == EXP.call) ? cast(CallExp)e2y : null;
DotVarExp dve = (ce && ce.e1.op == EXP.dotVariable)
? cast(DotVarExp)ce.e1 : null;
if (sd.ctor && ce && dve && dve.var.isCtorDeclaration() &&
// https://issues.dlang.org/show_bug.cgi?id=19389
dve.e1.op != EXP.dotVariable &&
e2y.type.implicitConvTo(t1))
{
/* Look for form of constructor call which is:
* __ctmp.ctor(arguments...)
*/
/* Before calling the constructor, initialize
* variable with a bit copy of the default
* initializer
*/
Expression einit = getInitExp(sd, exp.loc, sc, t1);
if (einit.op == EXP.error)
{
result = einit;
return;
}
auto ae = new BlitExp(exp.loc, exp.e1, einit);
ae.type = e1x.type;
/* Replace __ctmp being constructed with e1.
* We need to copy constructor call expression,
* because it may be used in other place.
*/
auto dvx = cast(DotVarExp)dve.copy();
dvx.e1 = e1x;
auto cx = cast(CallExp)ce.copy();
cx.e1 = dvx;
if (checkConstructorEscape(*sc, cx, false))
return setError();
Expression e0;
Expression.extractLast(e2x, e0);
auto e = Expression.combine(e0, ae, cx);
e = e.expressionSemantic(sc);
result = e;
return;
}
// https://issues.dlang.org/show_bug.cgi?id=21586
// Rewrite CondExp or e1 will miss direct construction, e.g.
// e1 = a ? S(1) : ...; -> AST: e1 = a ? (S(0)).this(1) : ...;
// a temporary created and an extra destructor call.
// AST will be rewritten to:
// a ? e1 = 0, e1.this(1) : ...; -> blitting plus construction
if (e2x.op == EXP.question)
{
/* Rewrite as:
* a ? e1 = b : e1 = c;
*/
CondExp econd = cast(CondExp)e2x;
Expression ea1 = new ConstructExp(econd.e1.loc, e1x, econd.e1);
Expression ea2 = new ConstructExp(econd.e2.loc, e1x, econd.e2);
Expression e = new CondExp(exp.loc, econd.econd, ea1, ea2);
result = e.expressionSemantic(sc);
return;
}
if (sd.postblit || sd.hasCopyCtor)
{
/* We have a copy constructor for this
*/
//printf("exp: %s\n", toChars(exp));
//printf("e2x: %s\n", toChars(e2x));
if (e2x.isLvalue())
{
if (sd.hasCopyCtor)
{
/* Rewrite as:
* e1 = init, e1.copyCtor(e2);
*/
Expression einit = new BlitExp(exp.loc, exp.e1, getInitExp(sd, exp.loc, sc, t1));
einit.type = e1x.type;
Expression e;
e = new DotIdExp(exp.loc, e1x, Id.ctor);
e = new CallExp(exp.loc, e, e2x);
e = new CommaExp(exp.loc, einit, e);
//printf("e: %s\n", e.toChars());
result = e.expressionSemantic(sc);
return;
}
else
{
if (!e2x.type.implicitConvTo(e1x.type))
{
error(exp.loc, "conversion error from `%s` to `%s`",
e2x.type.toChars(), e1x.type.toChars());
return setError();
}
/* Rewrite as:
* (e1 = e2).postblit();
*
* Blit assignment e1 = e2 returns a reference to the original e1,
* then call the postblit on it.
*/
Expression e = e1x.copy();
e.type = e.type.mutableOf();
if (e.type.isShared && !sd.type.isShared)
e.type = e.type.unSharedOf();
e = new BlitExp(exp.loc, e, e2x);
e = new DotVarExp(exp.loc, e, sd.postblit, false);
e = new CallExp(exp.loc, e);
result = e.expressionSemantic(sc);
return;
}
}
else if (sd.hasMoveCtor && !e2x.isCallExp() && !e2x.isStructLiteralExp())
{
/* The !e2x.isCallExp() is because it is already an rvalue
and the move constructor is unnecessary:
struct S {
alias TT this;
long TT();
this(T)(int x) {}
this(S);
this(ref S);
~this();
}
S fun(ref S arg);
void test() { S st; fun(st); }
*/
/* Rewrite as:
* e1 = init, e1.moveCtor(e2);
*/
Expression einit = new BlitExp(exp.loc, exp.e1, getInitExp(sd, exp.loc, sc, t1));
einit.type = e1x.type;
Expression e;
e = new DotIdExp(exp.loc, e1x, Id.ctor);
e = new CallExp(exp.loc, e, e2x);
e = new CommaExp(exp.loc, einit, e);
//printf("e: %s\n", e.toChars());
result = e.expressionSemantic(sc);
return;
}
else
{
/* The struct value returned from the function is transferred
* so should not call the destructor on it.
*/
e2x = valueNoDtor(e2x);
}
}
// https://issues.dlang.org/show_bug.cgi?id=19251
// if e2 cannot be converted to e1.type, maybe there is an alias this
if (!e2x.implicitConvTo(t1))
{
AggregateDeclaration ad2 = isAggregate(e2x.type);
if (ad2 && ad2.aliasthis && !isRecursiveAliasThis(exp.att2, exp.e2.type))
{
/* Rewrite (e1 op e2) as:
* (e1 op e2.aliasthis)
*/
exp.e2 = new DotIdExp(exp.e2.loc, exp.e2, ad2.aliasthis.ident);
result = exp.expressionSemantic(sc);
return;
}
}
}
else if (!e2x.implicitConvTo(t1))
{
sd.size(exp.loc);
if (sd.sizeok != Sizeok.done)
return setError();
if (!sd.ctor)
sd.ctor = sd.searchCtor();
if (sd.ctor)
{
/* Look for implicit constructor call
* Rewrite as:
* e1 = init, e1.ctor(e2)
*/
/* Fix Issue 5153 : https://issues.dlang.org/show_bug.cgi?id=5153
* Using `new` to initialize a struct object is a common mistake, but
* the error message from the compiler is not very helpful in that
* case. If exp.e2 is a NewExp and the type of new is the same as
* the type as exp.e1 (struct in this case), then we know for sure
* that the user wants to instantiate a struct. This is done to avoid
* issuing an error when the user actually wants to call a constructor
* which receives a class object.
*
* Foo f = new Foo2(0); is a valid expression if Foo has a constructor
* which receives an instance of a Foo2 class
*/
if (exp.e2.op == EXP.new_)
{
auto newExp = cast(NewExp)(exp.e2);
if (newExp.newtype && newExp.newtype == t1)
{
error(exp.loc, "cannot implicitly convert expression `%s` of type `%s` to `%s`",
newExp.toChars(), newExp.type.toChars(), t1.toChars());
errorSupplemental(exp.loc, "Perhaps remove the `new` keyword?");
return setError();
}
}
Expression einit = new BlitExp(exp.loc, e1x, getInitExp(sd, exp.loc, sc, t1));
einit.type = e1x.type;
Expression e;
e = new DotIdExp(exp.loc, e1x, Id.ctor);
e = new CallExp(exp.loc, e, e2x);
e = new CommaExp(exp.loc, einit, e);
e = e.expressionSemantic(sc);
result = e;
return;
}
if (search_function(sd, Id.call))
{
/* Look for static opCall
* https://issues.dlang.org/show_bug.cgi?id=2702
* Rewrite as:
* e1 = typeof(e1).opCall(arguments)
*/
e2x = typeDotIdExp(e2x.loc, e1x.type, Id.call);
e2x = new CallExp(exp.loc, e2x, exp.e2);
e2x = e2x.expressionSemantic(sc);
e2x = resolveProperties(sc, e2x);
if (e2x.op == EXP.error)
{
result = e2x;
return;
}
if (e2x.checkValue() || e2x.checkSharedAccess(sc))
return setError();
}
}
else // https://issues.dlang.org/show_bug.cgi?id=11355
{
AggregateDeclaration ad2 = isAggregate(e2x.type);
if (ad2 && ad2.aliasthis && !isRecursiveAliasThis(exp.att2, exp.e2.type))
{
/* Rewrite (e1 op e2) as:
* (e1 op e2.aliasthis)
*/
exp.e2 = new DotIdExp(exp.e2.loc, exp.e2, ad2.aliasthis.ident);
result = exp.expressionSemantic(sc);
return;
}
}
}

To ensure that we have proper coverage of all these cases, the test should include

  • An example that uses a copy constructor.
  • An example that uses a move constructor.
  • An example that uses a postblit.
  • An example where the right-hand side of the assignment is a conditional expression.

Original file line number Diff line number Diff line change
Expand Up @@ -7,3 +7,17 @@ void test()
S a, b;
a = b;
}

@mustuse struct Inner
{
this(int n) {}
}

struct Outer
{
Inner inner;
this(int n)
{
this.inner = n;
}
}
Loading