Fix Issue 18505 - delete deprecation message is misleading

[JinShil]

See dlang/druntime#2104 for context

cc @schveiguy, @dkgroot, @wilzbach

[dlang-bot]

Thanks for your pull request, @JinShil!

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✓	18505	minor	delete deprecation message is misleading

[wilzbach]

Shouldn't we direct people to __delete instead of advising them to do things manually?
After all, __delete has been added to allow a smooth transition from delete...

[JinShil]

I was under the impression that __delete was to be avoided, except as a last resort. I still don't understand the rationale for adding it.

We need to revisit the motivation and rationale for deprecating delete in the first place, and follow through with that. Adding __delete shows a real lack of conviction towards that design, IMO.

[wilzbach]

I was under the impression that __delete was to be avoided, except as a last resort. I still don't understand the rationale for adding it.

Well, in general yes, but __delete was added as a drop-in replacement for delete, s.t. you can upgrade your codebase with simply:

sed "s/delete \(.*\);/__delete(\1);/" -i **/*.d

https://dlang.org/changelog/2.079.0.html#core-memory-__delete

As mentioned, I don't think recommending calling GC.free manually is a good idea, because there are many things that might screw up accidentally (see dlang/druntime#2037). Also destroy does not work for structs and arrays, while delete does.

We need to revisit the motivation and rationale for deprecating delete in the first place, and follow through with that.

You were the one who pushed the deprecation through its final stage, but AFAICT the motivation is simple: delete can't be @safe by design, it can easily be replaced by library code (as __delete shows) and it generally shouldn't be necessary to call anyhow (that's what the GC, RAII or whatever form of memory management you use is for).

Adding __delete shows a real lack of conviction towards that design, IMO.

As mentioned, __delete is intended for those few users who are still using delete. IIRC there wasn't a single use in Phobos, a few in Druntime and one in Vibe.d.
It's really rarely used, because it's (usually) not necessary. So it was decided that __ is a good warning against its use in the future, but still allows a convenient way for people to upgrade their code as they can use a symbol from druntime.

When https://dlang.org/deprecate.html#delete was written, __delete didn't exist.

---

Anyhow I didn't want to start a major discussion, I was just saying that we should either mention __delete or point the users to a document like https://dlang.org/deprecate.html#delete which explains the rationale and offers suggestion on how their code can be upgraded.
IIRC Walter doesn't like short urls, but if you want to use urls I could setup something like dep.dlang.io/delete within an hour (including auto-deploy from GitHub).

[schveiguy]

I'm fine with using __delete as a recommendation (I mention that in the bug report), but at the same time, GC.free isn't any worse of a recommendation. Really, the issues you've identified are with destroy and not GC.free, and we are always recommending destroy.

If anything, the pitfalls of destroy should be identified in the docs (if they aren't already). Or even better, fixed.

I'd really like a function that I can call (if destroy isn't that function) that does everything needed to clean up a type just before calling GC.free. With __delete we are back to the state of "if you want to clean things up properly, it's coupled with memory deallocation." The purpose of this PR was just to let the user know that destroy doesn't deallocate, therefore replacing delete with destroy isn't enough if you are expecting that.

[JinShil]

Also destroy does not work for structs and arrays

From what I can tell destroy works fine for structs and arrays. https://run.dlang.io/is/fTQL7b

[schveiguy]

@JinShil I think the issue is this: https://run.dlang.io/is/yS5hHE

In other words, destroy is not a drop-in replacement for delete.

It works on static arrays and stack-based structs, because those are value types. But you wouldn't be using delete there.

[JinShil]

@schveiguy Ok. I understand what you mean.

But it does work for heap allocated structs if you use *s. This makes sense to me and is documented here: http://ddili.org/ders/d.en/memory.html#ix_memory.destroy Perhaps we should include that in the official documentation.

About heap allocated arrays. Should destroy work there? Should we file a bug report?

In other words, destroy is not a drop-in replacement for delete.

I fully understand that, but it was my understanding that we didn't want people to be using the delete idiom any more, and users were to be encouraged to refactor their code to make it support the destroy/free idiom. Therefore, I don't think it's best to encourage the use of __delete; it's existence is simply to get us out of a bind when all else fails.

[schveiguy]

But it does work for heap allocated structs if you use *s

Right, but is that what people will use? It's a pitfall that I feel is completely non-obvious. They will call destroy(s), it will compile, the GC will eventually call the dtor and free it, so you may not even notice that destroy does absolutely nothing in this case.

We don't have the same problems with classes, though admittedly they are a different animal.

About heap allocated arrays. Should destroy work there? Should we file a bug report?

I'm unsure. I don't think it should follow pointers from inside the array elements. But I would expect destroy to call the dtor on each of the elements. I don't know the current implementation details. We don't want to start destroying things recursively in a way that isn't expected. delete's destruction goal is to just get the immediate block ready for deallocation.

it was my understanding that we didn't want people to be using the delete idiom any more

Note that there are legitimate reasons to eagerly deallocate memory. The conservative GC can cause problems in some cases when you leave behind a giant block of data. So there still is a need to free data. This was the major reason why delete was used in the past.

destroy was sold (back in the day it was called clear) as the yin of destruction to the yang of memory deallocation that was in delete. But it's also used on value types that are stack allocated. So it needs to be able to work with non-pointer entities as well.

IMO, the struct case as you say is trivial to overcome (just use *s), we should make sure it's clearly documented. It's the array case that's troublesome to me. If we could solve that, then I think the need for __delete greatly diminishes.

[JinShil]

IMO, the struct case as you say is trivial to overcome (just use *s), we should make sure it's clearly documented.

See dlang/druntime#2112

It's the array case that's troublesome to me. If we could solve that, then I think the need for __delete greatly diminishes.

I need to think this through more, I welcome suggestions.