Bug 14544 – isForwardRange failed to recognise valid forward range

this code fails static assertion: struct Input { auto opSlice (size_t start, size_t end) { static struct InputSlice { @property bool empty () { return false; } @property char front () { return 0; } void popFront () {} InputSlice save () { return this; } } import std.range.primitives; static assert(isForwardRange!InputSlice); return InputSlice(); } } fixing code like this tames `isForwardRange`: template isForwardRange(R) { enum bool isForwardRange = isInputRange!R && is(typeof( (inout int = 0) { R r1 = R.init; //old: static assert (is(typeof(r1.save) == R)); auto s1 = r1.save; static assert (is(typeof(s1) == R)); })); } i wonder if some other checking primitives has similar bug.

You failed to put @property on save. That's why it won't compile.

wow, it's fun. it specified nowhere in the docs. what i see is "with the save primitive", which clearly not necessary a property. it also contradics my common sense too. with the fix i proposed it doesn't matter if `save` is property or not. oh, well, let it be another D wart that meant to make programmer feel stupid. D full of that anyway.

Oh, it's stupid that save is required to be a property, since it's not at all an abstraction for a variable, but I don't think that Andrei really understands the idea behind properties (certainly, he didn't when he made save a property, and I also think that that's why he's generally been unhappy with @property when it's come up). The problem is though that once it's required to be a property, changing it later risks breaking code. Given how poorly properties are checked by the compiler, we _might_ be able to make the change, but we'd have to be careful about it. Regardless, the fact that isForwardRange is the way it is and requires that save be a property is very much on purpose.

something should be fixed, i think. either documentation, or `isForwardRange`. and fixing `isForwardRange` will not break anything, as it will happily accept both prop and non-prop `save`...

(In reply to Jonathan M Davis from comment #3) > The problem is though that once > it's required to be a property, changing it later risks breaking code. Given > how poorly properties are checked by the compiler, we _might_ be able to > make the change, but we'd have to be careful about it. Regardless, the fact > that isForwardRange is the way it is and requires that save be a property is > very much on purpose. I'm not exactly sure how "is(typeof())" is working in this case but for a getter, @property doesn't make a difference, regardless if compiled with the -property flag. Take this code for example: @property int foo () { return 1; } @property void bar (int a) { } void main () { auto a = foo(); auto b = foo; bar = 3; bar(4); } This code compiles both with and without the -property flag. The only thing that will not compile is "bar = 3" if "bar" is _not_ marked with @property and it's compiled with the -property flag. What we can do what looks to be completely backwards compatible is modify the "isForwardRange" constraint to look like this instead: static assert (is(typeof(r1.save()) == R)); Or to make it more clear: static assert (is(ReturnType!(save) == R)); These two will work regardless if @property is used on "save" or if the code is compiled with -property.

I don't think this is an invalid bug. One is allowed to call r.save without parens, even without @property. The test is supposed to ensure that r1.save's return type is the same as the original range, but instead is *also* erroneously checking whether it's a non-property function or not. The check simply is wrong, and it's easy to get things like this wrong when one is dealing with duck typing (something I'd love to see fixed). Consider isInputRange's requirements: R r = R.init; // can define a range object if (r.empty) {} // can test for empty r.popFront(); // can invoke popFront() auto h = r.front; Neither of r.empty or r.front require them to be @property. This is simply a bug and needs to be fixed. I would be in favor of fixing as ketmar described. There shouldn't be a requirement that you need or don't need parentheses, the only important thing to check is if you call r1.save, do you get back the same range type. It also may be important to check whether you can assign it to something else, as that is the point of forward ranges (i.e., something that returns a non-copyable struct is going to pass isForwardRange right now if r.save returns by ref, but will not be useful as a forward range). Ketmar's update will fix this too.

PR: https://github.com/D-Programming-Language/phobos/pull/3276

supplemental fix for your PR: diff --git a/std/range/primitives.d b/std/range/primitives.d index 972b709..1e427a4 100644 --- a/std/range/primitives.d +++ b/std/range/primitives.d @@ -908,10 +908,10 @@ template isRandomAccessRange(R) static if(is(typeof(r[$]))) { - static assert(is(typeof(r.front) == typeof(r[$]))); + static assert(is(typeof(f) == typeof(r[$]))); static if(!isInfinite!R) - static assert(is(typeof(r.front) == typeof(r[$ - 1]))); + static assert(is(typeof(f) == typeof(r[$ - 1]))); } })); }

Thanks, done.

Wow, that typeof(somemember) is very strange. Without Ketmar's latest addition, it failed on 32-bit systems and passed on 64-bit. I think there is likely a bug somewhere, but I'm not very motivated to find it :)

hm. now that's strange. i'll try to find what's going on. i don't have 64 bit system, so didn't know that it works there.

Commit pushed to master at https://github.com/D-Programming-Language/phobos https://github.com/D-Programming-Language/phobos/commit/5494539c9d6c7b4414f0cabc5b79e9fcf792410b Merge pull request #3276 from schveiguy/fixforwardrange Fix issue 14544 - isForwardRange failed to recognise valid forward range

Commit pushed to dmd-cxx at https://github.com/dlang/phobos https://github.com/dlang/phobos/commit/5494539c9d6c7b4414f0cabc5b79e9fcf792410b Merge pull request #3276 from schveiguy/fixforwardrange