More pathological range subtleties

[H. S. Teoh]

On Tue, Feb 12, 2013 at 09:52:48PM +0100, monarch_dodra wrote:
[...]
> First: when foreach failed to call "save", it started down the wrong
> path. If you plan to re-use your range, you *must* call save. Let's
> try again with this:

Yes, a frightening amount of Phobos code suffers from this problem.


> //----
> 	auto fun(RoR)(RoR ror)
> 		if (isForwardRange!RoR && isInputRange!(ElementType!RoR))
> 	{
> 		foreach (ref e; ror.save)
> 		{
> 			if (!e.empty)
> 				e.popFront();
> 		}
> 		return ror;
> 	}
> //----
> 
> At this point, I'll argue that the only *legal* outcome is (1). Why?
> Because you asked for a "ref e", yet in your examples, your ranges
> did not yield references. This is a bug with the language, and the
> reason you are getting a strange behavior. From there, bad things
> are bound to happen.

I've always felt uneasy about the way D handles ref's in foreach loops.
It feels like it was a good idea that was carried over from D1, but
didn't get implemented thoroughly enough to handle the new stuff in D2.
Forcing opApply to *always* take a delegate with ref arguments is one
indication of the deeper problem. This makes it so that the container
can't customize its behaviour for the ref case and for the non-ref case,
and this conflation leads to problems down the road (like people just
passing a ref to a temporary variable when they want foreach loops to
not change the contained elements, which leads to bugs 'cos the person
writing the foreach loop thought things were being updated but they
actually aren't).

But anyway. That's a different issue from the one at hand.


> The foreach should *not* have legally compiled. That's why we
> usually avoid it in std.algorithm.
> 
> So if we try to avoid the foreach bug, and switch it back to a for
> loop.
> 
> //----
> 	auto fun(RoR)(RoR ror)
> 		if (isForwardRange!RoR && isInputRange!(ElementType!RoR))
> 	{
> 		auto bck = ror.save;
> 		for ( ; !ror.empty ; ror.popFront())
> 		{
> 			e = ror.front;
> 			if (!e.empty)
> 			{
> 				e.popFront();
> 				ror.front = e;
> 			}
> 		}
> 		return bck;
> 	}
> //----
> 
> Now, once you have written this code, the only way it could break is
> if RoR is an assignable forward transient range, that sends to the
> trash what is assigned to it. Which would be a violation of its own
> interface, so "not out problem" ® .

This code still doesn't work with the second RoR candidate I gave,
because .front always returns a fresh slice of the underlying array. The
thing is, the original range simply does not keep track of its
subranges, because they are conceptual, there's no actual storage where
they are kept.

But at least, this code should refuse to compile when you try to assign
ror.front = e (because .front is non-ref). I guess this is better than
nothing.


T

-- 
Mediocrity has been pushed to extremes.