End the deprecation period of using the result of a comma expression. #7813

wilzbach · 2018-01-31T00:35:36Z

The deprecation period could have ended a lot earlier.
DMD deprecations should really be tracked at a common place, s.t. for every
release cycle old deprecations can be bumped accordingly.

dlang.org: dlang/dlang.org#2151

quickfur · 2018-01-31T01:10:42Z

I wonder if there's a way to use libdparse, say, to collect all deprecated symbols into a list somewhere, so that we can review which symbols need to be bumped to the next stage of deprecation.

Depending on what we find in such a list, we might even be able to automatically parse the deprecation dates and output a list of symbols that need to be updated. In the distant (or not-so-distant?) future we could even have a script/bot that automatically submits PRs to this effect. Automation FTW!

quickfur · 2018-01-31T01:11:24Z

changelog/comma-deprecation-error.dd

@@ -0,0 +1,4 @@
+The deprecation period of using the result of comma expression has ended
+
+Comma expressions have proven to be a frequent source of confusion, and bug.


Nitpick: bug > bugs.

quickfur · 2018-01-31T01:11:46Z

src/dmd/expressionsem.d

@@ -5844,7 +5844,7 @@ private extern (C++) final class ExpressionSemanticVisitor : Visitor

        e.type = e.e2.type;
        if (e.type !is Type.tvoid && !e.allowCommaExp && !e.isGenerated)
-            e.deprecation("Using the result of a comma expression is deprecated");
+            e.deprecation("The result of a comma expression cant be used");


Nitpick: cant > can't.

quickfur · 2018-01-31T01:11:54Z

src/dmd/expressionsem.d

@@ -6272,7 +6272,7 @@ private extern (C++) final class ExpressionSemanticVisitor : Visitor
            /* Rewrite to get rid of the comma from rvalue
             */
            if (!(cast(CommaExp)exp.e2).isGenerated)
-                exp.deprecation("Using the result of a comma expression is deprecated");
+                exp.error("The result of a comma expression cant be used");


Well cant is the existing DMD style:

> ag "cant" | wc -l 466 > ag "can't" | wc -l 128

Existing bad grammar should not be justification for adding more bad grammar!

I have seen @WalterBright repeatedly use "cant" - even in recent PRs. That was my motivation to use "cant".

Examples:

fix Issue 14251 - synchronized (mtx) doesn't check attributes (pure, … #6092

BetterC: disallow throw and try-catch #7307

"cant" is not a word. I believe @WalterBright made a mistake in those PRs.

Yah let's fix those. Thanks!

Fair enough, I changed it to can't.

@JinShil Actually, "cant" is a word... but it has a different meaning. :-D

@wilzbach The next time Walter submits a PR with the wrong use of "cant", somebody should point it out so that it can be fixed. :-P

quickfur · 2018-01-31T01:12:10Z

test/fail_compilation/commaexp.d

+fail_compilation/commaexp.d(38): Error: The result of a comma expression cant be used
+fail_compilation/commaexp.d(39): Error: The result of a comma expression cant be used
+fail_compilation/commaexp.d(41): Error: The result of a comma expression cant be used
+fail_compilation/commaexp.d(42): Error: The result of a comma expression cant be used


quickfur · 2018-01-31T01:12:23Z

test/fail_compilation/diag10862.d

@@ -57,7 +57,7 @@ fail_compilation/diag10862.d(77): Error: assignment cannot be used as a conditio
 fail_compilation/diag10862.d-mixin-80(80): Error: assignment cannot be used as a condition, perhaps `==` was meant?
 fail_compilation/diag10862.d-mixin-81(81): Error: assignment cannot be used as a condition, perhaps `==` was meant?
 fail_compilation/diag10862.d-mixin-82(82): Error: assignment cannot be used as a condition, perhaps `==` was meant?
-fail_compilation/diag10862.d-mixin-83(83): Deprecation: Using the result of a comma expression is deprecated
+fail_compilation/diag10862.d-mixin-83(83): Error: The result of a comma expression cant be used


quickfur · 2018-01-31T01:12:33Z

test/fail_compilation/ice10949.d

@@ -1,7 +1,7 @@
 /*
 TEST_OUTPUT:
 ---
-fail_compilation/ice10949.d(12): Deprecation: Using the result of a comma expression is deprecated
+fail_compilation/ice10949.d(12): Error: the result of a comma expression cant be used


quickfur · 2018-01-31T01:13:44Z

test/runnable/ctorpowtests.d

@@ -136,7 +136,8 @@ static assert(bazra(14)==64);

 void moreCommaTests()
 {
-   auto k = (containsAsm(), containsAsm());
+   (containsAsm(), containsAsm());
+   auto k = containsAsm();


Huh? Shouldn't this be:

containsAsm(); auto k = containsAsm();

instead? This change doesn't make sense to me.

I thought it's nice to leave the use of the comma expression in. Here we can do this as containsAsm() is a no-op.

Ah, I see why now. Nevermind, carry on. :-P

dlang-bot · 2018-01-31T01:15:14Z

Thanks for your pull request, @wilzbach!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

wilzbach · 2018-01-31T01:30:59Z

I wonder if there's a way to use libdparse, say, to collect all deprecated symbols into a list somewhere, so that we can review which symbols need to be bumped to the next stage of deprecation.

I like the idea, but it would be a lot easier for Phobos. For dmd you are basically left with grep for now.
OTOH even for Phobos grep works fine - we even have the @@@DEPRECATED_ pattern for Phobos, which we don't have here.
Hence, my proposal to have a simple file or wiki in which this is being kept track of for dmd - essentially it's the table at https://dlang.org/deprecate.html, but just with more love (not many deprecations actually update this file and sometimes there are compiler bugs).

https://github.com/dlang/dmd/pull/7760/files is one of the first PRs to use the deprecated pattern in dmd too.

wilzbach · 2018-01-31T02:58:11Z

So this is finally passing the testsuite 🚀 and just in case someone was wondering obviously my motivation is to make this an error with 2.079, s.t. comma expressions can be re-purposed for built-in tuples.
See also: upcoming tuple DIP

JinShil · 2018-01-31T03:34:50Z

test/runnable/testscope.d

@@ -294,7 +294,7 @@ void test11()
    static int i;
    bar11(p, &i);

-    bar11((i,p), &i);  // comma expressions are deprecated, but need to test them
+    bar11(p, &i);


This test was specifically testing the comma operator. Now that it produces an error, it should probably be moved to the fail_compilation tests. However, given that the equivalent expression already exists on line 295, it would probably be ok to just delete it.

Yeah, I was very careful not to delete anything, but yeah in this case it does make sense. Thanks!

JinShil · 2018-01-31T03:43:00Z

test/runnable/xtest46.d

@@ -7866,7 +7867,7 @@ bool test16022()
 {


This issue 16022 was very specific to the comma operator. Since the result of a comma expression now emits an error, this test should no longer compile, and should probably be moved to the fail_compilation test suite.

Ok. I copied it to fail compilation. I don't see any reason to remove it here though.

JinShil · 2018-01-31T03:47:53Z

src/dmd/expressionsem.d

@@ -6272,7 +6272,7 @@ private extern (C++) final class ExpressionSemanticVisitor : Visitor
            /* Rewrite to get rid of the comma from rvalue
             */
            if (!(cast(CommaExp)exp.e2).isGenerated)
-                exp.deprecation("Using the result of a comma expression is deprecated");
+                exp.error("The result of a comma expression can't be used");


I'd prefer this to say "The result of a comma expression is deprecated and can no longer be used". But, it's ok the way it is; I'll pull it regardless.

Hmm it's not deprecated anymore, it's a real error and should be displayed to the user as such.

I agree with @wilzbach.

Would be nice to distinguish "can't be used" (due to some technical limitation) from "can't be used" (because we decided to forbid it). How about "Using the result of a comma expression is not allowed" ?

jacob-carlborg · 2018-01-31T07:05:09Z

changelog/comma-deprecation-error.dd

@@ -0,0 +1,4 @@
+The deprecation period of using the result of comma expression has ended
+
+Comma expressions have proven to be a frequent source of confusion, and bugs.


Please include an example that now fails to compile and show how to correct it. Perhaps include an example showing how the comma expression can still be used without using the result.

I was hoping I could avoid the redundancies from https://dlang.org/deprecate.html#Using%20the%20result%20of%20a%20comma%20expression, but okay copying doesn't cost us much.
I copied the text I have written for dlang.org a while ago.

jacob-carlborg · 2018-01-31T10:14:06Z

changelog/comma-deprecation-error.dd

+    foo((++a, b));
+
+    synchronized (lockA, lockB) {}
+    // isn't currently implemented, but still compiles "thanks" to the comma operator


"thanks" sounds a bit condescending. Perhaps replace with due.

Also submitted for dlang.org: dlang/dlang.org#2152

jacob-carlborg · 2018-01-31T10:16:15Z

test/runnable/mangle.d

    }
-    static assert((test2!f9525(), true));


Are the static assert not necessary anymore? Or will the static assert inside the function take care of that?

I think the static assert was only there, s.t. the function got evaluated and the static assert inside of it could trigger, hence I added evalTest1 and evalTest2.

CyberShadow · 2018-01-31T15:38:24Z

changelog/comma-deprecation-error.dd

+    ---
+
+    It's also common to use the comma operator in for-loop increment
+    statements to allow multiple expressions.


I think it should be clearer that this use case is not affected.

CyberShadow · 2018-01-31T15:39:10Z

changelog/comma-deprecation-error.dd

+)
+$(H4 Rationale)
+$(P The comma operator leads to unintended behavior (see below for a selection)
+    Moreover it is not commonly used and it blocks the ability to implement tuples.


It blocks implementing tuples as a language feature using commas, not in general

CyberShadow · 2018-01-31T15:42:19Z

changelog/comma-deprecation-error.dd

+$(P The comma operator leads to unintended behavior (see below for a selection)
+    Moreover it is not commonly used and it blocks the ability to implement tuples.
+
+    A selection of problems through the accidental use of the comma operator:


Honestly all of these examples are entirely unconvincing because they could be caught by a "discarding result of side-effect-free subexpression" warning.

It took them from here: https://dlang.org/deprecate.html#Using%20the%20result%20of%20a%20comma%20expression
dlang/dlang.org#1308

If you have better examples, just let me know.

CyberShadow · 2018-01-31T15:42:45Z

changelog/comma-deprecation-error.dd

+        // will always be reached
+    }
+
+    void foo(int x) {}


I don't understand this example. Perhaps it would make sense for void foo(int x, int y=0) ?

CyberShadow · 2018-01-31T15:44:52Z

src/dmd/expressionsem.d

@@ -6272,7 +6272,7 @@ private extern (C++) final class ExpressionSemanticVisitor : Visitor
            /* Rewrite to get rid of the comma from rvalue
             */
            if (!(cast(CommaExp)exp.e2).isGenerated)
-                exp.deprecation("Using the result of a comma expression is deprecated");
+                exp.error("The result of a comma expression can't be used");


Would be nice to distinguish "can't be used" (due to some technical limitation) from "can't be used" (because we decided to forbid it). How about "Using the result of a comma expression is not allowed" ?

wilzbach · 2018-02-01T11:18:25Z

Addressed all comments except for:

Honestly all of these examples are entirely unconvincing because they could be caught by a "discarding result of side-effect-free subexpression" warning.

It took them from here: https://dlang.org/deprecate.html#Using%20the%20result%20of%20a%20comma%20expression
dlang/dlang.org#1308
If you have better examples, just let me know. And we can always add them to changelog later.

wilzbach · 2018-02-05T14:52:25Z

@JinShil you still have this on "requested changes". I addressed your comments and copied the files over to fail_compilation, but kept the modified version in compilation. Are you okay with this?
I would like to move on with this, s.t. we can sip this with 2.079 and can introduce tuples into D as soon as possible ...

quickfur · 2018-02-06T01:07:04Z

Woohoo!!!! The comma operator is officially dead!!! celebration

wilzbach mentioned this pull request Jan 31, 2018

End the deprecation period of using the result of a comma expression dlang/dlang.org#2151

Merged

wilzbach force-pushed the error-comma branch from 1f471d5 to 5d1b0e7 Compare January 31, 2018 00:42

quickfur reviewed Jan 31, 2018

View reviewed changes

wilzbach force-pushed the error-comma branch from 5d1b0e7 to 577d491 Compare January 31, 2018 01:15

wilzbach force-pushed the error-comma branch from 577d491 to ad42672 Compare January 31, 2018 01:25

wilzbach force-pushed the error-comma branch from ad42672 to 23369bd Compare January 31, 2018 01:32

wilzbach added the Severity:Deprecation -> Error label Jan 31, 2018

wilzbach force-pushed the error-comma branch 3 times, most recently from 544bf0c to be0edfa Compare January 31, 2018 02:18

andralex approved these changes Jan 31, 2018

View reviewed changes

wilzbach force-pushed the error-comma branch from be0edfa to 462da06 Compare January 31, 2018 02:45

JinShil suggested changes Jan 31, 2018

View reviewed changes

wilzbach force-pushed the error-comma branch from 462da06 to 9eccdc4 Compare January 31, 2018 05:30

jacob-carlborg suggested changes Jan 31, 2018

View reviewed changes

wilzbach force-pushed the error-comma branch from 9eccdc4 to 06a276b Compare January 31, 2018 09:54

jacob-carlborg reviewed Jan 31, 2018

View reviewed changes

jacob-carlborg approved these changes Jan 31, 2018

View reviewed changes

wilzbach force-pushed the error-comma branch from 06a276b to 29f4e0b Compare January 31, 2018 10:25

wilzbach mentioned this pull request Jan 31, 2018

"thanks" sounds a bit condescending. dlang/dlang.org#2152

Merged

CyberShadow reviewed Jan 31, 2018

View reviewed changes

End the deprecation period of using the result of a comma expression.

2d8174f

wilzbach force-pushed the error-comma branch from 29f4e0b to 2d8174f Compare February 1, 2018 11:13

wilzbach added this to the 2.079.0 milestone Feb 5, 2018

JinShil approved these changes Feb 5, 2018

View reviewed changes

JinShil added the Merge:auto-merge label Feb 5, 2018

dlang-bot merged commit c55c9be into dlang:master Feb 6, 2018

wilzbach deleted the error-comma branch March 13, 2018 17:28

		@@ -0,0 +1,4 @@
		The deprecation period of using the result of comma expression has ended

		Comma expressions have proven to be a frequent source of confusion, and bug.

End the deprecation period of using the result of a comma expression. #7813

End the deprecation period of using the result of a comma expression. #7813

Conversation

wilzbach commented Jan 31, 2018 • edited Loading

quickfur commented Jan 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilzbach Jan 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlang-bot commented Jan 31, 2018

Bugzilla references

wilzbach commented Jan 31, 2018

wilzbach commented Jan 31, 2018

JinShil Jan 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilzbach Jan 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacob-carlborg Jan 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilzbach commented Feb 1, 2018

wilzbach commented Feb 5, 2018

quickfur commented Feb 6, 2018

wilzbach commented Jan 31, 2018 •

edited

Loading

wilzbach Jan 31, 2018 •

edited

Loading

JinShil Jan 31, 2018 •

edited

Loading

wilzbach Jan 31, 2018 •

edited

Loading

jacob-carlborg Jan 31, 2018 •

edited

Loading