Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Placement new doesn't call implicit copy constructor #20950

Open
TurkeyMan opened this issue Mar 4, 2025 · 31 comments
Open

Placement new doesn't call implicit copy constructor #20950

TurkeyMan opened this issue Mar 4, 2025 · 31 comments
Assignees

Comments

@TurkeyMan
Copy link
Contributor

struct Test
{
    int x, y, z;
//    this(ref Test) {}          // no explicit copy constructor (implicit exists)
    this(int x, int y, int z) {} // explicit normal constructor
}

void main()
{
    Test a;
    Test b = a;           // working: does call implicit copy constructor
    new(b) Test();        // working: does call implicit default constructor
    new(b) Test(1, 2, 3); // working: does call user constructor
    new(b) Test(a);       // ERROR: doesn't properly call the implicit copy constructor
}

> error : constructor `Test.this(int x, int y, int z)` is not callable using argument types `(Test)`

I think this is just an oversight/bug... if you uncomment the explicit copy constructor, this works as it should.
The default constructor works, but for whatever reason, the implicit constructors aren't selected properly.

@pbackus
Copy link
Contributor

pbackus commented Mar 6, 2025

Strictly speaking, there is no "implicit copy constructor" here (i.e., this is not an overload resolution failure)—but placement new should still perform copy initialization in this case.

@TurkeyMan
Copy link
Contributor Author

Well it can either perform a copy directly, or a copy constructor can be generated which will naturally be selected in all applicable constructs where a constructor may be called... I would do the latter for uniformity.

@WalterBright
Copy link
Member

This has nothing to do with placement new; the same error message happens without it:

struct Test
{
    int x, y, z;
//    this(ref Test) {}          // no explicit copy constructor (implicit exists)
    this(int x, int y, int z) {} // explicit normal constructor
}

void test()
{
    Test a;
    Test b = Test(a);
}
test.d(11): Error: constructor `test.Test.this(int x, int y, int z)` is not callable using argument types `(Test)`

The reason is the there is no implicit copy constructor in D. If there are no explicit constructors, the compiler will just do a bit copy. If there are explicit constructors, the compiler will select a constructor based on the overload rules. If there is no match, the compiler will not generate one that matches, it will just fail.

The rationale is that overloading can sometimes be baffling as to which overload is selected. (Unfortunately, the overload rules are not as simple as I originally intended.) Compounding the confusion is if the compiler is generating another unintended invisible constructor and calling none of the explicit overloads.

There is also the confusion about what kind of qualifiers would be on this invisible constructor.

So the idea is once one goes down the road of making the struct non-POD with non-trivial constructors, one needs to finish the job and be explicit about it.

@pbackus
Copy link
Contributor

pbackus commented Mar 11, 2025

I think the desired behavior here is for new(b) Test(a) to initialize b the same way it would be initialized in Test b = a;. This does not require generating a copy constructor, just as Test b = a; does not require generating a copy constructor.

Arguably, it would be inconsistent for new(b) Test(...) to sometimes call a constructor and sometimes not call a constructor. But we could say the same thing about Test b = a;—sometimes initialization with = calls a constructor, and sometimes it does not. In practice, it doesn't seem to cause much confusion.

It is worth noting that core.lifetime.emplace supports this use-case. You can write emplace(&b, a), and it will copy a into b. If our goal is to replace emplace with placement new, supporting this use-case would help with that.

@WalterBright
Copy link
Member

These should not behave differently as far as construction goes:

Test b = Test(a);
new (b) Test(a);

They look the same, and behaving differently would be quite surprising. Changing the behavior of the non-new one can also break existing code in invisible ways.

When the user creates a constructor for a type, he is saying to the compiler, "I've got this, I know what I'm doing." The compiler shouldn't generate more hidden constructors - the existence of those hidden constructors can even break the semantics of his code (yes I know about @disable, but have always thought it an ugly hack).

Consider this:

struct Test {
    Test(ref Test t, int i);
}

and the user accidentally writes:

Test b = Test(a);

when he meant to write:

Test b = Test(a, 1);

But the compiler doesn't detect that because it created an invisible Test(ref Test i), and something doesn't work right, and he goes mad trying to figure out why it is not calling the constructor he wrote for it.

D isn't for unnecessary syntax, but also it tries to avoid the risk of invisible bugs in the service of saving keystrokes.

Besides, it's not at all difficult to write a copy constructor, and it's nice to be explicit about it.

@TurkeyMan
Copy link
Contributor Author

TurkeyMan commented Mar 12, 2025

Okay, but right now, as in the code above:

Test a;
Test b = a;     // succeeds, performs copy-initialisation
new(b) Test(a); // fails, but should do exactly the same as the line above

These need to do the same thing. Whatever the resolution, both lines of code should have identical behaviour.

Likewise for this:

Test b = Test(1,2,3);     // perform a move initialisation of `b` (after user construction)
new(b) Test(Test(1,2,3)); // identical to the above line

That's the very next line of code I'll write after this issue is resolved to test the same with move construction?

Placement new should not have different behaviour. You appear to be overcomplicating it; the new expression just needs to do EXACTLY what the normal initialisation code does. You can't change any semantics, or think about anything new; just call the existing init code, whatever it does.

Test x = ...;     // <-- whatever this expression does in the existing language
new(x) Test(...); // <-- this expression should be IDENTICAL to the line above; call the same function

Ideally, the placement new should call the same code as variable initialisation; separate paths lead to bugs like this. I would try and refactor the initialisation logic into one place and both cases should call the exact same function to perform initialisation. They can't be different, the 2 expressions should be synonyms.

This must be equivalent:

// this must be identical...
Test b = void;
new(b) Test(...);

// to this
Test b = ...;

I even suggested several months ago, because I was trying to emphasise the importance of the 2 expressions being identical; I suggested in terms of implementation.

Test b = ...; // that this expression...

// could be lowered to this:
Test b = void;
new(b) Test(...);

Where placement new is the only source of init logic in the whole compiler, and all other initialisation expressions lower to a placement new expression. It's a bit of a refactor, but I think it would increase semantic sanity, and improve fragility.

@WalterBright
Copy link
Member

Once there is a constructor for a struct, it is no longer POD, and the compiler will no longer generate constructors for it. As my original example showed, the compiler was doing the same thing for:

Test b = Test(a);
new (b) Test(a);

I.e. giving the same error, because Test was not POD and so needed the user to write the necessary constructors.

AFAIK, the behavior of new is just the same as for a declaration (as far as construction goes), and I agree with you on that.

Our difference is that you wish the compiler to generate a default copy constructor for non-POD structs, and that is a mistake. (I don't remember what C++ did for that case, and if it did, it's still a mistake!)

@pbackus
Copy link
Contributor

pbackus commented Mar 12, 2025

According to the language spec:

A struct or union is Plain Old Data (POD) if it meets the following criteria:

  1. it is static, or not nested
  2. it has no postblits, copy constructors, destructors, or assignment operators
  3. it has no fields that are themselves non-POD

So, the struct in this example is POD.

@WalterBright
Copy link
Member

@pbackus you're right. I'll amend my comments with "has any user defined constructors" replacing "is POD".

@TurkeyMan
Copy link
Contributor Author

Once there is a constructor for a struct, it is no longer POD, and the compiler will no longer generate constructors for it. As my original example showed, the compiler was doing the same thing for:

Test b = Test(a);
new (b) Test(a);

I.e. giving the same error, because Test was not POD and so needed the user to write the necessary constructors.

AFAIK, the behavior of new is just the same as for a declaration (as far as construction goes), and I agree with you on that.

It's not though, did you try to reproduce this?

Paste this:

struct Test
{
    int x, y, z;
    this(int x, int y, int z) {} // explicit normal constructor
}

void main()
{
    Test a;

    Test b = a;           // working: performs a copy as expected
//    new(b) Test(a);       // ERROR: doesn't compile!
}

The initialisation compiles. Uncomment placement new, and you get a compile error.
The bottom line shouldn't give an error, it should behave identical to the live above.

Our difference is that you wish the compiler to generate a default copy constructor for non-POD structs, and that is a mistake. (I don't remember what C++ did for that case, and if it did, it's still a mistake!)

No actually, this has nothing to do with what I think the behaviour should or shouldn't be. I'm reporting that the new expression produces an error where the init statement doesn't.
They should both be identical, and we need to be able to depend on that. We can't have mismatching semantics between these 2 expressions.

@WalterBright
Copy link
Member

struct Test
{
    int x, y, z;
    this(int x, int y, int z) {} // explicit normal constructor
}

void main()
{
    Test a;

    Test b = Test(a);     //  Error: constructor `test.Test.this(int x, int y, int z)` is not callable using argument types `(Test)`
    new(b) Test(a);       //  Error: constructor `test.Test.this(int x, int y, int z)` is not callable using argument types `(Test)`
}

They work the same. What you're seeing is:

Test b = a;             // does a bit copy
Test b = Test(a);    // calls a constructor

are not the same thing.

@TurkeyMan
Copy link
Contributor Author

TurkeyMan commented Mar 12, 2025

I'm confused.

Test b = a;     // initialise `Test` with the lvalue `a`
new(b) Test(a); // <-- equivalent expression; init `Test` from the lvalue `a`

These expressions should be identical; initialise Test from the lvalue a.

The rewrite you show isn't right, you're showing 2 different things side-by-side; let me fix it:

    Test b = Test(a);     // initialise given a `Test` rvalue
    new(b) Test(Test(a)); // <-- equivalent expression, init from a `Test` rvalue

Those 2 expressions should be identical.

So; maybe there's something wrong with how the new expression interprets its argument?

@TurkeyMan
Copy link
Contributor Author

TurkeyMan commented Mar 12, 2025

They work the same. What you're seeing is:

Test b = a;             // does a bit copy
Test b = Test(a);    // calls a constructor

are not the same thing.

Yeah okay, so that's the bug... the new expression is not interpreting the argument correctly.
In the expression new(b) Test(a), the argument is not an rvalue Test(a), it is the lvalue a.
That should result in a normal copy initialisation; whether that's a call to a copy constructor or otherwise, exactly what the expression Test b = a; does is correct. Interpreting the expression to call a constructor is not correct, because Test b = a; doesn't call a constructor in this case, as you show.

Again; the expression new(b) Test(a) should/MUST behave identical to Test b = a;; init Test given the lvalue a.

@WalterBright
Copy link
Member

In the expression new(b) Test(a), the argument is not an rvalue Test(a), it is the lvalue a.

I don't want to explain to people why the Test(a):

Test b = Test(a);
new (b) = Test(a);

are different. They'll tell me that is a bug, and I'll be sympathetic!

Just write the constructor :-/

@TurkeyMan
Copy link
Contributor Author

TurkeyMan commented Mar 12, 2025

Just write the constructor :-/

This isn't about a missing constructor, it's a semantic mismatch.

In the expression new(b) Test(a), the argument is not an rvalue Test(a), it is the lvalue a.

I don't want to explain to people why the Test(a):

Test b = Test(a);
new (b) = Test(a);

are different. They'll tell me that is a bug, and I'll be sympathetic!

No, you will have to explain the current state, because it is not reasonable.
I don't know what to make of that code block above; it doesn't make sense.

If I were to try and write some sort of pseudo code to show how it works, maybe this:

Test b = a;
new (b) Test = (a);

You've misunderstood what the token Test means in that expression; it's not invoking a constructor, it's specifying the type for b. It's the same token as written left of the b in the direct initialisation: **Test** b = a; == new(b) **Test** (a)

These must be equivalent expressions:

Test b = a;
new(b) Test(a);

Test b = Test(a);
new(b) Test(Test(a));

Test b = int(10);
new(b) Test(int(10));

Test b = ...;
new(b) Test(...);

I promise that if what I write here isn't exactly what the code does, it will immediately surprise anyone that tries to use this. Everyone who tried out placement new so far ran into this issue instantly.

@TurkeyMan
Copy link
Contributor Author

TurkeyMan commented Mar 12, 2025

You can see how in both expressions, the word Test must be written once to declare what is being constructed... it is NOT the grammar for a call to a constructor on the new expression.
If you're concerned that's confusing (it's not; C++ is like this too, everyone expects this), then change the grammar. The expressions must be equivalent as I wrote them, otherwise this is a non-starter; it's impossible to describe default initialisation the way you've interpreted it.

@WalterBright
Copy link
Member

Test(a) is a call to a constructor. If there is no constructor, it is interpreted as a struct literal. It does not generate a constructor. This is consistent throughout the language.

It's not unusual for people coming from C++ to try things the C++ way they're used to first.

@TurkeyMan
Copy link
Contributor Author

TurkeyMan commented Mar 12, 2025

Pick a different grammar then. You seem to be stuck on that point, or you still misunderstand the actual point here...

Situation:

Test b = a;

This expression initialises a variable. a in that scenario could be absolutely any valid expression. Those initialisation semantics are defined however they are defined, and that is not the issue we're discussing.

The issue is that not all variables are on the stack; it's essential that we can initialise a variable the same way given an address that may or may not be on the stack.
If we can't reliably initialise a variable, then the language is unreasonable at it's most essential foundation.

I proposed:
new(b) Test(a) as equivalent to Test b = a, but it seems something else was implemented instead.

This should be read as:

**Type** b = a;
new(b) **Type** = (a);

It is NOT a constructor call, it is the type being specified for b, as is required in both expressions.

If you don't like the fact that the grammar looks like a constructor call, then please feel free to suggest a different grammar that you don't feel exposes that weakness.

Personally, I think placement new declared this way is natural and precedented, and I think anything you could propose would violate the rule of least surprise. Everybody who's tried this so far as hit this wall instantly; they understood it to be as I say, and they were surprised when it was reinterpreted as a constructor call, because there may or may not be a constructor.

This is for "initialisation", constructors are a subset of initialisation.

@WalterBright
Copy link
Member

new Test(a) has been in D for 25 years. Placement new hasn't changed it. I haven't heard a single complaint about it. Changing what it does will break existing code.

@TurkeyMan
Copy link
Contributor Author

TurkeyMan commented Mar 12, 2025

Hmmm, I've never used new in D before, so I certainly haven't encountered the issues.

The thing is, people usually don't complain about issues; they just fiddle with work-arounds until it works, and then they move on. That's not a good thing, but it's true. If they can't find a workaround, then they might make a noise.

We have a fundamental operation; initialising a variable, and we can't express that operation, except in the one case where the variable is allocated on the stack.
This is very bad for code generalisation. Most variables aren't on the stack.

Take a look at core.lifetime, and core.internal.lifetime; you will be seeing the direct implications of this non-uniformity.
That mess of hacks is, at its heart, the direct result of this single edge-case in the language. We can not eliminate core.lifetime unless we generalise variable initialisation.

There is a truckload of brittle and broken (fails various categories of optimisation, performs redundant work, etc) code in there handling this edge case.

That rats nest is essentially trying to emulate the edge cases we're talking about here.
Look at the 5-odd implementations of emplace, look at move... they're insane. You can see the swamp of brittle and broken attempts to match the semantics of Ty x = y.

Code like static if (__traits(compiles, [test if can call a constructor])) and then forward to a constructor, else get the init value, and implement some calls to memcpy, etc, among many other niche cases.
It is not a reasonable thing to implement in the library, and none of it precisely matches the semantics of Ty x = y, which is implemented in the compiler and there is no way to express.

And then you say "I can't change anything, because everything breaks", and the reality is that logic like that in core.lifetime is so amazingly brittle, that of course that's the outcome! So, are we satisfied with this, or do we move on a trajectory to actually fix it?

One way ore another, we must have the tool to initialise a variable. That shouldn't be subject to debate.
If we have that tool, then we can delete core.lifetime and all the problems that stem from that rats nest. Without that tool, we get core.lifetime, which attempts to emulate Ty x = y in the library, which is basically impossible, because it's too low level.

@TurkeyMan
Copy link
Contributor Author

You know, I reckon there's actually a decent chance that it wouldn't break new with this initialisation fix though; because the issue is that non-constructor based initialisation doesn't compile...

Can we try it and see the scale of breakage? If there was ever an issue in the history of D, this issue is worthy of a breaking change.

Or pick another grammar, and we start over with a new expression.

@Herringway
Copy link
Contributor

This bickering over basic semantics suggests that placement new was merged far too prematurely. Perhaps a DIP would have helped avoid this?

@TurkeyMan
Copy link
Contributor Author

TurkeyMan commented Mar 12, 2025

Super helpful contribution.

I don't believe process would have caught this; the words on the DIP would have been the same words as we've been using to describe this work the whole time.
I've said that it should be semantically identical to Ty x = y probably hundreds of times, with no rejection of that wording, and what I perceived as agreement on that. These are things that only occur when you actually do the thing. The only difference a DIP would have made is that we'd be having this conversation in 3 years time... or not at all.
This is progress, and this is necessary.

@WalterBright
Copy link
Member

This bickering over basic semantics suggests that placement new was merged far too prematurely.

Placement new is not the issue. The issue is the NewExpression syntax, which dates back 25 years.

https://dlang.org/spec/expression.html#new_expressions

@TurkeyMan
Copy link
Contributor Author

TurkeyMan commented Mar 12, 2025

Yeah, I get that now... so, we either deviate from that spec for placement new, or we leave placement new how it is, and also add something beside it with different grammar?
Are you sure that adjusting that spec is a breaking change? I don't think it would change existing code, it would accept new forms that don't currently compile, so it shouldn't be a breaking change in that sense...

@TurkeyMan
Copy link
Contributor Author

TurkeyMan commented Mar 12, 2025

Can we find an expression like new(b) Ty = a, or something that gives that same vibe that's not grammatically ambiguous with an assignment operator?
How does the language distinguish:

b = a;    // an assignment, distinct from:
Ty b = a; // an initialisation

Can

new(b) = a;

be similarly interpreted as an initialisation rather than an assignment?
It would imply that new(b) (with no constructor expression) be interpreted the same as a variable declaration when placed to the left of an = operator...
So:

Ty a
new(a)

are semantically equivalent; such that you can write:

Ty a = b;
new(a) = b;

And they are equivalent, both recipients of an initialisation.

@TurkeyMan
Copy link
Contributor Author

I still feel quite strongly that new should be fixed to be uniform, and then all initialisation expressions in the language are uniform. We don't need more edge cases, and correcting existing edge cases is always a good move if it can be done without tremendous disruption...

@pbackus
Copy link
Contributor

pbackus commented Mar 12, 2025

Personally I don't see anything wrong with allowing both types of new expression to perform copy-initialization without a constructor. E.g.,

struct S { int x; }
S original;
S* heapCopy = new S(original);
S stackCopy = void;
new(stackCopy) S(original);

This has the advantage that D programmers wouldn't have to learn two separate sets of rules for stack and heap initialization—both S s = initializer; and new S(initializer) would treat initializer the same way.

This kind of uniformity is also very useful for writing generic code, where one might want to use the same initializer for either a stack or heap allocation depending on some compile-time condition.

Given that this code currently does not compile, it should be a purely additive change, with minimal risk of breakage.

@pbackus
Copy link
Contributor

pbackus commented Mar 12, 2025

In fact, here is an enhancement request from 2014(!) for allowing new to copy structs without constructors:

https://issues.dlang.org/show_bug.cgi?id=12918

@TurkeyMan
Copy link
Contributor Author

Nice find!

@WalterBright
Copy link
Member

WalterBright commented Mar 13, 2025

The bug report 12918 doesn't really offer a solution, and doesn't address the problems I listed.

But, since this issue has nothing to do with placement new, and the current behavior has been specified and in existence since the Dawn of Man, I suggest a proposal to address it be put in the d.d.ideas newsgroup and gather more input from the general community.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants