Fix Issue 15798 - std.algorithm.mutation.copy takes target by value by JackStouffer · Pull Request #6039 · dlang/phobos

JackStouffer · 2018-01-16T18:34:29Z

copy is designed in such a way that passing in a generic output range as the target value makes no sense, as copy is supposed to return the "remainder" of target. As such, its inputs should be restricted to inputs that are assignable input ranges or slices.

Consider the following from the stackoverflow example

import std.stdio;
import std.digest.digest;
import std.digest.md;
import std.algorithm;

void main() {
    string s = "Hello!\n";
    auto d1 = makeDigest!MD5;
    auto d2 = makeDigest!MD5;
    foreach (ubyte b; s) {
        d1.put(b);
    }
    s.copy(d2);
    writeln(digest!MD5(s).toHexString);
    writeln(d1.finish().toHexString);
    writeln(d2.finish().toHexString);
}

This outputs

E134CED312B3511D88943D57CCD70C83
E134CED312B3511D88943D57CCD70C83
D41D8CD98F00B204E9800998ECF8427E

Because copy takes the target by value. What the user should have done is use std.range.primitives.put to copy an input range to an output range which is taken by reference.

This change adds a deprecation message that alerts the user. Any usage of copy in this way is very likely deprecating broken code.

dlang-bot · 2018-01-16T18:34:31Z

Thanks for your pull request, @JackStouffer!

Bugzilla references

Auto-close	Bugzilla	Description
✓	15798	std.algorithm.mutation.copy takes target by value

JackStouffer · 2018-01-16T18:58:12Z

std/stdio.d

-        .sort()                     // sort the lines
-        .copy(                      // copy output of .sort to an OutputRange
-            stdout.lockingTextWriter()); // the OutputRange
+        put(


This is a rare case where copy works as expected, the only reason being stdout.lockingTextWriter represents an output range which has only one global output destination, and therefore can be passed by value.

But generally it's a very bad idea to accept output ranges by value

Boo, this breaks the UFCS chain. Is there as UFCS-able put in Phobos?

wilzbach · 2018-01-16T19:04:32Z

std/stdio.d

+            stdin                               // read from stdin
+                .byLineCopy(Yes.keepTerminator) // copying each line
+                .array()                        // convert to array of lines
+                .sort()                         // sort the lines


I'm going to repeat @CyberShadow's comment as it got hidden with the last rebase:

Boo, this breaks the UFCS chain. Is there as UFCS-able put in Phobos?

Yah, that's a problem.

This is an existential problem with put, because the member hook is also called put.

JackStouffer · 2018-01-16T19:11:31Z

@CyberShadow In this specific instance you can call lockingTextWriter.put. But it's better to encourage people to use the global put as it takes care of the type of use cases where output ranges only define a char accepting put, and a string is passed.

JackStouffer · 2018-01-16T19:12:41Z

I get that using std.range.put isn't pretty, but it is more correct. This code worked by chance and not by design.

CyberShadow · 2018-01-16T19:12:50Z

But it's not UFCS-able :(

wilzbach · 2018-01-16T19:15:40Z

I get that using std.range.put isn't pretty,
But it's not UFCS-able :(

Wasn't this example one of the few prime examples of how elegant D code can look like? 😞

CyberShadow · 2018-01-16T19:19:53Z

UFCS is one of the more beautiful parts of D. Even though the code worked by chance, it saddens me that code like this now has to become uglier.

I often try to structure my code to improve its UFCS-ability, e.g. [1] [2]. I remember using this very pattern (for copying a range to a file) some time recently as well.

JackStouffer · 2018-01-16T19:22:14Z

There is a solution here: define some function X (we can bikeshed this) that just forwards to put, but because it's not called put we don't have to worry about name collision with the method of the type. That way it can be used in UFCS chains.

CyberShadow · 2018-01-16T19:23:51Z

Yep, + invert args. I guess .reverseArgs!put will work now, but it's not really idiomatic. Anyway not a blocker I guess.

schveiguy · 2018-01-16T19:42:33Z

I'm not 100% convinced this is the right path. "it should only work for arrays" seems short-sighted. It should only work for output ranges that don't copy their state by value. However, we have no mechanism to test for this. Even if we made it a rule that output ranges must be reference types, there's no static way to check for this.

Is there no other path forward here?

JackStouffer · 2018-01-16T19:52:18Z

@schveiguy it also works for ranges with assignable values a-la the retro example with copy.

schveiguy · 2018-01-16T20:03:38Z

it also works for ranges with assignable values

That's not necessarily a requirement:

struct HasAssignableElements
{
   int front;
   void popFront() {front = 0;}
   enum empty = false;
}

JackStouffer · 2018-01-16T20:12:15Z

@schveiguy I don't understand what that's supposed to illustrate. This would be treated as an output range by copy before this change and would discard all the values it's given except the last.

schveiguy · 2018-01-17T02:08:09Z

many output ranges will fail silently

I don't think there are many output ranges that have this problem. Most reference the data anyway, or are lvalues.

I am on the side of not breaking working, valid, reasonable code like the example stdin.byLineCopy(Yes.keepTerminator).array.sort.copy(stdout.lockingTextWriter).

andralex

This doesn't look too good. @JackStouffer have you considered changing copy such that it takes its target by auto ref?

andralex · 2018-01-17T02:36:00Z

std/stdio.d

+            output,                             // writing directly to stdout
+            stdin                               // read from stdin
+                .byLineCopy(Yes.keepTerminator) // copying each line
+                .array()                        // convert to array of lines


as an aside, the parens here must go

schveiguy · 2018-01-17T02:52:51Z

have you considered changing copy such that it takes its target by auto ref?

We can't do that completely, because that changes the semantics of copy with forward ranges that are input ranges with assignable elements:

auto buffer = new int[100];
iota(15).copy(buffer);
writeln(buffer[0 .. 15]);

Currently, this will print the numbers from 0 to 14. If copy is auto-ref, it will print 15 zeros.

I think we can use @JackStouffer's identification of the types of ranges that copy has problems with in this PR and just make THAT overload auto-ref. See #6039 (comment)

wilzbach · 2018-01-17T11:53:15Z

As no one pointed this out before - Jenkins failure is at tools:

changed.d(122): Deprecation: function std.algorithm.mutation.copy!(MapResult!(to, FilterResult!(__lambda5, Splitter!(cast(Flag)false, char[], Wrapper))), Appender!(int[])).copy is deprecated - std.algorithm.mutation.copy to non-arrays should be replaced with std.range.primitives.put

https://github.com/dlang/tools/blob/master/changed.d#L122

JackStouffer · 2018-01-17T14:00:58Z

@schveiguy @andralex @CyberShadow Removed the deprecation and made the new overload take the output range by auto ref. For the new overload, I also made it return void to avoid (heh) confusion with returning "unfilled" portions of output ranges when that doesn't apply.

schveiguy · 2018-01-17T15:51:53Z

I like this direction better.

I have considered further that it's possible an output range accepted by reference may break some code. Namely an output range that does put elements into an external location, but isn't expected to affect the lvalue that was used.

It might be possible to deprecate this functionality, but it might be too clever. If you pass in an lvalue by reference, you can pragma(msg, "Warning, in version 2.0XX, this call will accept the target by reference, and affect the original"), and then make a copy before using it.

schveiguy

I think this is solid, but we don't need to ban usage of copy now that it should work for lvalues.

schveiguy · 2018-01-17T15:53:45Z

std/digest/package.d

        File file = File(filename);

-        //As digests imlement OutputRange, we could use std.algorithm.copy
+        //As digests imlement OutputRange, we could use std.range.put


Can we revert this? copy should work for this now. Alternatively, the comment looks odd since you are calling hash.put, which I first thought was a UFCS call of std.range.put. If we leave the comment as calling std.range.put, maybe include the full call to show how it's different.

schveiguy · 2018-01-17T15:55:33Z

std/digest/package.d

        File file = File(filename);

-        //As digests implement OutputRange, we could use std.algorithm.copy
+        //As digests implement OutputRange, we could use std.range.put


schveiguy · 2018-01-17T15:56:45Z

std/digest/package.d

    auto oneMillionRange = repeat!ubyte(cast(ubyte)'a', 1000000);
    auto ctx = makeDigest!MD5();
-    copy(oneMillionRange, &ctx); //Note: You must pass a pointer to copy!
+    put(ctx, oneMillionRange);


Can revert this, and remove the &. Leave the note, but just say how ctx must be an lvalue for this to work.

schveiguy · 2018-01-17T15:57:24Z

std/digest/package.d

    Hash hash;
    hash.start();
-    copy(range, &hash);
+    put(hash, range);


Can revert, and also switch to UFCS if you want.

Since this one isn't a public example, and std.range is more likely to have already been imported, I'll leave this

OK, I didn't notice that it wasn't an example.

schveiguy · 2018-01-17T15:58:10Z

std/digest/package.d

    auto oneMillionRange = repeat!ubyte(cast(ubyte)'a', 1000000);
    auto ctx = new MD5Digest();
-    copy(oneMillionRange, ctx);
+    put(ctx, oneMillionRange);


schveiguy · 2018-01-17T15:59:58Z

std/stdio.d

 {
-    copy(data, File(fileName, "wb").lockingBinaryWriter);
+    auto w = File(fileName, "wb").lockingBinaryWriter;
+    put(w, data);


I'd prefer reverting this, but I'm fine with the new version as well.

I left this one to remove the global std.algorithm.mutation import.

wilzbach · 2018-01-17T21:00:30Z

changelog/copy-auto-ref.dd

+
+Before this release, $(REF copy, std, algorithm, mutation) took all 
+$(REF_ALTTEXT output ranges, isOutputRange, std, range, primitives) by value.
+Leads to some issues when output ranges aren't designed around a reference to


wilzbach · 2018-01-17T21:01:21Z

changelog/copy-auto-ref.dd

+Before this release, $(REF copy, std, algorithm, mutation) took all 
+$(REF_ALTTEXT output ranges, isOutputRange, std, range, primitives) by value.
+Leads to some issues when output ranges aren't designed around a reference to
+heap memory, e.g. the hash ranges in $(MREF std,digest,package):


I think you need to drop the package

Is there a place to see what this actually produces?

Yes, DAutoTest will show a preview, but it's failing atm due to trailing whitespace:

Searching for trailing whitespace ./changelog/pending.dd:180:With this release, when the target output range in a call to ./changelog/pending.dd:188:Before this release, $(REF copy, std, algorithm, mutation) took all posix.mak:875: recipe for target 'test' failed

wilzbach · 2018-01-17T21:09:58Z

changelog/copy-auto-ref.dd

+assert(h2.finish().toHexString == "D41D8CD98F00B204E9800998ECF8427E");
+-------
+
+This issue no longer occurs as long as `target` is an lvalue.


Just for clarity I would copy the same test with the behavior in >= 2.079

You can just note that in 2.079 going forward, h1 and h2's results will be identical.

wilzbach · 2018-01-17T21:14:41Z

std/algorithm/mutation.d

+    isInputRange!SourceRange &&
+    isOutputRange!(TargetRange, ElementType!SourceRange) &&
+    !isArray!TargetRange &&
+    !hasAssignableElements!TargetRange)


Only the last two overloads differ, so static if could simply things and provide better error messages here.
I assume you wanted to make it explicit that void is returned? The docs already do so.

Not sure what you are meaning here, but I don't know how you do auto ref correctly without an overload.

Ah fair enough :/

wilzbach · 2018-01-17T21:15:03Z

std/algorithm/mutation.d

 Returns:
-    The unfilled part of target
+    The unfilled part of `target` if `target` is not a `put` defining output
+    range.


void otherwise

wilzbach · 2018-01-17T21:16:20Z

std/algorithm/mutation.d


 import std.range.primitives;
-import std.traits : isArray, isBlitAssignable, isNarrowString, Unqual, isSomeChar;
+import std.traits : isArray, isBlitAssignable, isNarrowString, Unqual, isSomeChar, isDynamicArray;


Why is this import needed?

I think this is what the test originally was (not isArray). It may be leftover.

wilzbach · 2018-01-17T23:13:33Z

std/algorithm/mutation.d

    isInputRange!SourceRange &&
-    isOutputRange!(TargetRange, ElementType!SourceRange))
+    isOutputRange!(TargetRange, ElementType!SourceRange) &&
+    (isArray!TargetRange || hasAssignableElements!TargetRange))


Shouldn't this be just hasAssignableElements!TargetRange?

string a; isArray!(typeof(a)).writeln; // true isOutputRange!(typeof(a), char).writeln; // false a[0] = "b"; // cannot modify immutable expression

https://run.dlang.io/is/ivpUfM

Hm.. I think hasAssignableElements would fail on a char[], but not sure if that's a valid output range anyway.

tested, char[] is not an output range for char, which is... strange. It looks like there's special handling for characters in put, so I would have expected this to work.

schveiguy · 2018-01-18T14:10:31Z

changelog/copy-auto-ref.dd

+$(REF copy, std, algorithm, mutation) is any `put` defining output range, the
+target is taken as `auto ref`. This new overload is also `void` returning
+vs the other overloads which return the unfilled portion of the given range
+or array.


This isn't exact enough. It's really if the output range is not an input range with assignable elements. By deduction, this means it defines put, which is a necessary condition, but not a sufficient condition to trigger the auto-refness.

e.g. one could define an input range with assignable elements that ALSO defines put for some optimization. In this case, it won't be auto-ref.

Gah! If someone defines an output range which defines put and also has assignable elements, logically one would assume that any library function would prioritize put because that's supposedly the fundamental operation of output ranges. But copy prioritizes the r.front assignment, which according to the principal of least surprise is wrong. But "fixing" it would be a breaking change!

Thankfully, an output range which is also an input range with assignable elements is really rare.

Well, I would expect that if it does define assignable elements, either interface will properly work in this scenario (copy by value). So I think it's OK.

The real problem would be if you somehow had an assignable element input range of T, which was also an output range of U ;) But I won't worry about that case...

schveiguy · 2018-01-18T14:16:29Z

I would like to hear from @andralex about the implications of using auto ref here, particularly surrounding this scenario: #6039 (comment)

But other than that, LGTM.

JackStouffer · 2018-01-19T19:28:16Z

Fixed

schveiguy · 2018-01-19T19:44:20Z

changelog/copy-auto-ref.dd

+assert(h1.finish().toHexString == "E134CED312B3511D88943D57CCD70C83");
+assert(h2.finish().toHexString == "D41D8CD98F00B204E9800998ECF8427E");
+// this fails in 2.079 and later
+assert(h1.finish().toHexString != h2.finish().toHexString)


Are you allowed to call finish more than once? I'm not familiar with the API of digests. Also the assert is trivially obvious (the two strings are different per the above asserts).

I think in the suggestion I was saying, you could just put a note after the code saying the two digests will be the same in 2.079 and later.

Alternatively, you could leave out the 3rd assert and put as a comment: "In 2.079 and later, both digests will have the same value".

JackStouffer · 2018-01-19T23:04:33Z

Ping @andralex

schveiguy · 2018-01-20T01:44:40Z

Just wanted to say I am good with the rewording, thanks.

andralex

Let's integrate the two versions into one that takes auto ref. Then distinguish cases with static if inside.

This will simplify the constraints. For the arrays, the auto ref won't matter anyway. Thanks!

schveiguy · 2018-01-20T03:49:35Z

@andralex sorry, I'm not sure if you saw my previous comment. Switching to auto-ref does matter for arrays -- now you will advance the array along.

I guarantee there is code out there that does something like this:

auto buffer = new int[100];
iota(15).copy(buffer);
writeln(buffer[0 .. 15]);

Which will be semantically different if copy takes it's target via ref.

andralex · 2018-01-20T03:50:25Z

@schveiguy just create a local copy of the array inside the copy function

JackStouffer · 2022-10-14T19:10:28Z

Closing this as I'm no longer interested. If someone else wants to shepherd these changes, feel free.

JackStouffer requested review from PetarKirov, andralex and wilzbach as code owners January 16, 2018 18:34

dlang-bot added the Severity:Bug Fix label Jan 16, 2018

JackStouffer force-pushed the issue15798 branch 2 times, most recently from 99e9d6f to 43f097f Compare January 16, 2018 18:56

JackStouffer requested review from CyberShadow and schveiguy as code owners January 16, 2018 18:56

JackStouffer commented Jan 16, 2018

View reviewed changes

JackStouffer force-pushed the issue15798 branch from 43f097f to 764fb77 Compare January 16, 2018 19:01

wilzbach reviewed Jan 16, 2018

View reviewed changes

JackStouffer force-pushed the issue15798 branch from 764fb77 to 8c13f74 Compare January 16, 2018 19:07

JackStouffer requested a review from DmitryOlshansky as a code owner January 16, 2018 19:07

JackStouffer force-pushed the issue15798 branch from 8c13f74 to ad3f074 Compare January 16, 2018 19:18

JackStouffer force-pushed the issue15798 branch from ad3f074 to 9210e90 Compare January 16, 2018 19:27

JackStouffer force-pushed the issue15798 branch from 9210e90 to 3a78b6e Compare January 16, 2018 19:49

JackStouffer force-pushed the issue15798 branch from 3a78b6e to b3b8311 Compare January 16, 2018 19:57

JackStouffer added the Review:Needs Review label Jan 16, 2018

andralex reviewed Jan 17, 2018

View reviewed changes

JackStouffer force-pushed the issue15798 branch from b3b8311 to 4296776 Compare January 17, 2018 13:57

JackStouffer force-pushed the issue15798 branch from 4296776 to bda9ff7 Compare January 17, 2018 14:12

schveiguy requested changes Jan 17, 2018

View reviewed changes

Fix Issue 15798 - std.algorithm.mutation.copy takes target by value

67c4512

JackStouffer force-pushed the issue15798 branch from bda9ff7 to 67c4512 Compare January 17, 2018 17:44

wilzbach reviewed Jan 18, 2018

View reviewed changes

schveiguy reviewed Jan 18, 2018

View reviewed changes

schveiguy approved these changes Jan 18, 2018

View reviewed changes

JackStouffer force-pushed the issue15798 branch from bcd57cd to e890252 Compare January 19, 2018 19:28

schveiguy reviewed Jan 19, 2018

View reviewed changes

Added changelog entry

17a0715

JackStouffer force-pushed the issue15798 branch from e890252 to 17a0715 Compare January 19, 2018 23:04

andralex requested changes Jan 20, 2018

View reviewed changes

dlang-bot added Merge:Needs Rebase Review:Needs Work Merge:stalled labels May 27, 2021

JackStouffer closed this Oct 14, 2022

Uh oh!

Conversation

JackStouffer commented Jan 16, 2018

Uh oh!

dlang-bot commented Jan 16, 2018

Bugzilla references

Uh oh!

JackStouffer Jan 16, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JackStouffer commented Jan 16, 2018

Uh oh!

JackStouffer commented Jan 16, 2018

Uh oh!

CyberShadow commented Jan 16, 2018

Uh oh!

wilzbach commented Jan 16, 2018

Uh oh!

CyberShadow commented Jan 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JackStouffer commented Jan 16, 2018

Uh oh!

CyberShadow commented Jan 16, 2018

Uh oh!

schveiguy commented Jan 16, 2018

Uh oh!

JackStouffer commented Jan 16, 2018

Uh oh!

schveiguy commented Jan 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JackStouffer commented Jan 16, 2018

Uh oh!

schveiguy commented Jan 17, 2018

Uh oh!

andralex left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schveiguy commented Jan 17, 2018

Uh oh!

wilzbach commented Jan 17, 2018

Uh oh!

JackStouffer commented Jan 17, 2018

Uh oh!

schveiguy commented Jan 17, 2018

Uh oh!

schveiguy 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!

schveiguy Jan 17, 2018 • 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!

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

JackStouffer Jan 16, 2018 •

edited

Loading

CyberShadow commented Jan 16, 2018 •

edited

Loading

schveiguy commented Jan 16, 2018 •

edited

Loading

schveiguy Jan 17, 2018 •

edited

Loading