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

Jump to bottom

Fix Issue 19022 - CTorFlow: Show the line of the duplicated initialization for const/immutable fields #8399

Merged
merged 1 commit into from Jun 27, 2018
Merged

Fix Issue 19022 - CTorFlow: Show the line of the duplicated initialization for const/immutable fields #8399

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
36 changes: 19 additions & 17 deletions src/dmd/ctorflow.d
View file
Open in desktop
Expand Up @@ -17,6 +17,7 @@ module dmd.ctorflow;
import core.stdc.stdio;

import dmd.root.rmem;
import dmd.globals : Loc;

enum CSX : ushort
{
Expand All @@ -30,37 +31,33 @@ enum CSX : ushort
deprecate_18719 = 0x40, // issue deprecation for Issue 18719 - delete when deprecation period is over
}

/// Individual field in the Ctor with information about its callees and location.
struct FieldInit
{
CSX csx; /// information about the field's callees
Loc loc; /// location of the field initialization
}

/***********
* Primitive flow analysis for constructors
*/
struct CtorFlow
{
CSX callSuper; /// state of calling other constructors

CSX[] fieldinit; /// state of field initializations
FieldInit[] fieldinit; /// state of field initializations

void allocFieldinit(size_t dim)
{
fieldinit = (cast(CSX*)mem.xcalloc(CSX.sizeof, dim))[0 .. dim];
fieldinit = (cast(FieldInit*)mem.xcalloc(FieldInit.sizeof, dim))[0 .. dim];
}

void freeFieldinit()
{
if (fieldinit.ptr)
mem.xfree(fieldinit.ptr);
fieldinit = null;
}

CSX[] saveFieldInit()
{
CSX[] fi = null;
if (fieldinit.length) // copy
{
const dim = fieldinit.length;
fi = (cast(CSX*)mem.xmalloc(CSX.sizeof * dim))[0 .. dim];
fi[] = fieldinit[];
}
return fi;
fieldinit = null;
}

/***********************
Expand All @@ -70,7 +67,7 @@ struct CtorFlow
*/
CtorFlow clone()
{
return CtorFlow(callSuper, saveFieldInit());
return CtorFlow(callSuper, fieldinit.arraydup);
}

/**********************************
Expand All @@ -82,7 +79,7 @@ struct CtorFlow
{
callSuper |= csx;
foreach (ref u; fieldinit)
u |= csx;
u.csx |= csx;
}

/******************************
Expand All @@ -97,7 +94,12 @@ struct CtorFlow
{
assert(fieldinit.length == ctorflow.fieldinit.length);
foreach (i, u; ctorflow.fieldinit)
fieldinit[i] |= u;
{
auto fi = &fieldinit[i];
fi.csx |= u.csx;
if (fi.loc == Loc.init)
fi.loc = u.loc;
}
}
}
}
Expand Down
16 changes: 12 additions & 4 deletions src/dmd/declaration.d
View file
Open in desktop
Expand Up @@ -12,6 +12,7 @@

module dmd.declaration;

import core.stdc.stdio;
import dmd.aggregate;
import dmd.arraytypes;
import dmd.ctorflow;
Expand Down Expand Up @@ -112,7 +113,8 @@ bool modifyFieldVar(Loc loc, Scope* sc, VarDeclaration var, Expression e1)
break;
}
assert(i < dim);
const fi = sc.ctorflow.fieldinit[i];
auto fieldInit = &sc.ctorflow.fieldinit[i];
const fi = fieldInit.csx;

if (fi & CSX.this_ctor)
{
Expand All @@ -127,9 +129,14 @@ bool modifyFieldVar(Loc loc, Scope* sc, VarDeclaration var, Expression e1)
// dmd.ctorflow.CSX enum.
// @@@DEPRECATED_2019-01@@@.
if (fi & CSX.deprecate_18719)
.deprecation(loc, "%s field `%s` initialized multiple times", modStr, var.toChars());
{
.deprecation(loc, "%s field `%s` was initialized in a previous constructor call", modStr, var.toChars());
Copy link
Member Author

Choose a reason for hiding this comment

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

The fix for 18719 is too aggressive and only checks whether a this call is present.
Hence, there's no way to get the line number of the field with the previous initialization (there might be none after all).
See also: https://issues.dlang.org/show_bug.cgi?id=19030

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok. Perhaps an intermediate solution can be implemented. For example, printing the line number of the constructor call if there is no line number for the field.

Copy link
Member Author

Choose a reason for hiding this comment

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

The problem is the as of now we don't have this information. It currently triggers this deprecation whenever a this(...) call has been observed and a immutable field is modified. This means if there are three constructors it could be any of the two other.

}
else
{
.error(loc, "%s field `%s` initialized multiple times", modStr, var.toChars());
.errorSupplemental(fieldInit.loc, "Previous initialization is here.");
}
}
}
else if (sc.inLoop || (fi & CSX.label))
Expand All @@ -143,15 +150,16 @@ bool modifyFieldVar(Loc loc, Scope* sc, VarDeclaration var, Expression e1)
}
}

sc.ctorflow.fieldinit[i] |= CSX.this_ctor;
fieldInit.csx |= CSX.this_ctor;
fieldInit.loc = e1.loc;
if (var.overlapped) // https://issues.dlang.org/show_bug.cgi?id=15258
{
foreach (j, v; ad.fields)
{
if (v is var || !var.isOverlappedWith(v))
continue;
v.ctorinit = true;
sc.ctorflow.fieldinit[j] = CSX.this_ctor;
sc.ctorflow.fieldinit[j].csx = CSX.this_ctor;
}
}
}
Expand Down
8 changes: 6 additions & 2 deletions src/dmd/dscope.d
View file
Open in desktop
Expand Up @@ -191,7 +191,7 @@ struct Scope
}
s.slabel = null;
s.nofree = false;
s.ctorflow.fieldinit = ctorflow.saveFieldInit();
s.ctorflow.fieldinit = ctorflow.fieldinit.arraydup;
s.flags = (flags & SCOPEpush);
s.lastdc = null;
assert(&this != s);
Expand Down Expand Up @@ -290,7 +290,11 @@ struct Scope
foreach (i, v; ad.fields)
{
bool mustInit = (v.storage_class & STC.nodefaultctor || v.type.needsNested());
if (!mergeFieldInit(this.ctorflow.fieldinit[i], fies[i]) && mustInit)
auto fieldInit = &this.ctorflow.fieldinit[i];
const fiesCurrent = fies[i];
if (fieldInit.loc == Loc.init)
fieldInit.loc = fiesCurrent.loc;
if (!mergeFieldInit(this.ctorflow.fieldinit[i].csx, fiesCurrent.csx) && mustInit)
{
error(loc, "one path skips field `%s`", v.toChars());
}
Expand Down
2 changes: 1 addition & 1 deletion src/dmd/expression.d
View file
Open in desktop
Expand Up @@ -5429,7 +5429,7 @@ extern (C++) final class DotVarExp : UnaExp
{
if (f == vd)
{
if (!(sc.ctorflow.fieldinit[i] & CSX.this_ctor))
if (!(sc.ctorflow.fieldinit[i].csx & CSX.this_ctor))
{
/* If the address of vd is taken, assume it is thereby initialized
* https://issues.dlang.org/show_bug.cgi?id=15869
Expand Down
2 changes: 1 addition & 1 deletion src/dmd/expressionsem.d
View file
Open in desktop
Expand Up @@ -3290,7 +3290,7 @@ private extern (C++) final class ExpressionSemanticVisitor : Visitor
// initialized after this call.
foreach (ref field; sc.ctorflow.fieldinit)
{
field |= CSX.this_ctor | CSX.deprecate_18719;
field.csx |= CSX.this_ctor | CSX.deprecate_18719;
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/dmd/globals.d
View file
Open in desktop
Expand Up @@ -432,7 +432,7 @@ struct Loc
static immutable Loc initial; /// use for default initialization of const ref Loc's

nothrow:
extern (D) this(const(char)* filename, uint linnum, uint charnum)
extern (D) this(const(char)* filename, uint linnum, uint charnum) pure
{
this.linnum = linnum;
this.charnum = charnum;
Expand Down
70 changes: 60 additions & 10 deletions src/dmd/root/rmem.d
View file
Open in desktop
Expand Up @@ -242,18 +242,68 @@ else
}
}
}
/**
Makes a null-terminated copy of the given string on newly allocated memory.
The null-terminator won't be part of the returned string slice. It will be
at position `n` where `n` is the length of the input string.

Params:
s = string to copy

extern (D) static char[] xarraydup(const(char)[] s) nothrow
Returns: A null-terminated copy of the input array.
*/
extern (D) char[] xarraydup(const(char)[] s) nothrow
{
if (s)
{
auto p = cast(char*)mem.xmalloc(s.length + 1);
char[] a = p[0 .. s.length];
a[] = s[0 .. s.length];
p[s.length] = 0; // preserve 0 terminator semantics
return a;
}
return null;
if (!s)
return null;

auto p = cast(char*)mem.xmalloc(s.length + 1);
char[] a = p[0 .. s.length];
a[] = s[0 .. s.length];
p[s.length] = 0; // preserve 0 terminator semantics
return a;
}

///
unittest
{
auto s1 = "foo";
auto s2 = s1.xarraydup;
s2[0] = 'b';
assert(s1 == "foo");
assert(s2 == "boo");
assert(*(s2.ptr + s2.length) == '\0');
string sEmpty;
assert(sEmpty.xarraydup is null);
}

/**
Makes a copy of the given array on newly allocated memory.

Params:
s = array to copy

Returns: A copy of the input array.
*/
extern (D) T[] arraydup(T)(const scope T[] s) nothrow
{
if (!s)
return null;

const dim = s.length;
auto p = (cast(T*)mem.xmalloc(T.sizeof * dim))[0 .. dim];
p[] = s;
return p;
}

///
unittest
{
auto s1 = [0, 1, 2];
auto s2 = s1.arraydup;
s2[0] = 4;
assert(s1 == [0, 1, 2]);
assert(s2 == [4, 1, 2]);
string sEmpty;
assert(sEmpty.arraydup is null);
}
2 changes: 1 addition & 1 deletion src/dmd/semantic3.d
View file
Open in desktop
Expand Up @@ -673,7 +673,7 @@ private extern(C++) final class Semantic3Visitor : Visitor
else
{
bool mustInit = (v.storage_class & STC.nodefaultctor || v.type.needsNested());
if (mustInit && !(sc2.ctorflow.fieldinit[i] & CSX.this_ctor))
if (mustInit && !(sc2.ctorflow.fieldinit[i].csx & CSX.this_ctor))
{
funcdecl.error("field `%s` must be initialized but skipped", v.toChars());
}
Expand Down
2 changes: 1 addition & 1 deletion src/dmd/statementsem.d
View file
Open in desktop
Expand Up @@ -3236,7 +3236,7 @@ else
foreach (i, v; ad.fields)
{
bool mustInit = (v.storage_class & STC.nodefaultctor || v.type.needsNested());
if (mustInit && !(sc.ctorflow.fieldinit[i] & CSX.this_ctor))
if (mustInit && !(sc.ctorflow.fieldinit[i].csx & CSX.this_ctor))
{
rs.error("an earlier `return` statement skips field `%s` initialization", v.toChars());
errors = true;
Expand Down
8 changes: 5 additions & 3 deletions test/fail_compilation/diag12678.d
View file
Open in desktop
@@ -1,9 +1,11 @@
/*
TEST_OUTPUT:
---
fail_compilation/diag12678.d(19): Error: const field `cf1` initialized multiple times
fail_compilation/diag12678.d(22): Error: immutable field `if1` initialized multiple times
fail_compilation/diag12678.d(25): Error: const field `cf2` initialization is not allowed in loops or after labels
fail_compilation/diag12678.d(21): Error: const field `cf1` initialized multiple times
fail_compilation/diag12678.d(20): Previous initialization is here.
fail_compilation/diag12678.d(24): Error: immutable field `if1` initialized multiple times
fail_compilation/diag12678.d(23): Previous initialization is here.
fail_compilation/diag12678.d(27): Error: const field `cf2` initialization is not allowed in loops or after labels
---
*/

Expand Down
18 changes: 18 additions & 0 deletions test/fail_compilation/diag19022.d
View file
Open in desktop
@@ -0,0 +1,18 @@
/*
TEST_OUTPUT:
---
fail_compilation/diag19022.d(16): Error: immutable field `b` initialized multiple times
fail_compilation/diag19022.d(15): Previous initialization is here.
---
*/
// https://issues.dlang.org/show_bug.cgi?id=19022

struct Foo
{
immutable int b;
this(int a)
{
b = 2;
b = 2;
}
}
2 changes: 1 addition & 1 deletion test/fail_compilation/fail18719.d
View file
Open in desktop
Expand Up @@ -4,7 +4,7 @@
/*
TEST_OUTPUT:
---
fail_compilation/fail18719.d(29): Deprecation: immutable field `x` initialized multiple times
fail_compilation/fail18719.d(29): Deprecation: immutable field `x` was initialized in a previous constructor call
---
*/

Expand Down