Turn array.sort and array.reverse into errors. by quickfur · Pull Request #6554 · dlang/dmd

quickfur · 2017-02-21T21:45:48Z

Continuing the deprecation process, as array.sort and array.reverse have been warnings / deprecations since before the C++ to D switchover in Aug 2015. It's high time we moved forward.

After the next release, this code can be removed altogether and we will no longer need to invoke array.sort() with the empty parentheses in order to get std.algorithm.sort.

wilzbach · 2017-02-21T21:49:32Z

I think you need to update a couple of tests:

../src/dmd -conf= -m32 -Irunnable   -odgenerated/runnable -ofgenerated/runnable/test20_0 runnable/test20.d
runnable/test20.d(734): Error: use std.algorithm.reverse instead of .reverse property
runnable/test20.d(739): Error: use std.algorithm.reverse instead of .reverse property
runnable/test20.d(753): Error: use std.algorithm.reverse instead of .reverse property
runnable/test20.d(759): Error: use std.algorithm.reverse instead of .reverse property


==============================
Test failed: expected rc == 0, exited with rc == 1

UplinkCoder · 2017-02-21T21:52:39Z

If we want go forward with this the tests have to be removed.
dmd tests shall not import phobos

quickfur · 2017-02-21T21:54:24Z

Oops, forgot to run the tests before submitting the PR :D Will do that right now.

quickfur · 2017-02-21T21:57:47Z

Hmm. I'm not sure it's a good idea to just remove these tests. Should they be moved to phobos instead?

wilzbach · 2017-02-21T22:06:36Z

Hmm. I'm not sure it's a good idea to just remove these tests. Should they be moved to phobos instead?

Good idea! (assuming that Phobos reverse behaves equivalently in these cases )

quickfur · 2017-02-21T22:06:45Z

Hmm. The dmd testsuite has stress tests that use .reverse. What should be done in this case? I can't move these to phobos since they're supposed to be stress testing the compiler.

DmitryOlshansky · 2017-02-21T22:09:16Z

Hmm. The dmd testsuite has stress tests that use .reverse.

What is so special about reverse? It's just a call to runtime library, any other call should do.

quickfur · 2017-02-21T22:13:32Z

The result of calling reverse is explicitly checked for.

quickfur · 2017-02-21T22:14:52Z

@wilzbach I'd say it would be a bug if Phobos reverse didn't behave the same way.

DmitryOlshansky · 2017-02-21T22:15:01Z

The result of calling reverse is explicitly checked for.

Then it doesn't check anything other then that reverse is callable as runtime function and it's result is such and such. What is compiler stress-test about it?

quickfur · 2017-02-21T22:15:40Z

@DmitryOlshansky Honestly I've no idea. I'm just trying to get the testsuite to pass. :-D

DmitryOlshansky · 2017-02-21T22:21:22Z

I'm just trying to get the testsuite to pass. :-D

The simplest way is to actually admit they are no longer applicable and delete them ;)

quickfur · 2017-02-21T22:21:47Z

Haha, OK!

quickfur · 2017-02-21T22:25:29Z

Whoa, that file (test/runnable/stress.d) has tons of copy-pasta. Is this dating from before D had templates or what?! With today's language this file could easily be condensed to 10% of its size!

DmitryOlshansky · 2017-02-21T22:28:11Z

Is this dating from before D had templates or what?!

Given the simple name I'd expect it to be as old as early D1 so no templates totally expected. Another point is that you don't want to complicate matters with templates as these have to be tested in the compiler as well.

quickfur · 2017-02-21T22:42:22Z

Bah... test/runnable/wc2.d is an entire program that depends on sort. Not sure why this is part of the dmd testsuite, and what's the point of testing this if we can't import Phobos?

quickfur · 2017-02-21T22:43:10Z

(Though I suppose I could just take out the sort call and let the output be unsorted, but it sorta begs the question of what's the point of this test in the first place.)

quickfur · 2017-02-21T22:54:58Z

Actually, test/runnable/test12.d blatantly imports std.algorithm. Maybe I should do that instead of deleting all these tests!

quickfur · 2017-02-21T23:08:39Z

test/compilable/compile1.d

    alias T3 = const ubyte[this.sizeof]; // ok <- error
 }

-/***************************************************/


I deleted this test instead of importing std.algorithm, because bug 13481 is specific to built-in attributes, and now that .sort is gone, the bug no longer applies.

quickfur · 2017-02-21T23:10:40Z

OK, updated the test suite, mostly just by importing std.algorithm. I'm not sure where the idea came from that we're not allowed to use Phobos in the test suite; I've seen at least a handful of places that explicitly imports std.algorithm.

Isn't it only in the compiler code itself that we're refraining from importing Phobos? The test suite should be exempt, right?

UplinkCoder · 2017-02-21T23:22:22Z

compiler tests should not depend on phobos since then a change in Phobos can cause dmd tests to break and that is very confusing. 2017-02-22 0:10 GMT+01:00 H. S. Teoh <notifications@github.com>:

…

OK, updated the test suite, mostly just by importing std.algorithm. I'm not sure where the idea came from that we're not allowed to use Phobos in the test suite; I've seen at least a handful of places that explicitly imports std.algorithm. Isn't it only in the compiler code itself that we're refraining from importing Phobos? The test suite should be exempt, right? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#6554 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACtpCMujXv6VoacEn2i49xC--UM3OC4Kks5re270gaJpZM4MH53-> .

quickfur · 2017-02-21T23:23:57Z

But what about the existing compiler tests that import Phobos?

UplinkCoder · 2017-02-21T23:39:11Z

those should be reevaluated sometime.

quickfur · 2017-02-21T23:59:58Z

test/runnable/foreach4.d

    assert(a.array[1] == 82);

-    foreach (uint u; &a.reverse)
+    foreach (uint u; &a.forward)


Changed this to forward instead of reverse, because it seems that the idea was to test the uint index type, but the result of the operation is ignored. Besides, Phobos reverse does not return the array, so it can't be substituted here.

WalterBright · 2017-02-26T04:35:55Z

std.algorithm.sort and .reverse needs to be checked to see if the dmd tests are covered there. If so, delete the tests in dmd. If not, add the tests to the phobos unit tests, and delete them from dmd's tests.

yebblies · 2017-02-27T17:00:11Z

But what about the existing compiler tests that import Phobos?

They should not be there, and their existence is not a reason to add more phobos imports.

quickfur · 2017-02-27T17:05:39Z

Fair enough, I'll move the tests to phobos instead.

jacob-carlborg · 2017-03-15T09:13:14Z

src/mtype.d

-            arguments.push(e);
-            e = new CallExp(e.loc, ec, arguments);
-            e.type = next.arrayOf();
+            error(e.loc, "use std.algorithm.reverse instead of .reverse property");


Since this is a unique error message and not the generic "undefined identifier". Shouldn't there be a test for that, make sure it fails correctly?

Ideally all diagnostics should have a fail test, yes.

ibuclaw · 2017-04-06T06:50:52Z

src/mtype.d

@@ -4542,103 +4542,23 @@ extern (C++) class TypeArray : TypeNext
        }
        if (ident == Id.reverse && (n.ty == Tchar || n.ty == Twchar))


Are these the only places where Id.reverse and Id.sort are used?

Perhaps add a comment to set a future date for error -> gone.

In fact, these first two branches can be removed, as the error is the same for both strings and arrays in this PR.

ibuclaw · 2017-04-06T06:54:54Z

test/runnable/test12.d

+import std.algorithm : sort;
+
 void test45()
 {


Put these imports inside the function scope.

ibuclaw · 2017-04-06T06:59:22Z

@quickfur, can you please rebase?

andralex · 2017-05-20T15:49:06Z

@MartinNowak has the time for this come?

andralex

all involved, please let's take this home thx!

Continuing the deprecation process. After the next release, this code can be removed altogether and we will no longer need to invoke array.sort() with parentheses in order to get std.algorithm.sort.

quickfur · 2017-05-23T16:41:37Z

Would somebody like to take over this PR? I currently don't have enough time to sort through the various tests in the testsuite to update them. The compiler code itself is finished, but some of the tests need to be moved to Phobos, some need to be reevaluated, etc..

quickfur · 2017-05-23T16:42:57Z

P.S. I rebased the code on the latest git HEAD so it should be easy for somebody to take over now.

andralex · 2017-05-23T16:43:24Z

cc @somzzz got time?

UplinkCoder · 2017-05-23T16:51:49Z

@andralex I can take this one :)

ibuclaw · 2017-05-23T18:04:40Z

src/ddmd/mtype.d

-            e = new CallExp(e.loc, ec, arguments);
-            e.type = next.arrayOf();
+            error(e.loc, "use std.algorithm.sort instead of .sort property");
+            goto Lerror;


These first two branches can be removed, as the error is the same for both strings and arrays in this PR.

UplinkCoder · 2017-05-24T11:16:10Z

@andralex a few a tests are failing when we turn this into errors.
They showed deprecations warnings all the time but we never check for deprecations on successful tests.
If you want the expected behavior we have to remove the property checks.

UplinkCoder · 2017-05-24T11:16:40Z

e.g. we just throw an error that the property was not found.

ibuclaw · 2018-01-10T00:31:46Z

This was done in #6827

quickfur mentioned this pull request Feb 21, 2017

Added practical example to std.algorithm.iteration.group dlang/phobos#5177

Merged

quickfur mentioned this pull request Feb 21, 2017

Remove support for deprecated built-in array.reverse property. dlang/druntime#1770

Merged

quickfur commented Feb 21, 2017

View reviewed changes

quickfur force-pushed the builtin-sort-error branch from 97551f2 to 977babe Compare February 21, 2017 23:57

quickfur commented Feb 21, 2017

View reviewed changes

wilzbach added this to the 2.074.0 milestone Mar 4, 2017

jacob-carlborg reviewed Mar 15, 2017

View reviewed changes

MartinNowak removed this from the 2.074.0 milestone Mar 20, 2017

ibuclaw requested changes Apr 6, 2017

View reviewed changes

andralex approved these changes May 23, 2017

View reviewed changes

Turn array.sort and array.reverse into errors.

1833371

Continuing the deprecation process. After the next release, this code can be removed altogether and we will no longer need to invoke array.sort() with parentheses in order to get std.algorithm.sort.

quickfur force-pushed the builtin-sort-error branch from 205d4af to 1833371 Compare May 23, 2017 16:42

ibuclaw requested changes May 23, 2017

View reviewed changes

UplinkCoder mentioned this pull request May 24, 2017

remove .sort / .reverse properties and adjust tests #6827

Merged

dlang-bot added Review:Needs Rebase Review:Needs Work Review:stalled labels Jan 1, 2018

ibuclaw closed this Jan 10, 2018

		@@ -4542,103 +4542,23 @@ extern (C++) class TypeArray : TypeNext
		}
		if (ident == Id.reverse && (n.ty == Tchar \|\| n.ty == Twchar))

Uh oh!

Conversation

quickfur commented Feb 21, 2017

Uh oh!

wilzbach commented Feb 21, 2017

Uh oh!

UplinkCoder commented Feb 21, 2017

Uh oh!

quickfur commented Feb 21, 2017

Uh oh!

quickfur commented Feb 21, 2017

Uh oh!

wilzbach commented Feb 21, 2017

Uh oh!

quickfur commented Feb 21, 2017

Uh oh!

DmitryOlshansky commented Feb 21, 2017

Uh oh!

quickfur commented Feb 21, 2017

Uh oh!

quickfur commented Feb 21, 2017

Uh oh!

DmitryOlshansky commented Feb 21, 2017

Uh oh!

quickfur commented Feb 21, 2017

Uh oh!

DmitryOlshansky commented Feb 21, 2017

Uh oh!

quickfur commented Feb 21, 2017

Uh oh!

quickfur commented Feb 21, 2017

Uh oh!

DmitryOlshansky commented Feb 21, 2017

Uh oh!

quickfur commented Feb 21, 2017

Uh oh!

quickfur commented Feb 21, 2017

Uh oh!

quickfur commented Feb 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

quickfur commented Feb 21, 2017

Uh oh!

UplinkCoder commented Feb 21, 2017 via email

Uh oh!

quickfur commented Feb 21, 2017

Uh oh!

UplinkCoder commented Feb 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WalterBright commented Feb 26, 2017

Uh oh!

yebblies commented Feb 27, 2017

Uh oh!

quickfur commented Feb 27, 2017

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ibuclaw commented Apr 6, 2017

Uh oh!

andralex commented May 20, 2017

Uh oh!

andralex left a comment

Choose a reason for hiding this comment

Uh oh!

quickfur commented May 23, 2017

Uh oh!

quickfur commented May 23, 2017

Uh oh!

andralex commented May 23, 2017