Skip to content
This repository was archived by the owner on Oct 12, 2022. It is now read-only.
/ druntime Public archive

Comments

Fix Issue 22107 - [scope][dip1000] Can't .dup an array of structs wit…#3509

Merged
thewilsonator merged 1 commit intodlang:masterfrom
CyberShadow:pull-20210706-151733
Jul 7, 2021
Merged

Fix Issue 22107 - [scope][dip1000] Can't .dup an array of structs wit…#3509
thewilsonator merged 1 commit intodlang:masterfrom
CyberShadow:pull-20210706-151733

Conversation

@CyberShadow
Copy link
Member

…h impure copy constructor

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @CyberShadow!

Bugzilla references

Auto-close Bugzilla Severity Description
22107 normal [scope][dip1000] Can't .dup an array of structs with impure copy constructor

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + druntime#3509"

@dlang-bot dlang-bot added the Bug Fix Include reference to corresponding bugzilla issue label Jul 6, 2021
@CyberShadow
Copy link
Member Author

CC @dkorpel

@dkorpel
Copy link
Contributor

dkorpel commented Jul 6, 2021

I thought scope would be inferred now issue 21209 is fixed.

Is dup still @safe on structs that have a @safe but a non-scope copy-constructor with this change?

@CyberShadow
Copy link
Member Author

I thought scope would be inferred now issue 21209 is fixed.

I didn't see that ... but, I can reproduce the issue even with DMD master, if it helps.

Is dup still @safe on structs that have a @safe but a non-scope copy-constructor with this change?

Sorry, not sure how to test this.

@dkorpel
Copy link
Contributor

dkorpel commented Jul 6, 2021

Sorry, not sure how to test this.

Actually, since the array is scope, its elements are never scope, so it doesn't matter. The only tricky case I can think of is:

@safe:
struct S
{
    int x;
    int* y;

    this(ref S old)
    {
        y = &old.x;
    }
}

void main()
{   
    S[] oldArr = new S[1]; 
    S[] newArr = oldArr[].dup;
    assert(newArr[0].y == &oldArr[0].x); // allowed since it's on the heap

    S[1] oldArr1; // on the stack
    S[] newArr1 = oldArr1[].dup;  // now it's not allowed
}

This is horrible code, but the first case is technically valid.

@CyberShadow
Copy link
Member Author

Well, I don't know what's going on here, then. Especially considering that the "fix" is in the CTFE helper, and none of this pertains CTFE.

@CyberShadow
Copy link
Member Author

    S[] newArr1 = oldArr1[].dup;  // now it's not allowed

And now it is allowed with the changes in this PR, which I understand is incorrect. But then why wasn't this flagged as an error somewhere else?

@dkorpel
Copy link
Contributor

dkorpel commented Jul 6, 2021

Especially considering that the "fix" is in the CTFE helper, and none of this pertains CTFE.

The if (__ctfe) branch is still taken as far as escape analysis is concerned

@dkorpel
Copy link
Contributor

dkorpel commented Jul 6, 2021

And now it is allowed with the changes in this PR, which I understand is incorrect. But then why wasn't this flagged as an error somewhere else?

I think the fact that it didn't compile is completely accidental: it didn't compile for the same reason the correct case in this PR's unittest didn't compile. My copy constructor example is actually invalid, you're not allowed to assign a pointer to a ref to a member. I think that's a manifestation of issue 20245.

@dkorpel
Copy link
Contributor

dkorpel commented Jul 6, 2021

I just noticed that this is also needed to make druntime unittests compile with dlang/dmd#12010.

@thewilsonator thewilsonator merged commit 58fea19 into dlang:master Jul 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Bug Fix Include reference to corresponding bugzilla issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants