Revert "Fix Issue 11292 - Cannot re-initialize a const field in postblit"

[ibuclaw]

Reverts #11190

Rationale for revert, a postblit does not construct new memory, as can be shown in the failing test in https://issues.dlang.org/show_bug.cgi?id=21357

[dlang-bot]

Thanks for your pull request, @ibuclaw!

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✗	11292	normal	Cannot re-initialize a const field in postblit
✓	21357	regression	[REG2.093] postblit aliases old and new struct pointers

⚠️⚠️⚠️ Warnings ⚠️⚠️⚠️

• Regression or critical bug fixes should always target the stable branch. Learn more about rebasing to stable or the D release process.

To target stable perform these two steps:

1. Rebase your branch to upstream/stable:

git rebase --onto upstream/stable upstream/master

2. Change the base branch of your PR to stable

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#11921"

[kinke]

I don't think it's that simple - technically, a normal ctor (incl. copy ctor) doesn't initialize the memory either, that's done by the implicit T.init blit, analogous to the copy blit. I.e., this fails too:

struct GrayArea
{
    BatchState low;

    this(int x)
    {
        low = low.copy;
        low.arr[0] += x;
    }
}

void main()
{
    GrayArea a = GrayArea(1);
    assert(a.low.arr[0] == 1); // fails, is 2
}

I expected the original change to circumvent immutability, at least for the first assignment, but not via a promotion from AssignExp to ConstructExp.

[ibuclaw]

I expected the original change to circumvent immutability, at least for the first assignment, but not via a promotion from AssignExp to ConstructExp.

https://github.com/dlang/dmd/blob/master/src/dmd/declaration.d#L432-L433

I think this should really be done using another method, as there seems to be some blurring of lines going on over whether an assignment is initializing or modifying.

[BorisCarvajal]

Maybe just avoid converting AssignExp to ConstructExp inside postblits.
https://github.com/dlang/dmd/blob/master/src/dmd/expressionsem.d#L8609-L8614

        if (exp.op == TOK.assign
            && exp.e1.checkModifiable(sc) == Modifiable.initialization
            && !sc.func.isPostBlitDeclaration())  // use AssignExp in postblits
        {
            ...
            exp = new ConstructExp(exp.loc, exp.e1, exp.e2);

[ibuclaw]

Maybe just avoid converting AssignExp to ConstructExp inside postblits.
https://github.com/dlang/dmd/blob/master/src/dmd/expressionsem.d#L8609-L8614

        if (exp.op == TOK.assign
            && exp.e1.checkModifiable(sc) == Modifiable.initialization
            && !sc.func.isPostBlitDeclaration())  // use AssignExp in postblits
        {
            ...
            exp = new ConstructExp(exp.loc, exp.e1, exp.e2);

That won't do as genuine construct assignments would not have the right codegen now.

[kinke]

FWIW, I think this is good to go.

That won't do as genuine construct assignments would not have the right codegen now.

I doubt that; I guess working around non-mutability requires the ConstructExp promotion (as quickfix).

The proper way to allow a re-init of non-mutable fields in the postblit probably requires keeping it an AssignExp and implicitly casting a bit instead (probably only for the first assignment in the postblit). Copy ctors (with their own problems) are an alternative for the time being.

[ibuclaw]

I doubt that; I guess working around non-mutability requires the ConstructExp promotion (as quickfix).

@BorisCarvajal's proposed change is in the AssignExp semantic, so that would mean:

this(this) {
  SomeStruct a = makeSomeStruct();
}

Would not be a construct expression if that condition were added, though I'll ignore this as I'm digressing about matters that I have no desire about. :-)

The proper way to allow a re-init of non-mutable fields in the postblit probably requires keeping it an AssignExp and implicitly casting a bit instead (probably only for the first assignment in the postblit). Copy ctors (with their own problems) are an alternative for the time being.

My thoughts are perhaps modifyFieldVar should return a Modifiable instead of a bool, but also maybe there should be a new member added to the Modifiable enum as well, reinitialization, to distinguish between initializing in a constructor vs. postblit.

[ibuclaw]

Ping?