Fix Issue 23932 - Slot is allocated before evaluating the value during associative array initialization

[RazvanN7]

It seems that this behavior was on purpose: avoiding to create a temporary when an aa value is initialized, whereas creating it when an aa value is assigned. However, that does not make any sense in my opinion, because initialization is not properly defined for an aa elem. Currently, the compiler considers that an AA is initialized only when it encounters the first use of the aa variable (be it an element initialization or an aa initializer), it doesn't actually check if a particular element is initialized. Therefore, this PR creates temporaries for both the key and the value for both the initialization and the assignment case.

I had to modify a runnable test since now you will have an extra destructor call for the copy. This might be viewed as a breaking change for code that relies on it. However, this was buggy behavior and I expect no code to actually rely on it (famous last words).

[dlang-bot]

Thanks for your pull request and interest in making D better, @RazvanN7! We are looking forward to reviewing it, and you should be hearing from a maintainer soon. Please verify that your PR follows this checklist:

• My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
• My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
• I have provided a detailed rationale explaining my changes
• New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

---

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✓	23932	normal	Slot is allocated before evaluating the value during associative array initialization

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 + dmd#15264"

[CyberShadow]

I had to modify a runnable test since now you will have an extra destructor call for the copy.

Question, why copy and not move? Would moving not avoid the destructor call of the temporary?

Looking at the issue linked in the test, I'm hoping this won't affect the underlying use case there (ref-counted structs in AAs according to the author).

[CyberShadow]

Yes, unfortunately this introduces a regression. Adjusted test case from 14321:

import std.stdio;

struct Foo {
    int id;
    this(int id) {
        this.id = id;
        writeln("CTor");
    }

    this(this) {
        writeln("Postblit ", id);
    }

    ~this() {
        writeln("DTor ", id);
    }
}

void main() {
    Foo[string] foos;
    foos["test"] = Foo(42);

    writeln("end of main");
}

On master:

CTor
end of main
DTor42

On master + this PR:

CTor
DTor 42
end of main
DTor 42

So this destroys a populated Foo twice. For a reference-counted struct, it would cause the reference count to be 0 inside main and become negative after the second destruction.

I think it would be OK if the value was moved or swapped, at least then the destructor will be called with the .init version, which I think should be OK.

[RazvanN7]

Hmm, it's weird that the postblit is not called. I'll have to investigate further.

[RazvanN7]

The above code should create a temporary (ctor call), assign it the aa elem (postblit call) and then destroy both the temporary and the aa elem. The fundamental issue here seems to be the fact that the posblit is elided.

[RazvanN7]

Question, why copy and not move? Would moving not avoid the destructor call of the temporary?

If the rhs is an rvalue, the value is moved, but the temporary still needs to be destroyed. A potential optimization would be to move the constructed value twice (once in the temporary and then to the aa elem) and avoid a destructor call. This should work, but it feels like a hack.

[CyberShadow]

Bad news, this also causes a regression:

import std.stdio;

struct Foo {
    int id;
    // this(int id) {
    //     this.id = id;
    //     writeln("CTor ", id);
    // }

    this(this) {
        writeln("Postblit ", id);
    }

    ~this() {
        writeln("DTor ", id);
    }
}

void main() {
    Foo[string] foos;
    foos["test"] = Foo(1);
    // foos["test"] = Foo(2);

    writeln("end of main");
}

Output:

=== master ===
end of main
DTor 1
=== master+dmd#15264 ===
DTor 1
end of main
DTor 1

With foos["test"] = Foo(2); uncommented:

=== master ===
DTor 1
end of main
DTor 2
=== master+dmd#15264 ===
DTor 1
DTor 1
end of main
DTor 2

Looks like the first value to be assigned is getting destroyed twice.

[RazvanN7]

I'm gonna have a look at it tomorrow. I've wasted too much time on this.

[CyberShadow]

Thank you. If anything, it seems iffy to me that aa[key] = value is an initialization of aa. I would expect that to maybe get lowered to either aa = null; aa[key]=value; or aa = [key:value]; if aa needs initialization, either of which I think should already have sensible copy/move semantics.