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.

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 Issue 18708 - Flow analysis in constructors not done correctly fo… #8118

Merged
merged 1 commit into from
Apr 4, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 16 additions & 0 deletions src/dmd/ctorflow.d
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,22 @@ struct CtorFlow
foreach (ref u; fieldinit)
u |= csx;
}

/******************************
* OR CSX bits to `this`
* Params:
* ctorflow = bits to OR in
*/
void OR(const ref CtorFlow ctorflow)
{
callSuper |= ctorflow.callSuper;
if (fieldinit.length && ctorflow.fieldinit.length)
{
assert(fieldinit.length == ctorflow.fieldinit.length);
foreach (i, u; ctorflow.fieldinit)
fieldinit[i] |= u;
}
}
}


Expand Down
18 changes: 4 additions & 14 deletions src/dmd/dscope.d
Original file line number Diff line number Diff line change
Expand Up @@ -209,21 +209,11 @@ struct Scope
extern (C++) Scope* pop()
{
//printf("Scope::pop() %p nofree = %d\n", this, nofree);
Scope* enc = enclosing;
if (enclosing)
{
enclosing.ctorflow.callSuper |= ctorflow.callSuper;
if (ctorflow.fieldinit.length)
{
if (enclosing.ctorflow.fieldinit.length)
{
assert(ctorflow.fieldinit.ptr != enclosing.ctorflow.fieldinit.ptr);
foreach (i, u; ctorflow.fieldinit)
enclosing.ctorflow.fieldinit[i] |= u;
}
ctorflow.freeFieldinit();
}
}
enclosing.ctorflow.OR(ctorflow);
Copy link
Contributor

Choose a reason for hiding this comment

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

Indentention looks inconsistent.

Copy link
Contributor

@RazvanN7 RazvanN7 Apr 3, 2018

Choose a reason for hiding this comment

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

It isn't. This line is in an if block.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I’m a bit limited here on my phone.

ctorflow.freeFieldinit();

Scope* enc = enclosing;
if (!nofree)
{
enclosing = freelist;
Expand Down
6 changes: 4 additions & 2 deletions src/dmd/expressionsem.d
Original file line number Diff line number Diff line change
Expand Up @@ -8635,7 +8635,6 @@ private extern (C++) final class ExpressionSemanticVisitor : Visitor

e1x = resolveProperties(sc, e1x);
e1x = e1x.toBoolean(sc);
CSX cs1 = sc.ctorflow.callSuper;

if (sc.flags & SCOPE.condition)
{
Expand All @@ -8649,8 +8648,10 @@ private extern (C++) final class ExpressionSemanticVisitor : Visitor
}
}

CtorFlow ctorflow = sc.ctorflow.clone();
Expression e2x = exp.e2.expressionSemantic(sc);
sc.mergeCallSuper(exp.loc, cs1);
sc.merge(exp.loc, ctorflow);
ctorflow.freeFieldinit();

// for static alias this: https://issues.dlang.org/show_bug.cgi?id=17684
if (e2x.op == TOK.type)
Expand Down Expand Up @@ -9127,6 +9128,7 @@ private extern (C++) final class ExpressionSemanticVisitor : Visitor
e2x = resolveProperties(sc, e2x);

sc.merge(exp.loc, ctorflow1);
ctorflow1.freeFieldinit();

if (ec.op == TOK.error)
{
Expand Down
8 changes: 5 additions & 3 deletions src/dmd/statementsem.d
Original file line number Diff line number Diff line change
Expand Up @@ -2124,9 +2124,6 @@ else
/* https://dlang.org/spec/statement.html#IfStatement
*/

// Save 'root' of two branches (then and else)
CtorFlow ctorflow_root = sc.ctorflow.clone();

// check in syntax level
ifs.condition = checkAssignmentAsCondition(ifs.condition);

Expand Down Expand Up @@ -2181,6 +2178,9 @@ else
// This feature allows a limited form of conditional compilation.
ifs.condition = ifs.condition.optimize(WANTvalue);

// Save 'root' of two branches (then and else) at the point where it forks
CtorFlow ctorflow_root = scd.ctorflow.clone();

ifs.ifbody = ifs.ifbody.semanticNoScope(scd);
scd.pop();

Expand All @@ -2192,6 +2192,8 @@ else
// Merge 'then' results into 'else' results
sc.merge(ifs.loc, ctorflow_then);

ctorflow_then.freeFieldinit(); // free extra copy of the data

if (ifs.condition.op == TOK.error ||
(ifs.ifbody && ifs.ifbody.isErrorStatement()) ||
(ifs.elsebody && ifs.elsebody.isErrorStatement()))
Expand Down
64 changes: 64 additions & 0 deletions test/fail_compilation/test18708.d
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
/* TEST_OUTPUT:
---
fail_compilation/test18708.d(24): Error: one path skips field `s`
Copy link
Contributor

Choose a reason for hiding this comment

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

Can the missing path be included in the error message?

Copy link
Member Author

Choose a reason for hiding this comment

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

Changing error messages is beyond the scope of this PR.

fail_compilation/test18708.d(29): Error: one path skips field `s`
fail_compilation/test18708.d(34): Error: one path skips field `s`
fail_compilation/test18708.d(39): Error: one path skips field `s`
---
*/
// https://issues.dlang.org/show_bug.cgi?id=18708

struct S { int y; @disable this(); }

class C
{
S s;

this(S t)
{
if (bar(s = t)) foo(); // OK
}

this(S t, int i)
{
i || bar(s = t);
}

this(S t, int i, int j)
{
i && bar(s = t);
}

this(S t, int i, long j)
{
i ? bar(s = t) : i;
}

this(S t, int i, byte j)
{
i ? i : bar(s = t);
}
}

int bar(S s);
int foo();

/***********************************/

class E : Exception
{
this(string msg, int line = 0, int pos = 0) pure nothrow @safe
{
if (line)
super("hello");
else
super(msg);
}

this(string msg, string file, size_t line) pure nothrow @safe
{
super(msg, file, line);
}
}