www.digitalmars.com         C & C++   DMDScript  

digitalmars.D.learn - Unexpected foreach lowering

reply Lodovico Giaretta <lodovico giaretart.net> writes:
I'm probably missing something stupid but...
Why on earth do the two loops in main print a different result?
It looks like the foreach lowering is ignoring my definition of 
front...

=====================================================
import std.stdio, std.container.array;

struct RangeWrapper(Range)
{
	Range range;
	alias range this;
	
	auto front()
	{
		return range.front + 1;
	}
}
auto rangeWrapper(Range)(auto ref Range range)
{
	return RangeWrapper!Range(range);
}

void main()
{
	Array!int array;
	array.insertBack(3);
	
	foreach (i; rangeWrapper(array[]))
		writeln(i);           // prints 3, which is wrong
	
         // isn't the above foreach equivalent to the following 
loop ?

	for (auto r = rangeWrapper(array[]); !r.empty; r.popFront())
		writeln(r.front);     // correctly prints 4
}
=====================================================

Thank you for your help.
Aug 10 2016
next sibling parent Lodovico Giaretta <lodovico giaretart.net> writes:
On Wednesday, 10 August 2016 at 18:08:02 UTC, Lodovico Giaretta 
wrote:

 I'm probably missing something stupid but...
 Why on earth do the two loops in main print a different result?
 It looks like the foreach lowering is ignoring my definition of 
 front...

 =====================================================
 import std.stdio, std.container.array;

 struct RangeWrapper(Range)
 {
 	Range range;
 	alias range this;
 	
 	auto front()
 	{
 		return range.front + 1;
 	}
 }
 auto rangeWrapper(Range)(auto ref Range range)
 {
 	return RangeWrapper!Range(range);
 }

 void main()
 {
 	Array!int array;
 	array.insertBack(3);
 	
 	foreach (i; rangeWrapper(array[]))
 		writeln(i);           // prints 3, which is wrong
 	
         // isn't the above foreach equivalent to the following 
 loop ?

 	for (auto r = rangeWrapper(array[]); !r.empty; r.popFront())
 		writeln(r.front);     // correctly prints 4
 }
 =====================================================

 Thank you for your help.


This actually only happens with std.container.Array. Other ranges 
are ok. Even stranger...
Aug 10 2016
prev sibling next sibling parent reply =?UTF-8?Q?Ali_=c3=87ehreli?= <acehreli yahoo.com> writes:
On 08/10/2016 11:08 AM, Lodovico Giaretta wrote:

 I'm probably missing something stupid but...
 Why on earth do the two loops in main print a different result?
 It looks like the foreach lowering is ignoring my definition of front...

 =====================================================
 import std.stdio, std.container.array;

 struct RangeWrapper(Range)
 {
     Range range;
     alias range this;

     auto front()
     {
         return range.front + 1;
     }
 }
 auto rangeWrapper(Range)(auto ref Range range)
 {
     return RangeWrapper!Range(range);
 }

 void main()
 {
     Array!int array;
     array.insertBack(3);

     foreach (i; rangeWrapper(array[]))
         writeln(i);           // prints 3, which is wrong

         // isn't the above foreach equivalent to the following loop ?

     for (auto r = rangeWrapper(array[]); !r.empty; r.popFront())
         writeln(r.front);     // correctly prints 4
 }
 =====================================================

 Thank you for your help.


RangeWrapper does not provide the InputRange interface, so the compiler 
uses 'alias this' and iterates directly on the member range.

I tried making RangeWrapper an InputRange but failed. It still uses 'range'.

// Still fails with these:
      property bool empty() {
         return range.empty;
     }

     void popFront() {
         range.popFront();
     }

I don't know how the decision process works there.

Ali
Aug 10 2016
parent reply Lodovico Giaretta <lodovico giaretart.net> writes:
On Wednesday, 10 August 2016 at 18:38:00 UTC, Ali Çehreli wrote:

 RangeWrapper does not provide the InputRange interface, so the 
 compiler uses 'alias this' and iterates directly on the member 
 range.

 I tried making RangeWrapper an InputRange but failed. It still 
 uses 'range'.

 // Still fails with these:
      property bool empty() {
         return range.empty;
     }

     void popFront() {
         range.popFront();
     }

 I don't know how the decision process works there.

 Ali


That's strange, as RangeWrapper works correctly if instantiated 
with any underlying range EXCEPT std.container.Array.
Also, RangeWrapper does provide the InputRange interface, 
partially directly and partially with alias this. RangeWrapper 
should be "opaque", as it should not matter whether the methods 
needed for the InputRange interface are defined directly or 
inherited with alias this.
Aug 10 2016
parent reply =?UTF-8?Q?Ali_=c3=87ehreli?= <acehreli yahoo.com> writes:
On 08/10/2016 11:47 AM, Lodovico Giaretta wrote:

 On Wednesday, 10 August 2016 at 18:38:00 UTC, Ali Çehreli wrote:

 RangeWrapper does not provide the InputRange interface, so the
 compiler uses 'alias this' and iterates directly on the member range.

 I tried making RangeWrapper an InputRange but failed. It still uses
 'range'.

 // Still fails with these:
      property bool empty() {
         return range.empty;
     }

     void popFront() {
         range.popFront();
     }

 I don't know how the decision process works there.

 Ali


 That's strange, as RangeWrapper works correctly if instantiated with any
 underlying range EXCEPT std.container.Array.


A quick read reveals popFront() is implemented only for bool Arrays. 
That explains the issue.

I don't know whether it's an oversight.

Ali
Aug 10 2016
parent Lodovico Giaretta <lodovico giaretart.net> writes:
On Wednesday, 10 August 2016 at 19:37:39 UTC, Ali Çehreli wrote:

 A quick read reveals popFront() is implemented only for bool 
 Arrays. That explains the issue.

 I don't know whether it's an oversight.

 Ali


First of all, thank you for spending your time on this issue. I 
really appreciate that.

I may be reading that code wrong but...
Isn't popFront implemented for every type here?
https://github.com/dlang/phobos/blob/master/std/container/array.d#L128
Aug 10 2016
prev sibling parent reply Steven Schveighoffer <schveiguy yahoo.com> writes:
On 8/10/16 2:08 PM, Lodovico Giaretta wrote:

 I'm probably missing something stupid but...
 Why on earth do the two loops in main print a different result?
 It looks like the foreach lowering is ignoring my definition of front...

 =====================================================
 import std.stdio, std.container.array;

 struct RangeWrapper(Range)
 {
     Range range;
     alias range this;

     auto front()
     {
         return range.front + 1;
     }
 }
 auto rangeWrapper(Range)(auto ref Range range)
 {
     return RangeWrapper!Range(range);
 }

 void main()
 {
     Array!int array;
     array.insertBack(3);

     foreach (i; rangeWrapper(array[]))
         writeln(i);           // prints 3, which is wrong

         // isn't the above foreach equivalent to the following loop ?

     for (auto r = rangeWrapper(array[]); !r.empty; r.popFront())
         writeln(r.front);     // correctly prints 4
 }
 =====================================================

 Thank you for your help.


The issue is that it tries using [] on the item to see if it defines a 
range-like thing. Since you don't define opSlice(), it automatically 
goes to the subrange.

This breaks for int[] as well as Array.

If I add opSlice to your code (and return this) it works.

This is definitely a bug, it should try range functions before opSlice. 
Please file.

-Steve
Aug 10 2016
next sibling parent reply Lodovico Giaretta <lodovico giaretart.net> writes:
On Wednesday, 10 August 2016 at 20:54:15 UTC, Steven 
Schveighoffer wrote:

 On 8/10/16 2:08 PM, Lodovico Giaretta wrote:

 [...]


 The issue is that it tries using [] on the item to see if it 
 defines a range-like thing. Since you don't define opSlice(), 
 it automatically goes to the subrange.

 This breaks for int[] as well as Array.

 If I add opSlice to your code (and return this) it works.

 This is definitely a bug, it should try range functions before 
 opSlice. Please file.

 -Steve


Wow. Thanks. I didn't know the compiler would try opSlice. I will 
file it.
Aug 10 2016
next sibling parent Lodovico Giaretta <lodovico giaretart.net> writes:
On Wednesday, 10 August 2016 at 21:00:01 UTC, Lodovico Giaretta 
wrote:

 On Wednesday, 10 August 2016 at 20:54:15 UTC, Steven 
 Schveighoffer wrote:

 [...]


 Wow. Thanks. I didn't know the compiler would try opSlice. I 
 will file it.


Filed on bugzilla:

https://issues.dlang.org/show_bug.cgi?id=16374
Aug 10 2016
prev sibling parent reply Jonathan M Davis via Digitalmars-d-learn writes:
On Wednesday, August 10, 2016 21:00:01 Lodovico Giaretta via Digitalmars-d-
learn wrote:

 Wow. Thanks. I didn't know the compiler would try opSlice. I will
 file it.


It does that so that you can use foreach with containers without having to
call something on the container. The idea is that the container will
implement opSlice and make it return a range over the container, and foreach
will then use that range to iterate over the container.

- Jonathan M Davis
Aug 10 2016
parent reply Steven Schveighoffer <schveiguy yahoo.com> writes:
On 8/11/16 12:28 AM, Jonathan M Davis via Digitalmars-d-learn wrote:

 On Wednesday, August 10, 2016 21:00:01 Lodovico Giaretta via Digitalmars-d-
 learn wrote:

 Wow. Thanks. I didn't know the compiler would try opSlice. I will
 file it.


 It does that so that you can use foreach with containers without having to
 call something on the container. The idea is that the container will
 implement opSlice and make it return a range over the container, and foreach
 will then use that range to iterate over the container.


I get that. But it shouldn't try opSlice *first* if the item itself is a 
range (and it does do this). Many random-access ranges define opSlice, 
and most of the time range[] returns this. But in this case, it doesn't.

But it's a no-op for ranges, why waste time calling it?

-Steve
Aug 11 2016
parent Jonathan M Davis via Digitalmars-d-learn writes:
On Thursday, August 11, 2016 08:42:27 Steven Schveighoffer via Digitalmars-d-
learn wrote:

 On 8/11/16 12:28 AM, Jonathan M Davis via Digitalmars-d-learn wrote:

 On Wednesday, August 10, 2016 21:00:01 Lodovico Giaretta via
 Digitalmars-d-

 learn wrote:

 Wow. Thanks. I didn't know the compiler would try opSlice. I will
 file it.


 It does that so that you can use foreach with containers without having to
 call something on the container. The idea is that the container will
 implement opSlice and make it return a range over the container, and
 foreach will then use that range to iterate over the container.


 I get that. But it shouldn't try opSlice *first* if the item itself is a
 range (and it does do this). Many random-access ranges define opSlice,
 and most of the time range[] returns this. But in this case, it doesn't.

 But it's a no-op for ranges, why waste time calling it?


I was just explaining what the deal with opSlice and containers was, since
the OP wasn't familiar with it. I wasn't really saying anything about the
suggested change. However, I have to agree that the suggested change is
likely a good one. As it stands, no range should be implementing opSlice
with no arguments, since it's a container function, not a range function. No
range trait tests for it, and no range-based code should be using it. It's
just supposed to be used on containers to get a range not on the ranges
themselves. But some folks put it on ranges anyway (IIRC, there are even at
least a couple of cases in Phobos where opSlice is implemented on ranges
when it shouldn't be), and it causes problems. There might be a reason why
it would be a bad idea to change it so that opSlice is ignored by foreach if
the type is a range, but I can't think of any right now. Unfortunately,
ranges and foreach have a tendency to get a bit funny thanks to the
differing behavior between types (e.g. ranges that are implicitly saved when
used with foreach vs those that aren't), so mucking around with it should be
done with care, but it at least seems like your suggestion to skip opSlice
if the range primitives are there is a good one.

- Jonathan M Davis
Aug 11 2016
prev sibling parent reply ag0aep6g <anonymous example.com> writes:
On 08/10/2016 10:54 PM, Steven Schveighoffer wrote:

 The issue is that it tries using [] on the item to see if it defines a
 range-like thing. Since you don't define opSlice(), it automatically
 goes to the subrange.

 This breaks for int[] as well as Array.

 If I add opSlice to your code (and return this) it works.

 This is definitely a bug, it should try range functions before opSlice.
 Please file.


Related:

https://issues.dlang.org/show_bug.cgi?id=14619
Aug 10 2016
parent Steven Schveighoffer <schveiguy yahoo.com> writes:
On 8/10/16 5:14 PM, ag0aep6g wrote:

 On 08/10/2016 10:54 PM, Steven Schveighoffer wrote:

 The issue is that it tries using [] on the item to see if it defines a
 range-like thing. Since you don't define opSlice(), it automatically
 goes to the subrange.

 This breaks for int[] as well as Array.

 If I add opSlice to your code (and return this) it works.

 This is definitely a bug, it should try range functions before opSlice.
 Please file.


 Related:

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


That's the same issue. Thanks. Your case is even worse, because calling 
save could be unnecessary and costly.

Note that the compiler will turn this:

foreach(...; foo[]) into this: foreach(...; foo[][])

ugly...

-Steve
Aug 11 2016