I opened the following bug before reading reduce's documentation carefully:

  https://issues.dlang.org/show_bug.cgi?id=12610

import std.stdio;
import std.algorithm;

void main()
{
    int[] arr = [ 0 ];

    int[1] seed;
    int[] result = reduce!((sum, _) => sum[])(seed, arr);

    writefln("%s", result);
}

The output is garbage like [373728176].

Note that the original seed is a value type, which is later sliced by the lambda.

Now I think that it can be considered a user error. Do you agree whit that? If so, this is something else we should be careful about similar to the internal buffer of byLine.

Ali

On Tuesday, 22 April 2014 at 17:31:22 UTC, Ali Çehreli wrote:
> I opened the following bug before reading reduce's documentation carefully:
>
>   https://issues.dlang.org/show_bug.cgi?id=12610
>
> import std.stdio;
> import std.algorithm;
>
> void main()
> {
>     int[] arr = [ 0 ];
>
>     int[1] seed;
>     int[] result = reduce!((sum, _) => sum[])(seed, arr);
>
>     writefln("%s", result);
> }
>
> The output is garbage like [373728176].
>
> Note that the original seed is a value type, which is later sliced by the lambda.
>
> Now I think that it can be considered a user error. Do you agree whit that? If so, this is something else we should be careful about similar to the internal buffer of byLine.
>
> Ali

I see one user bug, and one language bug, but no library issues.

The user bug is the lambda, that returns a reference to a local ("sum[]"). That said, I do believe the compiler should be able to catch think.

The "int[] reduce...", however, I think is a outright language issue. Implicilty calling opSlice on a static array is one thing, but doing it on an rvalue is an outright aberration. The code should be rejected no questions asked.

On 04/22/2014 11:03 AM, monarch_dodra wrote:

> On Tuesday, 22 April 2014 at 17:31:22 UTC, Ali Çehreli wrote:
>> I opened the following bug before reading reduce's documentation
>> carefully:
>>
>>   https://issues.dlang.org/show_bug.cgi?id=12610
>>
>> import std.stdio;
>> import std.algorithm;
>>
>> void main()
>> {
>>     int[] arr = [ 0 ];
>>
>>     int[1] seed;
>>     int[] result = reduce!((sum, _) => sum[])(seed, arr);
>>
>>     writefln("%s", result);
>> }
>>
>> The output is garbage like [373728176].
>>
>> Note that the original seed is a value type, which is later sliced by
>> the lambda.
>>
>> Now I think that it can be considered a user error. Do you agree whit
>> that? If so, this is something else we should be careful about similar
>> to the internal buffer of byLine.
>>
>> Ali
>
> I see one user bug,

Let this thread be a lesson to me (and other mortals) then.

> and one language bug, but no library issues.
>
> The user bug is the lambda, that returns a reference to a local
> ("sum[]"). That said, I do believe the compiler should be able to catch
> think.
>
> The "int[] reduce...", however, I think is a outright language issue.
> Implicilty calling opSlice on a static array is one thing, but doing it
> on an rvalue is an outright aberration. The code should be rejected no
> questions asked.

I don't think there is slicing an rvalue though. (?) reduce() is taking a copy of the seed and then returning a slice to it because the user slices it in their lambda. It effectively does the following, which unfortunately compiles:

int[] foo()
{
    int[1] sum;
    return sum[];    // <-- no warning
}

void main()
{
    int[] result = foo();
}

The code would be safe if the user gave a slice to reduce(). This brings up another concern though: Template authors must watch out for cases like the above. There would be cases where we shouldn't simply take a copy of a T object and return it.

Ali

On Tue, 22 Apr 2014 14:17:57 -0400, Ali Çehreli <acehreli@yahoo.com> wrote:

> I don't think there is slicing an rvalue though. (?) reduce() is taking a copy of the seed and then returning a slice to it because the user slices it in their lambda. It effectively does the following, which unfortunately compiles:
>
> int[] foo()
> {
>      int[1] sum;
>      return sum[];    // <-- no warning
> }

It's not slicing an rvalue, but the above is trivially no different than:

int[] foo()
{
   int[1] sum;
   return sum;
}

which does NOT compile.

The issue is that D's escape analysis is very very simplistic. It would be nice if it were better.

Ironically, because of return type inference in lambdas, you circumvented the check!

-Steve

On Tuesday, 22 April 2014 at 18:17:58 UTC, Ali Çehreli wrote:
> On 04/22/2014 11:03 AM, monarch_dodra wrote:
> > The "int[] reduce...", however, I think is a outright
> language issue.
> > Implicilty calling opSlice on a static array is one thing,
> but doing it
> > on an rvalue is an outright aberration. The code should be
> rejected no
> > questions asked.
>
> I don't think there is slicing an rvalue though. (?) reduce() is taking a copy of the seed and then returning a slice to it because the user slices it in their lambda. It effectively does the following, which unfortunately compiles:
>
> int[] foo()
> {
>     int[1] sum;
>     return sum[];    // <-- no warning
> }

Reduce returns "the seed". It's actually doing something more like this:

int[1] foo()
{
    int[1] sum
    sum = sum[]; //The lambda operates, and the
                 //result is assigned back to the seed.
    return sum; //Returns the seed.
}

So when writing:

> void main()
> {
>     int[] result = foo();
> }

There's a *second* level of bug. (assigning an rvalue "int[1]" to int[])

*This* works fine though:
    int[1] seed;
    int[1] result = reduce!((ref sum, _) => sum)(seed, arr);

BTW, I'm re-implemented reduce recently (not yet pulled), but I was *very* thorough about documenting what it does:
https://github.com/D-Programming-Language/phobos/pull/2060

Could you take a look at it (the documentation I mean), and tell me if everything is what you would have expected?

On Tuesday, 22 April 2014 at 18:34:47 UTC, Steven Schveighoffer wrote:
> On Tue, 22 Apr 2014 14:17:57 -0400, Ali Çehreli <acehreli@yahoo.com> wrote:
>
>> I don't think there is slicing an rvalue though. (?) reduce() is taking a copy of the seed and then returning a slice to it because the user slices it in their lambda. It effectively does the following, which unfortunately compiles:
>>
>> int[] foo()
>> {
>>     int[1] sum;
>>     return sum[];    // <-- no warning
>> }
>
> It's not slicing an rvalue, but the above is trivially no different than:

In this case no, but;
//----
int[1] foo();
int[] a = foo();
//----
*is* slicing an rvalue, and it *does* compile. I don't think there needs to be escape analysis to catch this.

On Tue, 22 Apr 2014 14:47:19 -0400, monarch_dodra <monarchdodra@gmail.com> wrote:

> In this case no, but;
> //----
> int[1] foo();
> int[] a = foo();
> //----
> *is* slicing an rvalue, and it *does* compile. I don't think there needs to be escape analysis to catch this.

Oh yeah, that's bad.

-Steve

On Tuesday, 22 April 2014 at 18:49:41 UTC, Steven Schveighoffer wrote:
> On Tue, 22 Apr 2014 14:47:19 -0400, monarch_dodra <monarchdodra@gmail.com> wrote:
>
>> In this case no, but;
>> //----
>> int[1] foo();
>> int[] a = foo();
>> //----
>> *is* slicing an rvalue, and it *does* compile. I don't think there needs to be escape analysis to catch this.
>
> Oh yeah, that's bad.
>
> -Steve

It's filed:
https://issues.dlang.org/show_bug.cgi?id=12625

I hope it gets fixed. I really see no justification for it.

On 04/22/2014 11:45 AM, monarch_dodra wrote:

> Reduce returns "the seed". It's actually doing something more like this:
>
> int[1] foo()
> {
>      int[1] sum
>      sum = sum[]; //The lambda operates, and the
>                   //result is assigned back to the seed.
>      return sum; //Returns the seed.
> }

My original lambda that returned a slice was correct then. The seed would eventually be copied out. Had the compiler not allow slicing the rvalue then I would be in good shape.

> BTW, I'm re-implemented reduce recently (not yet pulled), but I was
> *very* thorough about documenting what it does:
> https://github.com/D-Programming-Language/phobos/pull/2060
>
> Could you take a look at it (the documentation I mean), and tell me if
> everything is what you would have expected?

I think it looks great! :)

Two comments/questions which I did not make on github:

1) Some of the documentation comments that are inside a scope are not formatted as such. For example, this comment does not start with /++ :


https://github.com/monarchdodra/phobos/blob/reduceReimpl/std/algorithm.d#L753

I wonder whether they are still included in the produced documentation.

2) I think even single-line code blocks should have curly brackets but Phobos code does not follow that guideline. :)

Ali

On Saturday, 26 April 2014 at 06:24:26 UTC, Ali Çehreli wrote:
> On 04/22/2014 11:45 AM, monarch_dodra wrote:
>
> > Reduce returns "the seed". It's actually doing something more
> like this:
> >
> > int[1] foo()
> > {
> >      int[1] sum
> >      sum = sum[]; //The lambda operates, and the
> >                   //result is assigned back to the seed.
> >      return sum; //Returns the seed.
> > }
>
> My original lambda that returned a slice was correct then. The seed would eventually be copied out. Had the compiler not allow slicing the rvalue then I would be in good shape.

Well... your lambda *was* returning a slice to its local copy of sum. So I thin kit is still wrong. "(ref sum, _) => sum[]" would have been correct though.

> > BTW, I'm re-implemented reduce recently (not yet pulled), but
> I was
> > *very* thorough about documenting what it does:
> > https://github.com/D-Programming-Language/phobos/pull/2060
> >
> > Could you take a look at it (the documentation I mean), and
> tell me if
> > everything is what you would have expected?
>
> I think it looks great! :)
>
> Two comments/questions which I did not make on github:
>
> 1) Some of the documentation comments that are inside a scope are not formatted as such. For example, this comment does not start with /++ :
>
>
> https://github.com/monarchdodra/phobos/blob/reduceReimpl/std/algorithm.d#L753
>
> I wonder whether they are still included in the produced documentation.

Nope, that was a mistake on my part. Good catch.

> 2) I think even single-line code blocks should have curly brackets but Phobos code does not follow that guideline. :)
>
> Ali

It depends I say. I usually do that, but for certain functions, such as reduce, it would *literally* double the amount of lines required to write it. I that point, the function becomes long enough for it to be a readability problem.