Bug 17635 – [REG 2.066.0] cannot convert unique immutable(int)** to immutable

Lifted the forum: http://forum.dlang.org/post/[email protected] This code should be accepted: ---- alias T = immutable int; T** f(const T** input) pure { T** output; return output; } void main() { T i; T* p = &i; immutable T** r = f(&p); /* Error: cannot implicitly convert expression f(& p) of type immutable(int)** to immutable(int**) */ } ---- I can't say that I'm 100% sure about this. Maybe there's a good reason why the code must be rejected. But I don't see it. And if I read the spec right, it says that the code should compile. Per the spec [1], `f` is a "pure factory function". That means the compiler "may assume that all mutable memory returned by the call [...] is newly allocated by the function". As far as I understand, the spec also calls this "unique". The spec also says [2]: "An expression may be converted from mutable [...] to immutable if the expression is unique and all expressions it transitively refers to are either unique or immutable." Since all parts of `f`'s return value are either unique or immutable, it should be convertible to immutable. The code compiles with 2.065.0. Fails since 2.066.0. [1] https://dlang.org/spec/function.html#pure-functions [2] https://dlang.org/spec/const3.html#implicit_qualifier_conversions

(In reply to ag0aep6g from comment #0) > The code compiles with 2.065.0. Fails since 2.066.0. Broken by https://github.com/dlang/dmd/pull/3085

I found one of the problems in getIndirection() in func.d: extern (C++) Type getIndirection(Type t) { t = t.baseElemOf(); if (t.ty == Tarray || t.ty == Tpointer) return t.nextOf().toBasetype(); if (t.ty == Taarray || t.ty == Tclass) return t; if (t.ty == Tstruct) return t.hasPointers() ? t : null; // TODO // should consider TypeDelegate? return null; } https://github.com/dlang/dmd/blob/master/src/ddmd/func.d#L2459 The TODO means we're one level of indirection off, meaning all the rest of the code that relies on getIndirection() is botched with structs, including traverseIndirections(), which simply cannot work right with this error.

Bug introduced by: https://github.com/dlang/dmd/commit/f3b5817a3542f4fa4eb4a6e70658854e0d8e4aa3#diff-43282ebf5a2de5fdbcb3b5083ddf949dR3127

After a quick glance, I'm not sure whether this is indeed what is at fault here. If it was indeed getIndirection() that was at fault, shouldn't it then also be used to decompose aggregate members in traverseIndirections? In other words, wouldn't returning struct { T** } (or struct{ struct { T** } }, etc.) instead of T** still falsely trip. If I were to guess, I would say that traverseIndirections() is missing a condition that returns false if the constness of the outermost layers is not compatible instead of continuing to decompose the pointer types. That is, T* and const(T*) should be regarded as a definite non-match, irrespective of what T is. It seems like the previous `ta->immutableOf()->equals(tb->immutableOf())` check did that, but with false positives.

(In reply to David Nadlinger from comment #5) > After a quick glance, I'm not sure whether this is indeed what is at fault > here. If it was indeed getIndirection() that was at fault, shouldn't it then > also be used to decompose aggregate members in traverseIndirections? Looking at my old changes, that used to indeed be the case, before I removed the corresponding check in https://github.com/dlang/dmd/commit/fa6898c3feb1c500085d73df716b9a1ce9e4afa1. It seems like my fix was indeed suboptimal/wrong, in that it left the code in an overly conservative state, and I suppose it might be easier to start from Kenji's original code again than from what I did.

The trouble with the getIndirection() is that passing a type: int* and: struct S { int* p; } behave differently. The first passes 'int' to traverseIndirections(), losing the *, the second passes 'S', keeping the *. There's just no way to make that work consistently. Kenji had papered it over, which created the bug ag0ep6g found. But when I fixed that one, then this one (from testInference.d) started passing when it should fail: struct S2 { const(int)* ptr; } immutable(S2) foo2b(const int*[] prm) pure { S2 s; return s; } I can't make both work because of the getIndirection() inconsistency.

Commits pushed to master at https://github.com/dlang/dmd https://github.com/dlang/dmd/commit/983a02eea143b1e314ad24493cc5691a3b143667 fix Issue 17635 - [REG 2.066.0] cannot convert unique immutable(int)** to immutable https://github.com/dlang/dmd/commit/0615ca3302da7393067a6e0c9d8ca61ab65062a6 Merge pull request #7179 from WalterBright/fix17635 fix Issue 17635 - [REG 2.066.0] cannot convert unique immutable(int)*… merged-on-behalf-of: Andrei Alexandrescu <[email protected]>

The original test case has been fixed, but it can be made to fail again by changing the parameter type slightly. As far as I can tell, the parameter type shouldn't matter, as long as it's not mutable. ---- alias T = immutable int; T** f(I)(const I input) pure { T** output; return output; } void main() { /* Not regressions in 2.066 (i.e. 2.065 also rejected these): */ T*** a1; immutable T** r1 = f(a1); static struct S2 { T* foo; } S2* a2; immutable T** r2 = f(a2); /* But this one compiles with 2.065, so it's another regression: */ static struct S3 { T foo; } S3* a3; immutable T** r3 = f(a3); } ---- Tested with DMD64 D Compiler v2.076.1-b1-166-g2f98e66e5. Reopening. Let me know if I should file a new issue (or multiple ones) instead.

(In reply to ag0aep6g from comment #9) > The original test case has been fixed, but it can be made to fail again by > changing the parameter type slightly. As far as I can tell, the parameter > type shouldn't matter, as long as it's not mutable. > > ---- > alias T = immutable int; > > T** f(I)(const I input) pure > { > T** output; > return output; > } > > void main() > { > /* Not regressions in 2.066 (i.e. 2.065 also rejected these): */ > > T*** a1; > immutable T** r1 = f(a1); > > static struct S2 { T* foo; } > S2* a2; > immutable T** r2 = f(a2); > > /* But this one compiles with 2.065, so it's another regression: */ > > static struct S3 { T foo; } > S3* a3; > immutable T** r3 = f(a3); > } > ---- > > Tested with DMD64 D Compiler v2.076.1-b1-166-g2f98e66e5. > > Reopening. Let me know if I should file a new issue (or multiple ones) > instead. Yes, the procedure is that every bug report is associated with one test case. The test case for this specific bug has been fixed. Furthermore, the proposed test case has never compiled with dmd, therefore it is not a regression. I am closing this, please file the new test case as a new bug report.

(In reply to RazvanN from comment #10) > I am closing this, please file the new test case as a new bug report. https://issues.dlang.org/show_bug.cgi?id=21660

The exact test case from this issue fails again in 2.096.0 when compiling with `-preview=dip1000`. Reopening.

@aG0aep6G created dlang/dmd pull request #12890 "fix issues 22130, 17635 - pure factory functions" fixing this issue: - fix issues 22130, 17635 - pure factory functions Issue 22130 - [REG2.080.1][DIP1000] pure factory functions stopped working Issue 17635 - [REG 2.066.0] cannot convert unique immutable(int)** to immutable Immutable parameters are not needed for strong purity. Const is enough. Mutability in the return type is an additional condition on top of strong purity when considering side effects. The test case for issue 17635 was inverted without good reason in <https://github.com/dlang/dmd/pull/8048>. Flipping again to revert that mistake. https://github.com/dlang/dmd/pull/12890

dlang/dmd pull request #12890 "fix issues 22130, 17635 - pure factory functions" was merged into master: - 8e5fc75426076f6e83dea731bc3e9f4b0935de3e by aG0aep6G: fix issues 22130, 17635 - pure factory functions Issue 22130 - [REG2.080.1][DIP1000] pure factory functions stopped working Issue 17635 - [REG 2.066.0] cannot convert unique immutable(int)** to immutable Immutable parameters are not needed for strong purity. Const is enough. Mutability in the return type is an additional condition on top of strong purity when considering side effects. The test case for issue 17635 was inverted without good reason in <https://github.com/dlang/dmd/pull/8048>. Flipping again to revert that mistake. https://github.com/dlang/dmd/pull/12890