From 80619240a71ea68c7d4733f5010263b8a04a46de Mon Sep 17 00:00:00 2001 From: David Nadlinger Date: Sun, 12 Jan 2014 20:16:42 +0100 Subject: [PATCH 1/5] Issue 11503 - immutable return value from pure function implicitly converts to mutable. --- src/cast.c | 2 +- test/fail_compilation/fail11503a.d | 21 +++++++++++++++++++++ test/fail_compilation/fail11503b.d | 13 +++++++++++++ 3 files changed, 35 insertions(+), 1 deletion(-) create mode 100644 test/fail_compilation/fail11503a.d create mode 100644 test/fail_compilation/fail11503b.d diff --git a/src/cast.c b/src/cast.c index 7208fc11901c..eca167a653cb 100644 --- a/src/cast.c +++ b/src/cast.c @@ -618,7 +618,7 @@ Expression *CallExp::implicitCastTo(Scope *sc, Type *t) * convert to immutable */ if (f && f->isolateReturn() && - type->immutableOf()->equals(t->immutableOf())) + type->immutableOf()->equals(t)) { /* Avoid emitting CastExp for: * T[] make() pure { ... } diff --git a/test/fail_compilation/fail11503a.d b/test/fail_compilation/fail11503a.d new file mode 100644 index 000000000000..28f7befdf91b --- /dev/null +++ b/test/fail_compilation/fail11503a.d @@ -0,0 +1,21 @@ +struct S +{ + immutable(S)* s; + this(int) immutable pure + { + s = &this; + } + int data; +} + +immutable(S)* makes() pure +{ + return new immutable S(0); +} + +void main() +{ + S* s = makes(); // s is mutable and contains an immutable reference to itself + //s.s.data = 7; // this is immutable + s.data = 3; // but this is not!!! +} diff --git a/test/fail_compilation/fail11503b.d b/test/fail_compilation/fail11503b.d new file mode 100644 index 000000000000..80549de8e6ca --- /dev/null +++ b/test/fail_compilation/fail11503b.d @@ -0,0 +1,13 @@ +immutable int[] x = [1, 2, 3]; + +auto makes() pure +{ + return x; +} + +int main() +{ + auto a = x; + int[] b = makes(); + return b[1]; +} From 4bfca0385a442bc713e9ad9087cfd308f12d2cba Mon Sep 17 00:00:00 2001 From: David Nadlinger Date: Mon, 13 Jan 2014 00:52:42 +0100 Subject: [PATCH 2/5] Remove CallExp::implicitCastTo altogether. The pure -> immutable case is checked in implicitConvTo as well, and since it yields MATCHexact, the castTo() in Expression::implicitCastTo is a no-op anyway. --- src/cast.c | 21 --------------------- src/expression.h | 1 - 2 files changed, 22 deletions(-) diff --git a/src/cast.c b/src/cast.c index eca167a653cb..2d1ad42fe289 100644 --- a/src/cast.c +++ b/src/cast.c @@ -610,27 +610,6 @@ MATCH AssocArrayLiteralExp::implicitConvTo(Type *t) return Expression::implicitConvTo(t); } -Expression *CallExp::implicitCastTo(Scope *sc, Type *t) -{ - //printf("CallExp::implicitCastTo(%s of type %s) => %s\n", toChars(), type->toChars(), t->toChars()); - - /* Allow the result of strongly pure functions to - * convert to immutable - */ - if (f && f->isolateReturn() && - type->immutableOf()->equals(t)) - { - /* Avoid emitting CastExp for: - * T[] make() pure { ... } - * immutable T[] arr = make(); // unique return - */ - Expression *e = copy(); - e->type = t; - return e; - } - return Expression::implicitCastTo(sc, t); -} - MATCH CallExp::implicitConvTo(Type *t) { #if 0 diff --git a/src/expression.h b/src/expression.h index 5a7d2686ab0d..46a20f98847d 100644 --- a/src/expression.h +++ b/src/expression.h @@ -1103,7 +1103,6 @@ class CallExp : public UnaExp int isLvalue(); Expression *toLvalue(Scope *sc, Expression *e); Expression *addDtorHook(Scope *sc); - Expression *implicitCastTo(Scope *sc, Type *t); MATCH implicitConvTo(Type *t); Expression *doInline(InlineDoState *ids); From fa6898c3feb1c500085d73df716b9a1ce9e4afa1 Mon Sep 17 00:00:00 2001 From: David Nadlinger Date: Mon, 13 Jan 2014 03:07:07 +0100 Subject: [PATCH 3/5] Issue 11909 - Struct members and static arrays break pure function escape analysis (immutability violation). --- src/func.c | 18 +++++------------- test/fail_compilation/fail11503c.d | 15 +++++++++++++++ test/fail_compilation/fail11503d.d | 22 ++++++++++++++++++++++ 3 files changed, 42 insertions(+), 13 deletions(-) create mode 100644 test/fail_compilation/fail11503c.d create mode 100644 test/fail_compilation/fail11503d.d diff --git a/src/func.c b/src/func.c index 711825113c85..219304c41a33 100644 --- a/src/func.c +++ b/src/func.c @@ -3295,9 +3295,11 @@ Type *getIndirection(Type *t) } /************************************** - * Traverse this and t, and then check the indirections convertibility. + * Returns true if memory reachable through a reference B to a value of type tb, + * which has been constructed with a reference A to a value of type ta + * available, can alias memory reachable from A based on the types involved + * (either directly or via any number of indirections). */ - int traverseIndirections(Type *ta, Type *tb, void *p = NULL, bool a2b = true) { if (a2b) // check ta appears in tb @@ -3305,8 +3307,6 @@ int traverseIndirections(Type *ta, Type *tb, void *p = NULL, bool a2b = true) //printf("\ttraverse(1) %s appears in %s\n", ta->toChars(), tb->toChars()); if (ta->constConv(tb)) return 1; - else if (ta->immutableOf()->equals(tb->immutableOf())) - return 0; else if (tb->ty == Tvoid && MODimplicitConv(ta->mod, tb->mod)) return 1; } @@ -3315,8 +3315,6 @@ int traverseIndirections(Type *ta, Type *tb, void *p = NULL, bool a2b = true) //printf("\ttraverse(2) %s appears in %s\n", tb->toChars(), ta->toChars()); if (tb->constConv(ta)) return 1; - else if (tb->immutableOf()->equals(ta->immutableOf())) - return 0; else if (ta->ty == Tvoid && MODimplicitConv(tb->mod, ta->mod)) return 1; } @@ -3329,11 +3327,10 @@ int traverseIndirections(Type *ta, Type *tb, void *p = NULL, bool a2b = true) }; Ctxt *ctxt = (Ctxt *)p; - Type *tbb = tb->toBasetype(); + Type *tbb = tb->toBasetype()->baseElemOf(); if (tbb != tb) return traverseIndirections(ta, tbb, ctxt, a2b); - tb = tb->baseElemOf(); if (tb->ty == Tclass || tb->ty == Tstruct) { for (Ctxt *c = ctxt; c; c = c->prev) @@ -3347,11 +3344,6 @@ int traverseIndirections(Type *ta, Type *tb, void *p = NULL, bool a2b = true) { VarDeclaration *v = sym->fields[i]; Type *tprmi = v->type->addMod(tb->mod); - if (!(v->storage_class & STCref)) - tprmi = getIndirection(tprmi); - if (!tprmi) - continue; - //printf("\ttb = %s, tprmi = %s\n", tb->toChars(), tprmi->toChars()); if (traverseIndirections(ta, tprmi, &c, a2b)) return 1; diff --git a/test/fail_compilation/fail11503c.d b/test/fail_compilation/fail11503c.d new file mode 100644 index 000000000000..dc45eefc0939 --- /dev/null +++ b/test/fail_compilation/fail11503c.d @@ -0,0 +1,15 @@ +struct Data +{ + char[256] buffer; + @property const(char)[] filename() const pure nothrow + { + return buffer[]; + } +} + +void main() +{ + Data d; + string f = d.filename; + d.buffer[0] = 'a'; +} diff --git a/test/fail_compilation/fail11503d.d b/test/fail_compilation/fail11503d.d new file mode 100644 index 000000000000..d96e2a8b249a --- /dev/null +++ b/test/fail_compilation/fail11503d.d @@ -0,0 +1,22 @@ +struct Data2 +{ + char buffer; +} + +@property const(char)[] filename(const ref Data2 d) pure nothrow +{ + return (&d.buffer)[0 .. 1]; +} + +@property const(char)[] filename2(const Data2* d) pure nothrow +{ + return (&d.buffer)[0 .. 1]; +} + +void main() +{ + Data2 d; + string f = d.filename; + string g = (&d).filename2; + d.buffer = 'a'; +} From 8160871629cb7b3c5261a9048a49cd6261ca433b Mon Sep 17 00:00:00 2001 From: David Nadlinger Date: Mon, 13 Jan 2014 05:15:19 +0100 Subject: [PATCH 4/5] Clarify traverseIndirections implementation. No functional change intended. --- src/func.c | 49 ++++++++++++++++++++++++++----------------------- 1 file changed, 26 insertions(+), 23 deletions(-) diff --git a/src/func.c b/src/func.c index 219304c41a33..1f4a69977896 100644 --- a/src/func.c +++ b/src/func.c @@ -3299,26 +3299,31 @@ Type *getIndirection(Type *t) * which has been constructed with a reference A to a value of type ta * available, can alias memory reachable from A based on the types involved * (either directly or via any number of indirections). + * + * Note that this relation is not symmetric in the two arguments. For example, + * a const(int) reference can point to a pre-existing int, but not the other + * way round. */ -int traverseIndirections(Type *ta, Type *tb, void *p = NULL, bool a2b = true) +int traverseIndirections(Type *ta, Type *tb, void *p = NULL, bool reversePass = false) { - if (a2b) // check ta appears in tb + Type *source = ta; + Type *target = tb; + if (reversePass) { - //printf("\ttraverse(1) %s appears in %s\n", ta->toChars(), tb->toChars()); - if (ta->constConv(tb)) - return 1; - else if (tb->ty == Tvoid && MODimplicitConv(ta->mod, tb->mod)) - return 1; - } - else // check tb appears in ta - { - //printf("\ttraverse(2) %s appears in %s\n", tb->toChars(), ta->toChars()); - if (tb->constConv(ta)) - return 1; - else if (ta->ty == Tvoid && MODimplicitConv(tb->mod, ta->mod)) - return 1; + source = tb; + target = ta; } + if (source->constConv(target)) + return 1; + else if (target->ty == Tvoid && MODimplicitConv(source->mod, target->mod)) + return 1; + + // No direct match, so try breaking up one of the types (starting with tb). + Type *tbb = tb->toBasetype()->baseElemOf(); + if (tbb != tb) + return traverseIndirections(ta, tbb, p, reversePass); + // context date to detect circular look up struct Ctxt { @@ -3327,10 +3332,6 @@ int traverseIndirections(Type *ta, Type *tb, void *p = NULL, bool a2b = true) }; Ctxt *ctxt = (Ctxt *)p; - Type *tbb = tb->toBasetype()->baseElemOf(); - if (tbb != tb) - return traverseIndirections(ta, tbb, ctxt, a2b); - if (tb->ty == Tclass || tb->ty == Tstruct) { for (Ctxt *c = ctxt; c; c = c->prev) @@ -3345,14 +3346,14 @@ int traverseIndirections(Type *ta, Type *tb, void *p = NULL, bool a2b = true) VarDeclaration *v = sym->fields[i]; Type *tprmi = v->type->addMod(tb->mod); //printf("\ttb = %s, tprmi = %s\n", tb->toChars(), tprmi->toChars()); - if (traverseIndirections(ta, tprmi, &c, a2b)) + if (traverseIndirections(ta, tprmi, &c, reversePass)) return 1; } } else if (tb->ty == Tarray || tb->ty == Taarray || tb->ty == Tpointer) { Type *tind = tb->nextOf(); - if (traverseIndirections(ta, tind, ctxt, a2b)) + if (traverseIndirections(ta, tind, ctxt, reversePass)) return 1; } else if (tb->hasPointers()) @@ -3360,8 +3361,10 @@ int traverseIndirections(Type *ta, Type *tb, void *p = NULL, bool a2b = true) // FIXME: function pointer/delegate types should be considered. return 1; } - if (a2b) - return traverseIndirections(tb, ta, ctxt, false); + + // Still no match, so try breaking up ta if we have note done so yet. + if (!reversePass) + return traverseIndirections(tb, ta, ctxt, true); return 0; } From 855e03a3d6aba6c45eb46f6e99f5131f980b8a5c Mon Sep 17 00:00:00 2001 From: David Nadlinger Date: Mon, 13 Jan 2014 05:16:34 +0100 Subject: [PATCH 5/5] Make traverseIndirections() return bool instead of int. --- src/func.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/func.c b/src/func.c index 1f4a69977896..999db461c5e9 100644 --- a/src/func.c +++ b/src/func.c @@ -3304,7 +3304,7 @@ Type *getIndirection(Type *t) * a const(int) reference can point to a pre-existing int, but not the other * way round. */ -int traverseIndirections(Type *ta, Type *tb, void *p = NULL, bool reversePass = false) +bool traverseIndirections(Type *ta, Type *tb, void *p = NULL, bool reversePass = false) { Type *source = ta; Type *target = tb; @@ -3315,9 +3315,9 @@ int traverseIndirections(Type *ta, Type *tb, void *p = NULL, bool reversePass = } if (source->constConv(target)) - return 1; + return true; else if (target->ty == Tvoid && MODimplicitConv(source->mod, target->mod)) - return 1; + return true; // No direct match, so try breaking up one of the types (starting with tb). Type *tbb = tb->toBasetype()->baseElemOf(); @@ -3335,7 +3335,7 @@ int traverseIndirections(Type *ta, Type *tb, void *p = NULL, bool reversePass = if (tb->ty == Tclass || tb->ty == Tstruct) { for (Ctxt *c = ctxt; c; c = c->prev) - if (tb == c->type) return 0; + if (tb == c->type) return false; Ctxt c; c.prev = ctxt; c.type = tb; @@ -3347,26 +3347,26 @@ int traverseIndirections(Type *ta, Type *tb, void *p = NULL, bool reversePass = Type *tprmi = v->type->addMod(tb->mod); //printf("\ttb = %s, tprmi = %s\n", tb->toChars(), tprmi->toChars()); if (traverseIndirections(ta, tprmi, &c, reversePass)) - return 1; + return true; } } else if (tb->ty == Tarray || tb->ty == Taarray || tb->ty == Tpointer) { Type *tind = tb->nextOf(); if (traverseIndirections(ta, tind, ctxt, reversePass)) - return 1; + return true; } else if (tb->hasPointers()) { // FIXME: function pointer/delegate types should be considered. - return 1; + return true; } // Still no match, so try breaking up ta if we have note done so yet. if (!reversePass) return traverseIndirections(tb, ta, ctxt, true); - return 0; + return false; } /********************************************