Walter and I were looking at indexOf in the standard library. It has several overloads, of which let's look at the first two:

https://dlang.org/library/std/string/index_of.html

Now, why couldn't they be merged into a single function with this constraint (their bodies are identical):

ptrdiff_t indexOf(Range) (
  Range s,
  dchar c,
  CaseSensitive cs = Yes.caseSensitive
)
if (isInputRange!Range && isSomeChar!(ElementType!Range));

It makes sense to look for a character in any range of characters.

Attempting to build that fails because of this type fails to work:

    struct TestAliasedString
    {
        string get() @safe @nogc pure nothrow { return _s; }
        alias get this;
        @disable this(this);
        string _s;
    }

The intuition is that the function should be general enough to figure out that, hey, TestAliasedString is kinda sorta a subtype of string. So the call should work.

So let's see why it doesn't work - i.e. why is TestAliasedString an input range? The definition of isInputRange (in std.range.primitives) is:

enum bool isInputRange(R) =
    is(typeof(R.init) == R)
    && is(ReturnType!((R r) => r.empty) == bool)
    && is(typeof((return ref R r) => r.front))
    && !is(ReturnType!((R r) => r.front) == void)
    && is(typeof((R r) => r.popFront));

Turns out the second clause fails. That takes us to the definition of empty in the same module:

@property bool empty(T)(auto ref scope const(T) a)
if (is(typeof(a.length) : size_t))
{
    return !a.length;
}

The intent is fairly clear - if a range defines empty as a size_t (somewhat oddly relaxed to "convertible to size_t"), then empty can be nicely defined in terms of length. Cool. But empty doesn't work with TestAliasedString due to an overlooked matter: the "const". A mutable TestAliasedString converts to a string, but a const or immutable TestAliasedString does NOT convert to a const string! So this fixes that matter:

    struct TestAliasedString
    {
        string get() @safe @nogc pure nothrow { return _s; }
        const(string) get() @safe @nogc pure nothrow const { return _s; }
        alias get this;
        @disable this(this);
        string _s;
    }

That makes empty() work, but also raises a nagging question: what was the relationship of TestAliasedString to string before this change? Surely that wasn't subtyping. (My response would be: "Odd.") And why was Phobos under the obligation to cater for such a type and its tenuous relationship to a range?

But wait, there's more. Things still don't work because of popFront. Looking at its definition:

void popFront(C)(scope ref inout(C)[] str) @trusted pure nothrow
if (isNarrowString!(C[]))
{ ... }

So, reasonably this function takes the range by reference so it can modify its internals. HOWEVER! The implementation of TestAliasedString.get() returns an rvalue, i.e. it's the equivalent of a conversion involving a temporary. Surely that's not to match, whether in the current language or the one after the rvalue DIP.

The change that does make the code work is:

    struct TestAliasedString
    {
        ref string get() @safe @nogc pure nothrow { return _s; }
        ref const(string) get() @safe @nogc pure nothrow const { return _s; }
        alias get this;
        @disable this(this);
        string _s;
    }

This indeed does implement a subtyping relationship, and passes the isInputRange test.

What's the moral of the story here? Generality is good, but it seems in several places in phobos (of which this is just one example), a combination of vague specification and aiming for a nice ideal of "work with anything remotely reasonable" has backfired into a morass of inconsistently defined and supported corner cases.

For this case in particular - I don't think we should support all types that support some half-hearted form of subtyping, at the cost of reducing generality and deprecating working code.

On Tuesday, 19 March 2019 at 02:52:34 UTC, Andrei Alexandrescu wrote:
> @property bool empty(T)(auto ref scope const(T) a)
> if (is(typeof(a.length) : size_t))
> {
>     return !a.length;
> }
>
> The intent is fairly clear - if a range defines empty as a size_t (somewhat oddly relaxed to "convertible to size_t"), then empty can be nicely defined in terms of length.

If it was intended for arrays only, then
@property bool empty(T)(scope const T[] a)
{
    return !a.length;
}

But if not, what is the reason to define length, but not empty? Anyway, the type may need to be mutable, e.g. it might want to allocate and store the array, so
@property bool empty(T)(auto ref scope T a)
if (is(typeof(a.length) : size_t))
{
    return !a.length;
}

> That makes empty() work, but also raises a nagging question: what was the relationship of TestAliasedString to string before this change? Surely that wasn't subtyping. (My response would be: "Odd.") And why was Phobos under the obligation to cater for such a type and its tenuous relationship to a range?

The design rationale is not documented :) so we can only guess.
My guess is that it was done for some string container (appender?) that returns rvalue string, so the change to ref return would be incorrect.

> But wait, there's more. Things still don't work because of popFront. Looking at its definition:
>
> void popFront(C)(scope ref inout(C)[] str) @trusted pure nothrow
> if (isNarrowString!(C[]))
> { ... }
>
> So, reasonably this function takes the range by reference so it can modify its internals. HOWEVER! The implementation of TestAliasedString.get() returns an rvalue, i.e. it's the equivalent of a conversion involving a temporary. Surely that's not to match, whether in the current language or the one after the rvalue DIP.

That's conflation of collections and ranges. A collection would be a range factory.

> What's the moral of the story here? Generality is good, but it seems in several places in phobos (of which this is just one example), a combination of vague specification and aiming for a nice ideal of "work with anything remotely reasonable" has backfired into a morass of inconsistently defined and supported corner cases.
>
> For this case in particular - I don't think we should support all types that support some half-hearted form of subtyping, at the cost of reducing generality and deprecating working code.

Isn't generality one of phobos acceptance criteria?

On Tuesday, 19 March 2019 at 02:52:34 UTC, Andrei Alexandrescu wrote:
> And why was Phobos under the obligation to cater for such a type and its tenuous relationship to a range?

There's two paths to investigate this:

1. Use git blame to find the commit when it was introduced, and follow the thread to the pull request, Bugzilla issue, etc.

2. Open a draft PR with a fix, and see what breaks (both within D repositories i.e. auto-tester, and in the D ecosystem at large i.e. the project tester).

Good to see you working on D actively again BTW.

On Tuesday, 19 March 2019 at 02:52:34 UTC, Andrei Alexandrescu wrote:
> Walter and I were looking at indexOf in the standard library. It has several overloads, of which let's look at the first two:
>
> [...]

+1 that these corner cases should not be supported in phobos, but at this point how could phobos drop support for these cases? In fact I'm pretty sure I've written a fair amount of code that depends on them (i.e. using alias this to subtype with an rvalue...hey, we all make mistakes). Of course I'm more than willing to fix my code but we're talking about a huge breaking change here. What's the path forward?

On 3/18/19 10:52 PM, Andrei Alexandrescu wrote:
> @property bool empty(T)(auto ref scope const(T) a)
> if (is(typeof(a.length) : size_t))
> {
>      return !a.length;
> }
> 
> The intent is fairly clear - if a range defines empty as a size_t (somewhat oddly relaxed to "convertible to size_t"), then empty can be nicely defined in terms of length. Cool.

This is implemented wrong.

import std.range;

struct S
{
   size_t length() const { return 0; }
}

void main()
{
   S s;
   assert(s.length == 0); OK
   assert(s.empty); // Error, cannot deduce function
}

The problem is a simple one: typeof(a.length) is not size_t, but a function type.

If you put @property on the length function it would work, but I think we should not require that.

-Steve

On Tuesday, 19 March 2019 at 02:52:34 UTC, Andrei Alexandrescu wrote:
> Walter and I were looking at indexOf in the standard library.
>
> What's the moral of the story here? Generality is good, but it seems in several places in phobos (of which this is just one example), a combination of vague specification and aiming for a nice ideal of "work with anything remotely reasonable" has backfired into a morass of inconsistently defined and supported corner cases.
>

Phobos looks too generalized for me and not enough standardized. For (well known) example, min/max functions for integers. Both for security and safety reasons it is better to raise a compile-time error rather than to implement clever signity-aware comparison. I did this in Mir. There a lot of such small nitpicks but their reimplementation make life either for middle size and large projects. It would be nice to have well-standardized generic building blocks like `mir`/`max` in Phobos with verifiable BetterC availability.

Best,
Ilya

On 3/19/2019 2:01 AM, Kagamin wrote:
> Isn't generality one of phobos acceptance criteria?

I'm not so sure about that. Should Phobos work on abstractions that have a square wheel? Or only well-formed abstractions? In my experience, when an abstraction has a square wheel, it's genesis was a poor understanding of how to build an abstraction, not a requirement of that abstraction. Hence, rejecting it helps the programmer build better abstractions.

I.e. should Phobos accept things because it can, or because it should?

On 3/19/2019 6:35 AM, Jonathan Marler wrote:
> +1 that these corner cases should not be supported in phobos, but at this point how could phobos drop support for these cases? In fact I'm pretty sure I've written a fair amount of code that depends on them (i.e. using alias this to subtype with an rvalue...hey, we all make mistakes). Of course I'm more than willing to fix my code but we're talking about a huge breaking change here. What's the path forward?

I'd suggest making a separate overload for the square wheels, and deprecate them.

On Tuesday, 19 March 2019 at 02:52:34 UTC, Andrei Alexandrescu wrote:
> Walter and I were looking at indexOf in the standard library. It has several overloads, of which let's look at the first two:
>
> https://dlang.org/library/std/string/index_of.html
>
> Now, why couldn't they be merged into a single function with this constraint (their bodies are identical):
>
> ptrdiff_t indexOf(Range) (
>   Range s,
>   dchar c,
>   CaseSensitive cs = Yes.caseSensitive
> )
> if (isInputRange!Range && isSomeChar!(ElementType!Range));
>
> It makes sense to look for a character in any range of characters.
>
> Attempting to build that fails because of this type fails to work:
>
>     struct TestAliasedString
>     {
>         string get() @safe @nogc pure nothrow { return _s; }
>         alias get this;
>         @disable this(this);
>         string _s;
>     }
>
> The intuition is that the function should be general enough to figure out that, hey, TestAliasedString is kinda sorta a subtype of string. So the call should work.
>
> So let's see why it doesn't work - i.e. why is TestAliasedString an input range? The definition of isInputRange (in std.range.primitives) is:
>
> enum bool isInputRange(R) =
>     is(typeof(R.init) == R)
>     && is(ReturnType!((R r) => r.empty) == bool)
>     && is(typeof((return ref R r) => r.front))
>     && !is(ReturnType!((R r) => r.front) == void)
>     && is(typeof((R r) => r.popFront));
>
> Turns out the second clause fails. That takes us to the definition of empty in the same module:
>
> @property bool empty(T)(auto ref scope const(T) a)
> if (is(typeof(a.length) : size_t))
> {
>     return !a.length;
> }
>
> The intent is fairly clear - if a range defines empty as a size_t (somewhat oddly relaxed to "convertible to size_t"), then empty can be nicely defined in terms of length. Cool. But empty doesn't work with TestAliasedString due to an overlooked matter: the "const". A mutable TestAliasedString converts to a string, but a const or immutable TestAliasedString does NOT convert to a const string! So this fixes that matter:
>
>     struct TestAliasedString
>     {
>         string get() @safe @nogc pure nothrow { return _s; }
>         const(string) get() @safe @nogc pure nothrow const { return _s; }
>         alias get this;
>         @disable this(this);
>         string _s;
>     }
>
> That makes empty() work, but also raises a nagging question: what was the relationship of TestAliasedString to string before this change? Surely that wasn't subtyping. (My response would be: "Odd.") And why was Phobos under the obligation to cater for such a type and its tenuous relationship to a range?
>
> But wait, there's more. Things still don't work because of popFront. Looking at its definition:
>
> void popFront(C)(scope ref inout(C)[] str) @trusted pure nothrow
> if (isNarrowString!(C[]))
> { ... }
>
> So, reasonably this function takes the range by reference so it can modify its internals. HOWEVER! The implementation of TestAliasedString.get() returns an rvalue, i.e. it's the equivalent of a conversion involving a temporary. Surely that's not to match, whether in the current language or the one after the rvalue DIP.
>
> The change that does make the code work is:
>
>     struct TestAliasedString
>     {
>         ref string get() @safe @nogc pure nothrow { return _s; }
>         ref const(string) get() @safe @nogc pure nothrow const { return _s; }
>         alias get this;
>         @disable this(this);
>         string _s;
>     }
>
> This indeed does implement a subtyping relationship, and passes the isInputRange test.
>
> What's the moral of the story here? Generality is good, but it seems in several places in phobos (of which this is just one example), a combination of vague specification and aiming for a nice ideal of "work with anything remotely reasonable" has backfired into a morass of inconsistently defined and supported corner cases.
>
> For this case in particular - I don't think we should support all types that support some half-hearted form of subtyping, at the cost of reducing generality and deprecating working code.

So basically one more reason to avoid const entirely?

On Tuesday, 19 March 2019 at 02:52:34 UTC, Andrei Alexandrescu wrote:
> The change that does make the code work is:
>
>     struct TestAliasedString
>     {
>         ref string get() @safe @nogc pure nothrow { return _s; }
>         ref const(string) get() @safe @nogc pure nothrow const { return _s; }
>         alias get this;
>         @disable this(this);
>         string _s;
>     }
>

Isn't this exactly the kind of code where you're supposed to use inout?