pure -> immutable fixes #3085

dnadlinger · 2014-01-12T19:26:02Z

https://d.puremagic.com/issues/show_bug.cgi?id=11503
https://d.puremagic.com/issues/show_bug.cgi?id=11909

The actual changes are in 3c0342e and ac4aed5, the rest are just cleanup commits.

tgehr · 2014-01-12T20:10:28Z

src/cast.c

@@ -616,7 +616,7 @@ Expression *CallExp::implicitCastTo(Scope *sc, Type *t)
     * convert to immutable
     */
    if (f && f->isolateReturn() &&
-        type->immutableOf()->equals(t->immutableOf()))
+        type->immutableOf()->equals(t))


Beat me to it. :o)
That said, my fix was different. Are you sure the expressed goal of 'Avoid emitting CastExp' is still adequately met by the modified code?

I now removed the code altogether, see next commit.

dnadlinger · 2014-01-13T00:34:57Z

Anybody knows what's up with Darwin_32? (Edit: There was a transitory failure, see https://d.puremagic.com/test-results/pull.ghtml?projectid=1&runid=858475.)

dnadlinger · 2014-01-13T02:50:03Z

@andralex: That's the second part I was talking about before.

dnadlinger · 2014-01-13T02:56:15Z

Note: Without the functionSemantic commit, the current Phobos build (i.e. without dlang/phobos#1852) segfaults. I didn't have time to find a reduced test case yet, but it is not related to the actual issues covered here.

andralex · 2014-01-13T02:57:21Z

Unittests lgtm. Compiler experts, please review, thanks!

dnadlinger · 2014-01-13T03:02:12Z

Regarding the tests: I find that squashing multiple failings tests into a single file using TEST_OUTPUT gets confusing rather quickly (e.g. like in fail_compilation/testInference, where similar passing and failing tests are mixed), but if the majority disagrees, I'd be happy to adapt them.

yebblies · 2014-01-13T03:45:31Z

As the person who wrote the initial code for the conversion to immutable, it's both encouraging and troubling that I no longer understand how it works!

9rnsr · 2014-01-13T03:57:24Z

To: @klickverbot

Note: Without the functionSemantic commit, the current Phobos build (i.e. without dlang/phobos#1852) segfaults.

Maybe the ICE is related to bug 11822, bug 11850, 11857? If so, it is timely fixed by #3081 in git-head.

9rnsr · 2014-01-13T04:07:24Z

To fix issue 11503, we should disable immutable -> mutable conversion on pure return, but we should continue to enable mutable -> immutable conversion at the same place. So completely removing CallExp::implicitCastTo would be wrong.

On the other hand, fixing 11503 would break currently implemented unique constructor feature (#1726) so that it relies on the possibility of immutable to mutable conversion on pure constructor. That's why I'm proposing DIP53 before fixing the bug.

dnadlinger · 2014-01-13T04:19:42Z

@9rnsr: Regarding CallExp::implicitCastTo, see the commit message – that case is handled in CallExp::implicitConvTo too.

Regarding unique constructors, all the current test cases still work. Specifically, I didn't touch the constructor conversion rules in functionResolve.

If you think that this breaks valid code, could you please provide a test case?

dnadlinger · 2014-01-13T04:19:50Z

@yebblies: Better? ;)

9rnsr · 2014-01-13T04:25:33Z

CallExp::implicitCastTo is added by my refactoring #2702. See the reason comment in the function.

        /* Avoid emitting CastExp for:
         *  T[] make() pure { ...  }
         *  immutable T[] arr = make();  // unique return
         */

dnadlinger · 2014-01-13T04:30:00Z

@9rnsr: Backed out the functionResolve change, as it indeed should be fixed by #3081. But regarding CallExp::implicitCastTo, if you don't agree with my analysis in the commit message, could you please be more specific as to why? The example from your comment doesn't produce a CastExp without the (horribly broken!) special case either.

9rnsr · 2014-01-13T04:42:35Z

@klickverbot In past, if CallExp::implicitCastTo does not exist, NRVO form detection in AssignExp::toElem was blocked by the redundant CastExp. If I remember correctly, bug 11394 test in runnable/inline.d is one of the cases.

dnadlinger · 2014-01-13T04:45:26Z

@9rnsr: Well, that test case certainly seems to pass now. ;) If there is a way that Expression::implicitConvTo can find a different match that leads to a CastExp being emitted, I must have missed it.

MartinNowak · 2014-01-18T23:10:40Z

Why the unrelated commits?

dnadlinger · 2014-01-18T23:17:15Z

@MartinNowak: They are refactorings concerning same piece of code, and were made when I worked on the issue resp. add documentation to the original code in response to review comments. Just click on the individual commits to review them one by one. The changes are well-separated, so each commit itself should be almost trivial (except for the actual fixes, of course).

I could of course submit a pull request for each, but they would be interdependent, just making life harder for everyone. Just throwing everything into a single commit with the message "Issue 11503 - immutable return value from pure function implicitly converts to mutable." would of course also be an option. ;P

…nverts to mutable.

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.

…cape analysis (immutability violation).

No functional change intended.

dnadlinger · 2014-01-25T12:44:48Z

Ping?

@MartinNowak: I separated out the one hardly related commit into #3149. If it helps review, I can also split it up even further, but using the commit view things should be fairly clear…

MartinNowak · 2014-01-25T18:36:26Z

Looks reasonable, but I didn't dig very deep into this.

pure -> immutable fixes

CyberShadow · 2014-02-15T17:23:13Z

~~This pull request introduced a regression: https://d.puremagic.com/issues/show_bug.cgi?id=12177~~

Sorry, I think that was a fluke.

CyberShadow · 2017-07-11T18:24:51Z

This pull request may have introduced a regression:
https://issues.dlang.org/show_bug.cgi?id=17635

WalterBright · 2017-10-03T20:03:04Z

This function is utterly baffling to me, especially how it swaps source and target around, swaps ta and tb around independently, arggh.

The bug in it is it falsely returns true for ta = const(immutable(int)*) and tb = immutable(int)*. I don't know if it is a detail or the whole algorithm is wrong. @klickverbot can you please help?

dnadlinger · 2017-10-03T20:27:23Z

@WalterBright: The code was originally written by Kenji – I just fixed some small bugs and made it moderately more legible. I'm having a quick look at the problem (and at cleaning up the source/target things) right now, though.

andralex · 2017-10-03T20:31:09Z

@klickverbot much appreciated!

WalterBright · 2017-10-04T01:52:38Z

@klickverbot thanks for taking a look. I wrote this: #7179 and it now passes the test suite and the regression, but I am not terribly happy with it. I'd like a more understandable solution. For one thing it needs better comments.

tgehr reviewed Jan 12, 2014
View reviewed changes

dnadlinger added 5 commits January 25, 2014 13:42

Issue 11503 - immutable return value from pure function implicitly co…

8061924

…nverts to mutable.

Remove CallExp::implicitCastTo altogether.

4bfca03

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.

Issue 11909 - Struct members and static arrays break pure function es…

fa6898c

…cape analysis (immutability violation).

Clarify traverseIndirections implementation.

8160871

No functional change intended.

Make traverseIndirections() return bool instead of int.

855e03a

dnadlinger mentioned this pull request Jan 25, 2014

[trivial] Remove tautological null check. #3149

Merged

MartinNowak added a commit that referenced this pull request Jan 26, 2014

Merge pull request #3085 from klickverbot/pure-return-immutable

01ca07b

pure -> immutable fixes

MartinNowak merged commit 01ca07b into dlang:master Jan 26, 2014

dnadlinger deleted the pure-return-immutable branch January 26, 2014 21:51

WalterBright mentioned this pull request Oct 3, 2017

fix Issue 17635 - [REG 2.066.0] cannot convert unique immutable(int)*… #7179

Merged

dnadlinger mentioned this pull request Oct 3, 2017

func: Clarify traverseIndirections() #7180

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pure -> immutable fixes #3085

pure -> immutable fixes #3085

dnadlinger commented Jan 12, 2014

tgehr Jan 12, 2014

dnadlinger Jan 13, 2014

dnadlinger commented Jan 13, 2014

dnadlinger commented Jan 13, 2014

dnadlinger commented Jan 13, 2014

andralex commented Jan 13, 2014

dnadlinger commented Jan 13, 2014

yebblies commented Jan 13, 2014

9rnsr commented Jan 13, 2014

9rnsr commented Jan 13, 2014

dnadlinger commented Jan 13, 2014

dnadlinger commented Jan 13, 2014

9rnsr commented Jan 13, 2014

dnadlinger commented Jan 13, 2014

9rnsr commented Jan 13, 2014

dnadlinger commented Jan 13, 2014

MartinNowak commented Jan 18, 2014

dnadlinger commented Jan 18, 2014

dnadlinger commented Jan 25, 2014

MartinNowak commented Jan 25, 2014

CyberShadow commented Feb 15, 2014

CyberShadow commented Jul 11, 2017

WalterBright commented Oct 3, 2017

dnadlinger commented Oct 3, 2017

andralex commented Oct 3, 2017

WalterBright commented Oct 4, 2017

pure -> immutable fixes #3085

pure -> immutable fixes #3085

Conversation

dnadlinger commented Jan 12, 2014

tgehr Jan 12, 2014

Choose a reason for hiding this comment

dnadlinger Jan 13, 2014

Choose a reason for hiding this comment

dnadlinger commented Jan 13, 2014

dnadlinger commented Jan 13, 2014

dnadlinger commented Jan 13, 2014

andralex commented Jan 13, 2014

dnadlinger commented Jan 13, 2014

yebblies commented Jan 13, 2014

9rnsr commented Jan 13, 2014

9rnsr commented Jan 13, 2014

dnadlinger commented Jan 13, 2014

dnadlinger commented Jan 13, 2014

9rnsr commented Jan 13, 2014

dnadlinger commented Jan 13, 2014

9rnsr commented Jan 13, 2014

dnadlinger commented Jan 13, 2014

MartinNowak commented Jan 18, 2014

dnadlinger commented Jan 18, 2014

dnadlinger commented Jan 25, 2014

MartinNowak commented Jan 25, 2014

CyberShadow commented Feb 15, 2014

CyberShadow commented Jul 11, 2017

WalterBright commented Oct 3, 2017

dnadlinger commented Oct 3, 2017

andralex commented Oct 3, 2017

WalterBright commented Oct 4, 2017