Skip to content
This repository has been archived by the owner on Oct 12, 2022. It is now read-only.

Fix issue 11725: AA.dup should return mutable AA where possible. #2838

Closed
wants to merge 1 commit into from

Conversation

quickfur
Copy link
Member

Since the original AA has been duplicated, it should be safe to mutate the copy. No need to needlessly restrict the copy to be const too.

Since the original AA has been duplicated, it should be safe to mutate
the copy. No need to needlessly restrict the copy to be const too.
@dlang-bot
Copy link
Contributor

Thanks for your pull request, @quickfur!

Bugzilla references

Auto-close Bugzilla Severity Description
11725 normal [AA] Cannot dup const AA

Testing this PR locally

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

dub fetch digger
dub run digger -- build "master + druntime#2838"

@dlang-bot dlang-bot added the Bug Fix Include reference to corresponding bugzilla issue label Oct 24, 2019
@quickfur
Copy link
Member Author

quickfur commented Oct 24, 2019

Hmm. Running into trouble with immutable int[int] here: returning a mutable copy makes it not implicitly convertible back to the original type. I looked over the implementation of array.dup and it seems there's some kind of hack that triggers DIP25 (to make the compiler infer uniqueness, and thereby implicit convertibility to immutable), but I'm not sure I understand exactly what's being done and how/whether to port it to the AA case.

Help, anybody?

// bug 11725: return mutable AA if possible
import core.internal.traits : Unconst;
static if (is(V : Unconst!V) && is(K : Unconst!K))
alias Result = Unconst!V[Unconst!K];
Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably be done for K and V independently.

Furthermore this should handle indirections by only stripping head const, e.g. const int* should be replaced with const(int)*. This also raises the question on how to handle structs with indirections:

struct S
{
    int* ptr;
}

void foo(const S[int] aa)
{
    auto copy = aa.dup();
    *(copy[0].ptr) = 2; // Can write const data
}

Copy link
Member

Choose a reason for hiding this comment

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

Technically, keys should always be const. In fact, D makes sure of that for indirections:

int[char[]] aa;
pragma(msg, typeof(aa)); // int[const(char)[]]

And the is(V : Unconst!V) should take care of the structs with indirection cases.

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right, Unconst was more elegant than I remembered.

Copy link
Member

Choose a reason for hiding this comment

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

Well, Unconst could be dumb or poorly implemented :) It's the is expression that makes the difference.

@schveiguy
Copy link
Member

there's some kind of hack that triggers DIP25

Isn't it the purity? If you accept an immutable item, and return a mutable item in a pure function, it's assumed unique.

Note that you have to accept a const parameter for this to work. So in the case of an AA with mutable values, you would have to accept it as a const parameter explicitly. This is why there are 2 overloads of T[].dup.

@quickfur
Copy link
Member Author

Does anyone want to take this PR up? I've been busy and haven't been able to work on this. Would be nice if somebody took over who better understand how this all works. :-D

@schveiguy
Copy link
Member

Sorry, also busy. Should be pretty quick though, I may do it next week if I find some time.

@Geod24
Copy link
Member

Geod24 commented Jul 9, 2022

Druntime have been merged into DMD. Please re-submit your PR to dlang/dmd repository.

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 stalled
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants