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

Default postblit errors everywhere, should be deprecated... #20838

Open
TurkeyMan opened this issue Feb 9, 2025 · 5 comments
Open

Default postblit errors everywhere, should be deprecated... #20838

TurkeyMan opened this issue Feb 9, 2025 · 5 comments

Comments

@TurkeyMan
Copy link
Contributor

TurkeyMan commented Feb 9, 2025

Code like this:

struct MQTTClientCredentials
{
	String username;
	String password;
	Array!String whitelist;
	Array!String blacklist;
}

It's just a normal struct, but I get these errors:

mqtt\broker.d(16): Deprecation: `struct MQTTClientCredentials` implicitly-generated postblit hides copy constructor.
mqtt\broker.d(16):        The field postblit will have priority over the copy constructor.
mqtt\broker.d(16):        To change this, the postblit should be disabled for `struct MQTTClientCredentials`

String and Array have copy constructors (and recently, move constructors), postblits are old and dead.
It's frustrating that I have to write this(this) @disable; in basically every struct I write.

Can we delete this error message, and make sure the compiler never attempt to generate a default postblit?
It should only attempt to use postblit semantics if there was one explicitly implemented, and it shouldn't be implicitly generating one, that's long behind us.

It might be the case the the implicitly generated copy-constructor actually needs to call the postblit for fields that have a postblit specified...
Ie; for any fields that have a postblit, instead of doing a copy initialisation, do a blit and call postblit, only for that field?

I'd personally even go so far as a compiler option that makes postblit completely deprecated; surely it's time?

I have run into situations where copy/move semantics and postblit semantics conflict, and resolution has required some really gnarly workarounds.

@jmdavis
Copy link
Member

jmdavis commented Feb 9, 2025

Actually, right now, if you want code that works correctly in general, you use a postblit constructor and not a copy constructor. The reason for this is that dynamic arrays and associative arrays do not support copy constructors properly. TypeInfo does not support copy constructors at all, and the only druntime hooks that do are the ones that have been templated. This means that some dynamic array operations correctly support copy constructors, but some do not, and associative arrays don't support them at all.

So, while I agree that we would ideally be using copy constructors, as far as the language goes, their implementation has yet to be completed, and I would strongly advise that code in general use postblit constructors until all of the druntime hooks have been fixed; otherwise, you're going to have some subtle bugs that could be a royal pain to track down. I wasted a ton of time at work when trying to add copy constructors to some code before I figured out that they simply don't work properly with dynamic arrays and associative arrays, and I had to switch to postblit constructors. I would advise that everyone else do the same unless they know for a fact that the types in question are not going to be used with either dynamic arrays or associative arrays. IMHO, the ball was dropped big time when copy constructors were added to the language, since core language features were not updated to work with them, and they clearly weren't tested with them, or the problem would have been caught.

And I'm fairly certain that there are similar problems with move constructors too, particularly since some of the druntime hooks use memcpy, and TypeInfo certainly hasn't been updated to know anything about them.

@TurkeyMan
Copy link
Contributor Author

Hmmm. I've never used a dynamic array or an associative array, so I guess I haven't noticed that :/

So... what's going on with copy constructors if they don't work? They've been in a for a long time!
I can't really use postblit... it's insufficient for a lot of my code, and it's totally incompatible with move semantics.

Regarding move constructor issues; it's an experimental/unreleased feature, there's definitely a bunch of library work to do when the core mechanics feel solid.

@jmdavis
Copy link
Member

jmdavis commented Feb 9, 2025

So... what's going on with copy constructors if they don't work?

The basic language feature works if you're just dealing with a struct on the stack, but not all language features work with it. AFAIK, if you're not using dynamic arrays (and actually, static arrays might have the same problem, though I don't recall looking into that, so I don't know), and you're not using associative arrays, then you're probably fine. As with any language feature, it's possible that there are bugs that aren't known, but the part that I know is broken is that the druntime side of things simply was not implemented when copy constructors were added. When some of the druntime hooks were templatized, those were made to handle copy constructors in addition to postblit constructors, but TypeInfo was never updated, and any hooks that rely on it can't support copy constructors properly (which includes some of the hooks related to dynamic arrays and all of the hooks related to associative arrays). And since I'm not intimately aware with exactly which compiler features have hooks in druntime, I don't know exactly which features are even potentially broken by the druntime side of things not being updated properly. I just know for sure that dynamic arrays and associative arrays are broken with regards to copy constructors.

So, if you're just passing around structs on the stack or dealing with them on the heap directly rather than in any kind of array, and you aren't interacting with the druntime hooks anywhere, then AFAIK, you're fine. But most people writing D code are going to be using dynamic arrays and associative arrays in at least some of their code, and those are simply broken with regards to copy constructors right now. It seems like when Walter (or whoever implemented copy constructors) implemented them, he completely forgot about the druntime hooks or just never got around to them. I did bring it up in a DLF meeting a few months ago with regards to move constructors precisely because I was hoping that they wouldn't fall into the same trap, but since TypeInfo still doesn't know about move constructors, I find it highly likely that the druntime side of things has fallen through the cracks again (or best case just hasn't been dealt with yet).

In the ideal scenario here, we just templatize all of the druntime hooks and ditch TypeInfo, and then the druntime side of things will be able to see the actual types that it's dealing with at compile time and handle them appropriately instead of doing stuff like operating on void* and using TypeInfo to try to do the right thing, but the guy who was working on templatizing the hooks did so as part of SAoC, and he's been too busy to finish it since then. So, until we either update TypeInfo to know about this stuff and update all of the hooks to use those updates appropriately for both copy constructors and move constructors - or we finish templatizing all of the hooks and make sure that they all work correctly with copy constructors and move constructors - I would advise against relying on either copy constructors or move constructors working with any types that interact with druntime hooks. It probably isn't a problem at all for folks using -betterC.

They've been in a for a long time!

Honestly, that's part of why I was really pissed when I figured out that dynamic arrays and associative arrays don't work properly with them - especially since it caused me to waste a ton of time at work, because I'd wrongly assumed that they worked properly given that the feature wasn't new. The spec even currently tells folks that they should use copy constructors instead of postblit constructors (and I really should take the time to update what it says to make the situation clearer). So, it looks to me like when copy constructors were implemented, all that was really considered was structs on the stack and not all of the various language features that interact with structs, since it wouldn't have taken much testing to see that dynamic arrays and associative arrays couldn't handle them properly. And for better or worse, it's less obvious now, since they partially work with dynamic arrays thanks to some of the hooks being templatized, but clearly, when they were introduced, no such testing was done, because otherwise, it would have been obvious that the hooks had been forgotten.

I can't really use postblit... it's insufficient for a lot of my code, and it's totally incompatible with move semantics.

Well, do whatever you need to do, but anyone who's in a situation where they actually need both copy or move constructors and features that require that the druntime hooks use them appropriately is kind of screwed, because not all of the hooks are going to work with them properly. If you're not using druntime at all (and from some of what you've said previously, I suspect that you aren't), then you're probably fine. In my case though, I had to ditch copy constructors and switch to postblit constructors, much as I would have liked to use copy constructors.

@TurkeyMan
Copy link
Contributor Author

Right... well, this issue is much larger than I imagined, but not less relevant.
Obviously this situation is completely unacceptable; how can the language exist in a state where the spec which specifies appropriate copy semantics (copy construction/assignment) is completely broken?
Kinda feels like this should be a super-top-priority issue. How can anybody be expected to take D seriously if they can't even copy objects as specced?
This is another one of those things I know now, which I have to go out of my way to hide from my colleagues at all costs, just to protect my own credibility and reputation! It's not just embarrassing for D, it's embarrassing for each one of us that promote the language to others!

@TurkeyMan
Copy link
Contributor Author

@WalterBright it seems this category of issues is a general blocker to the progression of copy/move semantics. Can you work with @jmdavis and the people who know where this is at to address the outstanding issues relating to copy semantics? We can't really expect to complete the move semantics work while copy semantics are still pending.

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

2 participants