Fix Issue 9433 - Deprecate delete by JinShil · Pull Request #7342 · dlang/dmd

JinShil · 2017-11-21T04:18:44Z

See:
https://dlang.org/deprecate.html
https://dlang.org/deprecate.html#delete
http://forum.dlang.org/post/jzldqlkxgewtxjyffekb@forum.dlang.org
http://forum.dlang.org/post/gqgnquwhmykvpafwpemr@forum.dlang.org

Spec and Documentation Update: dlang/dlang.org#1941.

There are only 5 lines of code change in this PR, and 4 of them are comments. The rest are mostly line number changes for the test suite, as many of the tests now emit a deprecation warning in addition to their other output.

dlang-bot · 2017-11-21T04:18:46Z

Thanks for your pull request, @JinShil!

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✓	9433	normal	Deprecate delete

rracariu · 2017-11-21T06:51:38Z

delete is still required for cases like https://issues.dlang.org/show_bug.cgi?id=17230

JinShil · 2017-11-25T06:57:13Z

delete is still required for cases like https://issues.dlang.org/show_bug.cgi?id=17230

That is due to a separate bug: https://issues.dlang.org/show_bug.cgi?id=17267. Once that bug is fixed, you should be able to use destroy.

wilzbach

While it was planned for 2.072, there's a real chance we might see this deprecation for 2.078 🎉

wilzbach · 2017-12-05T01:54:25Z

src/ddmd/parse.d

            e = parseUnaryExp();
+            // 1. Deprecation for 1 year
+            // 2. Error for 1 year
+            // 3. Removal, "delete" can be used for other identities


How about using a similar deprecated pattern as in Phobos?

// @@@DEPRECATED_2017-12@@@

In any case, it would be good to start using easily greppable years into the source code, s.t. these deprecations don't get forgotten.

// @@@DEPRECATED_2017-12@@@

How can I be sure that this will be merged this month? Maybe just add @@@DEPRECATED@@@

The trick is to annoy @andralex and @WalterBright daily ;-)
Okay all jokes aside, at Phobos these deprecation strings are put in a PR and this has never been a huge problem as at least for Phobos deprecation PRs are merged rather quickly. Yep, there have been cases in which a PR has been merged later, but it hasn't been a big deal whether a deprecation lasted for 23 or 24 months. On the contrary (1) with Martin's polished release two months release cycle, it's still 9 or 10 releases (compared to 3-4 releases two or three years ago) and people are now complaining that the deprecation periods are too long. And secondly (2) the obvious advantage is that someone will actually switch the deprecation to an error.
Anyhow, the deprecation cycle is rather ill-defined as of now.

OK, I added @@@DEPRECATED_2017-12@@@.

wilzbach · 2017-12-05T01:59:38Z

Btw could you also send a respective PR to dlang.org, s.t. the update of the deprecation page doesn't get lost? (Unfortunately this tends to happen too often).

JinShil · 2017-12-05T02:06:25Z

Btw could you also send a respective PR to dlang.org, s.t. the update of the deprecation page doesn't get lost?

~~I would be happy to, but I need to know what version I can expect this PR to land in.~~
dlang/dlang.org#1941 Betting on 2.078

JinShil · 2017-12-12T03:42:16Z

@WalterBright and @andralex Can I please get a ruling on this? I think time is running out for the 2.078.0 release.

wilzbach · 2017-12-12T04:06:49Z

@WalterBright and @andralex Can I please get a ruling on this? I think time is running out for the 2.078.0 release.

I had a short chat with @andralex about this last week and the only thing he was worried about was that no drop-in replacement for delete exists.

However, after looking at it for a bit, I don't see any reason why people if destroy and GC.free doesn't work for them, couldn't use _d_delclass directly:

extern (C) void _d_delclass(Object* p);

void main() {
    class B { int a; }
    B b = new B();
    b.a = 10;

    import std.stdio;
    b.writeln; // test.main.B
    _d_delclass(cast(Object*) &b);
    b.writeln; // null
}

JinShil · 2017-12-12T04:22:46Z

I had a short chat with @andralex about this last week and the only thing he was worried about was that no drop-in replacement for delete exists.

Why does a drop-in replacement need to exist if it's deprecated?

However, after looking at it for a bit, I don't see any reason why people if destroy and GC.free doesn't work for them, couldn't use _d_delclass directly:

Clever! 👍

andralex · 2017-12-12T15:05:32Z

Why does a drop-in replacement need to exist if it's deprecated?

The idea here is purely operational - someone who has a codebase using delete on their hands needs a quick way to upgrade the compiler whilst keeping the code unchanged. So we need to have a simple recipe "for legacy code using delete e; please replace with xxx(e);.

andralex · 2017-12-12T15:06:25Z

src/ddmd/parse.d

+            // 1. Deprecation for 1 year
+            // 2. Error for 1 year
+            // 3. Removal, "delete" can be used for other identities
+            deprecation("The `delete` keyword has been deprecated.  Use `object.destroy()` instead.");


destroy is not a pound-for-pound replacement for delete.

andralex · 2017-12-12T15:08:20Z

changelog/deprecate_delete.dd

+
+See the $(LINK2 $(ROOT_DIR)deprecate.html#delete, Deprecated Features) for more information.
+
+Starting with this release, using the `delete` keyword will result in a deprecation warning.  The warning is scheduled to remain for 1 year, after which it will be changed to an error.  The error is then to remain for 1 year, after which `delete` will be removed as a keyword.


The changelog should specify the replacement function.

andralex · 2017-12-12T15:09:28Z

@wilzbach _d_delclass would be suitable. It's not an easy replacement, but probably we don't want to make it too easy.

JinShil · 2017-12-12T15:18:51Z

So we need to have a simple recipe "for legacy code using delete e; please replace with xxx(e);.

How long would such a recipe need to be maintained? I understand the value of having such a recipe to make for smoother transition, but don't we eventually want to usher users away from such things and encourage them to refactor their code so they can make use of destroy?

I'll do the legwork, but I need to a more concrete roadmap.

andralex · 2017-12-12T15:38:41Z

How long would such a recipe need to be maintained? I understand the value of having such a recipe to make for smoother transition, but don't we eventually want to usher users away from such things and encourage them to refactor their code so they can make use of destroy?

There's no significant burden to having a function in the standard library that does what delete does. It will be marked as unsafe and documented as such.

andralex · 2017-12-12T17:49:02Z

@JinShil just spoke to @wilzbach, he'll write the function. Thanks to both!

JinShil · 2017-12-13T02:54:55Z

just spoke to @wilzbach, he'll write the function.

Ok. Thanks. What's our recommendation to users, then? "Prefer object.destroy(), but if that's not suitable, resort to the {insert function name} library function"?

andralex · 2017-12-13T17:45:46Z

@JinShil yes, something along these lines.

JinShil · 2017-12-13T23:16:54Z

@wilzbach I'll make changes as soon as I know what function to direct users to.

JinShil · 2017-12-28T00:33:04Z

just spoke to @wilzbach, he'll write the function.

@wilzbach, is this statement true?

wilzbach · 2017-12-29T00:50:40Z

@wilzbach, is this statement true?

Yes. I'm sorry that you missed 2.078 :/
I will get to it ASAP.

wilzbach · 2018-01-11T18:25:56Z

Yes. I'm sorry that you missed 2.078 :/
I will get to it ASAP.

Finally managed to get to it:

dlang/druntime#2037

Sorry for the long delay

JinShil · 2018-02-08T01:52:08Z

This is ready for additional scrutiny and potential merge, now that dlang/druntime#2037 is in.

jacob-carlborg · 2018-02-08T07:36:04Z

changelog/deprecate_delete.dd

+warning is scheduled to remain for 1 year, after which it will be changed to an error.  The
+error is then to remain for 1 year, after which `delete` will be removed as a keyword.
+
+As a replacement, users are encouraged to use $(LINK2 $(ROOT_DIR)phobos/object.html#.destroy),


Should it be phobos in the link?

JinShil · 2018-02-08T08:53:48Z

The links to object.destroy and core.memory.__delete don't resolve. __delete doesn't even seem to be in the docs I could use some help on that.

wilzbach

FYI: there are plans to use scheduled versions instead of scheduled date. The upcoming DIP also mentions a few other things - I am not saying that you need to apply them hear, but I think it doesn't hurt to have a look at what could be the mandatory soon.

wilzbach · 2018-02-08T08:57:34Z

changelog/deprecate_delete.dd

+
+Starting with this release, using the `delete` keyword will result in a deprecation warning.  The
+warning is scheduled to remain for 1 year, after which it will be changed to an error.  The
+error is then to remain for 1 year, after which `delete` will be removed as a keyword.


Are you aware of Jack's new deprecation DIP?
dlang/DIPs#108
One of the things that are on the list is instead of having a fixed date, a fixed version is better for users because it's important which version will stop supporting it (there might be a few delay of publishing a version, e.g. LDC or distros in general)

Yes, I've seen the DIP, but it needs to go through a review and approval process which I think will take time. Given the the fact that we don't have a well-defined deprecation process, I made something up, and that's what you see here. Feel free to suggest an alternative wording, or maybe I should just omit the schedule altogether so we don't have to commit to anything. I don't have an opinion either way.

Yes, please omit the schedule from the changelog.
In my experience, we most likely won't stick to it anyways.

wilzbach · 2018-02-08T09:02:53Z

Nothing to worry about - __delete hasn't been released yet and is only visible on the preview pages: http://dtest.dlang.io/artifact/website-f9fb5f80bc0b4127acd5d08058b245ed9a9cabb3-677d592a5d032c630664b91a89c8a0af/web/phobos-prerelease/core_memory.html#.__delete

JinShil · 2018-02-08T09:18:14Z

Nothing to worry about - __delete hasn't been released yet and is only visible on the preview pages:

What about object.destroy? That gives me a 404. Will that be resolved at publication time too?

wilzbach · 2018-02-08T09:52:57Z

From https://github.com/dlang/dlang.org/blob/master/dlang.org.ddoc#L353

The REF1 variants work around DDoc limitations and allow linking to
modules not inside packages (i.e. object.)

Argh...

-> $(REF1 destroy, object) works. I will update DRuntime and Phobos too. Good catch!

REF1 is currently only used once in entire DRuntime and Phobos, so maybe a special OBJECTREF macro would have been better? Anyhow, have a look at the only use of `REF1` to understand the motivation for this PR: https://dlang.org/phobos/std_file.html#.thisExePath Also there are three other PRs being blocked on this: - dlang/dmd#7342 - dlang/druntime#2082 - dlang/phobos#6140

wilzbach

Looks all good to me. Thanks for pushing this!

__delete has been added

dlang-bot added the Severity:Bug Fix label Nov 21, 2017

JinShil added the Review:WIP Work In Progress - not ready for review or pulling label Nov 21, 2017

JinShil removed the Review:WIP Work In Progress - not ready for review or pulling label Nov 21, 2017

JinShil added the Review:Easy Review label Nov 26, 2017

wilzbach approved these changes Dec 5, 2017

View reviewed changes

JinShil mentioned this pull request Dec 5, 2017

Fix Issue 9433 - Deprecate delete dlang/dlang.org#1941

Merged

wilzbach added this to the 2.078.0 milestone Dec 5, 2017

JinShil requested a review from WalterBright as a code owner December 6, 2017 22:46

andralex previously requested changes Dec 12, 2017

View reviewed changes

andralex reviewed Dec 12, 2017

View reviewed changes

JinShil added the Merge:Blocked label Dec 28, 2017

MartinNowak removed this from the 2.078.0 milestone Dec 29, 2017

dlang-bot added Review:Needs Rebase and removed Review:Needs Rebase labels Jan 1, 2018

wilzbach mentioned this pull request Jan 11, 2018

Add __delete for migration from delete dlang/druntime#2037

Merged

dlang-bot removed the Review:Needs Rebase label Feb 7, 2018

JinShil removed the Merge:Blocked label Feb 7, 2018

jacob-carlborg reviewed Feb 8, 2018

View reviewed changes

JinShil added this to the 2.079.0 milestone Feb 8, 2018

wilzbach reviewed Feb 8, 2018

View reviewed changes

wilzbach closed this Feb 8, 2018

wilzbach reopened this Feb 8, 2018

wilzbach mentioned this pull request Feb 8, 2018

Fix REF1 to behave analogous to the other REF macros dlang/dlang.org#2191

Merged

wilzbach approved these changes Feb 8, 2018

View reviewed changes

wilzbach added the Merge:72h no objection -> merge The PR will be merged if there are no objections raised. label Feb 8, 2018

Fix Issue 9433 - Deprecate delete

f88ab39

wilzbach added the Merge:auto-merge label Feb 12, 2018

dlang-bot merged commit 11daae8 into dlang:master Feb 12, 2018

JinShil mentioned this pull request Feb 14, 2018

Add ddoc for -i #7777

Merged

JinShil deleted the deprecate_delete branch May 28, 2018 04:10


		See the $(LINK2 $(ROOT_DIR)deprecate.html#delete, Deprecated Features) for more information.

		Starting with this release, using the `delete` keyword will result in a deprecation warning. The warning is scheduled to remain for 1 year, after which it will be changed to an error. The error is then to remain for 1 year, after which `delete` will be removed as a keyword.

Uh oh!

Comments

Conversation

JinShil commented Nov 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dlang-bot commented Nov 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bugzilla references

Uh oh!

rracariu commented Nov 21, 2017

Uh oh!

JinShil commented Nov 25, 2017

Uh oh!

wilzbach left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JinShil Dec 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilzbach commented Dec 5, 2017

Uh oh!

JinShil commented Dec 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JinShil commented Dec 12, 2017

Uh oh!

wilzbach commented Dec 12, 2017

Uh oh!

JinShil commented Dec 12, 2017

Uh oh!

andralex commented Dec 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andralex commented Dec 12, 2017

Uh oh!

JinShil commented Dec 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andralex commented Dec 12, 2017

Uh oh!

andralex commented Dec 12, 2017

Uh oh!

JinShil commented Dec 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andralex commented Dec 13, 2017

Uh oh!

JinShil commented Dec 13, 2017

Uh oh!

JinShil commented Dec 28, 2017

Uh oh!

wilzbach commented Dec 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wilzbach commented Jan 11, 2018

Uh oh!

JinShil commented Feb 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JinShil commented Feb 8, 2018

Uh oh!

wilzbach left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JinShil commented Nov 21, 2017 •

edited

Loading

dlang-bot commented Nov 21, 2017 •

edited

Loading

JinShil Dec 5, 2017 •

edited

Loading

JinShil commented Dec 5, 2017 •

edited

Loading

JinShil commented Dec 12, 2017 •

edited

Loading

JinShil commented Dec 13, 2017 •

edited

Loading

wilzbach commented Dec 29, 2017 •

edited

Loading

wilzbach Feb 8, 2018 •

edited

Loading

wilzbach commented Feb 8, 2018 •

edited

Loading