Consolidating Range traits #1010

Merged
1 commit merged into from Sep 10, 2014

Conversation

Projects
None yet
8 participants
Collaborator

monarchdodra commented Dec 14, 2012

Specifically:

hasMobileElements/hasAssignableElements/hasLvalueElements/hasSwappableElements

They will now all require to be input ranges. The previous requirements were inconsistent and were:

  • hasMobileElements: Nothing
  • hasAssignableElements: Forward range
  • hasLvalueElements: Forward range
  • hasSwappableElements: Forward range

Furthermore, for these traits we will now also verify that the property holds true for back on bidirectional ranges, as well as index for RA. (This allowed me to catch that Array!bool.range did not, in fact, have back assign).

  • hasMobileElements: Already present: simplified code
  • hasAssignableElements: Added tests
  • hasLvalueElements: Added tests
  • hasSwappableElements: Added tests

Finally, for hasMobileElements, we also verfy that the returned type is equivalent to the type returned by front. This is consistent with the checks required of isBidirectionalRange or isRandomAccessRange


Fianlly, I added string/dstring/char[]/dchar[] tests in the unittests. This should help clear up what string types have which traits (which is not always obvious for newcomers).

Also unlocked the @@@6336 unittest, since it was fixed.

std/range.d
@@ -4397,7 +4438,7 @@ private string lockstepApply(Ranges...)(bool withIndex) if (Ranges.length > 0)
foreach (ti, Type; Ranges)
{
- static if(hasLvalueElements!Type)
+ static if(hasLvalueElements!(Unqual!Type))
@monarchdodra

monarchdodra Dec 14, 2012

Collaborator

This is in lockstep, specifically, for supporting lockstepping on a immutable(T[]):

immutable(T[]) is not an actual range (no popFront), but lockstep seems to accept anything provided its unqual'ed type is a range.

TBH, this may work for immutable arrays, but I question the design for any complex range.

@jmdavis

jmdavis Dec 22, 2012

Member

Why is this needed? immutable(T[]) is not a range, so I see no reason to try and support it in a range-based function, and if you use immutable(T[]) with IFTI, then the template is instantiated with immutable(T)[], so the range-based function actually takes the array as a mutable slice, meaning that a range-based function would never operate on a fully immutable array. So, I don't think that this change makes any sense.

@monarchdodra

monarchdodra Dec 22, 2012

Collaborator

Not sure what IFTI is. The main problem is that lockstep is designed to work with foreach, and foreach works with immutable arrays:

    immutable(int[]) a = [1, 2, 3];

    foreach(ref v; a)
        v.writeln();

which means we want:

    immutable(int[]) a = [1, 2, 3];
    immutable(int[]) b = [4, 5, 6];

    foreach(ref u, ref v; lockstep(a, b))
        writefln("%s & %s", u, v);

The problem is that since sequence is a template, then the actual type that is passed to sequence is immutable(int[]).

This all worked well up to the point where I refined hasLvalueElements to at least check input range, which is only the case when the array is unqualed.

Truth is (I'd say), that the conditions to sequence are not tight enough, and should be:
isInputRange!R || isArray!R

This way, we accept either of actual mutable ranges, or arrays (mutable, or not). Once there, I guess the internal cast becomes irrelevant. In any case, the whole thing should be better documented.

I'll see if I can't get this fixed in another pull.

@jmdavis

jmdavis Dec 22, 2012

Member

Not sure what IFTI is.

Implicit Function Template Instantiation. It's what happens when you use a templated function without explicitly instantiating it. The template arguments are inferred from the function arguments.

The problem is that since sequence is a template, then the actual type that is passed to sequence is immutable(int[]).

No. It's not. Unless you explicitly instantiate a template with immutable(T[]), no template will ever be instantiated with it, because IFTI takes the slice type of the array, not the actual type of the array. So, if you pass an immutable(T[]) to a templated function, it'll be instantiated with immutable(T)[], not immutable(T[]). No templated anything is going to be operating on immutable arrays unless it's explicitly instantiated with that type or it's operating on a local variable which was explicitly declared to be fully immutable.

How on earth are you getting a fully immutable array in a template? Do you have a concrete example of this or you simply assuming that this is a problem based on your reading of the code?

@monarchdodra

monarchdodra Dec 23, 2012

Collaborator

How on earth are you getting a fully immutable array in a template? Do you have a concrete example of this or you simply assuming that this is a problem based on your reading of the code?

I traced it via simple (static) printing: pragama(msg, Range.stringof);: It's definitely there.

Implicit Function Template Instantiation. It's what happens when you use a templated function without explicitly instantiating it. The template arguments are inferred from the function arguments.

Ah I see... But that stops once varargs get int the story (apparently). This passes:

auto foo(Args...)(Args args)
{
    static assert(is(Args[0] == immutable(int[])));
}

void main()
{
    immutable(int[]) a = [1, 2, 3];
    foo(a);
}

Is this a bug, or do varargs explicitly bypass IFTI?

Where I'm calling lockstep(args...), which is taking it's arguments as immutable, then forwarding to: Lockstep!(Args...)(Args args), which is passing the array as explicitly immutable. I guess that's how it happened.

As I said, there was probably already something strange going on there, because I didn't insert the first unqual. This is in the function signature:

allSatisfy!(isInputRange, staticMap!(Unqual, Args[0..$ - 1]))

BTW, I also noticed that a foreach can iterate on a static array. Should this code be made legal?

void main()
{
    int[3] a = [1, 2, 3];
    int[3] b = [4, 5, 6];

    foreach(ref u, ref v; lockstep(a, b))
        writefln("%s & %s", u, v);
}

In any case, I'll try and see if I can't do some work inside lockstep, to fix the issue at the root, rather than band-aid the implementation.

@jmdavis

jmdavis Dec 23, 2012

Member

Is this a bug, or do varargs explicitly bypass IFTI?

I would be inclined to say that it's a bug, but I don't know for sure. Slices didn't originally return a tail-const version of arrays, and IFTI didn't originally instantiate with the tail-const version. They used to use the exact type of the original array, which caused a fair bit of grief. That was fixed a year or two back, but varargs may not have been taken into account when that was done. I don't know. I'd say that it's worth bringing up in the newsgroup and/or creating a bug report for it though.

BTW, I also noticed that a foreach can iterate on a static array. Should this code be made legal?

Maybe, but probably not. In general, static arrays shouldn't work with range-based functions without slicing them first. Exceptions to that are rare. I haven't really dealt with lockstep though, so I don't know what all goes on with it and don't know what the consequences of having it work with static arrays would be. But given that all you have to do to use a static array with a range-based function is to slice it, I don't think that it's all that big a deal to not accept static arrays. Also, static arrays would be copied normally if they were accepted directly, which would probably make the semantics too different from how the ranges work.

Member

quickfur commented Dec 14, 2012

I'm not 100% sure, but the forward range requirements may have come from Andrei's perspective that input ranges are extremely basic and generic, and not much can be assumed about them (his proposal was that input ranges are allowed to be transient, and forward ranges not). So isAssignable, hasSwappableElements, etc., are intended only to apply to forward ranges or higher. I don't think there's 100% agreement in this area yet, though.

Collaborator

monarchdodra commented Dec 14, 2012

I'm not 100% sure, but the forward range requirements may have come from Andrei's perspective that input ranges are extremely basic and generic, and not much can be assumed about them (his proposal was that input ranges are allowed to be transient, and forward ranges not). So isAssignable, hasSwappableElements, etc., are intended only to apply to forward ranges or higher. I don't think there's 100% agreement in this area yet, though.

I understand the argument, but at the same time, I think it is up to the implementation of the range to choose what it offers, and up to the algorithm to decide what it can operate on.

If the trait is too restrictive, you are preventing all input ranges from even potentially possessing any of these functionality (which they could posses).

If anything, I was hesitating putting restrictions on input range at all. For example, hasLength and isInfinite don't even care for ranges, and only look at a single member function...

std/range.d
- return moveAt(r, 0);
- })));
+ alias E = ElementType!R;
+ R r() inout @property;
@jmdavis

jmdavis Dec 22, 2012

Member

Are you 100% certain that this does the same thing as the inout trick above? Though TBH, I confess that I don't entirely understand what inout is supposed to be doing there in the first place.

@monarchdodra

monarchdodra Dec 22, 2012

Collaborator

It's for objects that are declared as inout(int)[]. EG: static assert(!hasSwappableElements!(inout(int)[]));

TBH, I don't know what a inout(int)[] really is.

The original code was written by @denis-sh , in his implementation of lvalueOf in #954.

While I can only observe that it is needed, he actually took the initiative to put it in, so he may have a better idea about this.

@9rnsr

9rnsr Dec 23, 2012

Member

TBH, I don't know what a inout(int)[] really is.

inout(T)[] should be treated as same as const(T)[] in most trait templates such as this.

A variable which typed inout cannot be declared in normal function. It is only allowed in inout functions.

A hack to avoid the compiler check is using is(typeof((inout int _dummy = 0){ ... })) instead of is(typeof({ ... })). Adding one inout parameter makes the lambda inout funciton, then inout variable declaration (e.g. inout(int)[] r;) will be allowed. (Note that (inout int = 0) is an unnamed inout parameter.)

In #954, @denis-sh uses T lvalueOf(T) inout @property.
It it a hack to avoid another compiler check: If a function has an inout return type, it should have one or more inout parameters.
...But, I think this is not correct. lvalueOf is a module level function, not a member function. And qualified module level function is illegal in D. I'm not sure why this is accepted ... it is almost certainly a bug in the compiler IMO.

However, back to here, R r() inout @property; might be legal, because the r() is a nested function, and qualified nested function might be legal in the future. But I think there is no benefit using R r() inout @property; instead of R r = void;.

Finally, there is no merit by changing how to declare r.

Collaborator

monarchdodra commented Dec 23, 2012

OK, I filed 9198, and Kenji seems to be on it.

In the mean-time, I slightly tweaked lockstep, but as soon as Kenji is done with 9198, we can remove ALL the Unqual and other crap, and just stick to isInputRange.

Collaborator

monarchdodra commented Dec 23, 2012

Ok, I removed the whole R r() inout @property; thing.

It is not an issue here, because a range is mutable, but I'll still point out that R r = void does not always work (due to immutability).

Collaborator

monarchdodra commented Jan 9, 2013

OK: 9198 has been fixed, so sequence correctly works now. Still, I sanitized it by requiring that R itself be a range type, and not Unqual!R.

The rest of the pull is unchanged.

Can I get a re-review?

Collaborator

monarchdodra commented Jan 11, 2013

OK: 9198 has been fixed, so...

Anything not directly related to the original development of consolidating the range traits has been spliced into its own pull:

D-Programming-Language#1069

@ghost ghost assigned andralex Feb 10, 2013

Member

alexrp commented Feb 10, 2013

Assigning to @andralex.

Collaborator

monarchdodra commented Jul 8, 2013

Ping on this pull? I think these tighter requirements for these traits will benefit everyone.

@ghost

ghost commented Oct 22, 2013

In the meantime the container fix could have been part of another pull request, so it isn't blocked by a review of a larger changeset.

Collaborator

monarchdodra commented Oct 23, 2013

In the meantime the container fix could have been part of another pull request, so it isn't blocked by a review of a larger changeset.

Done: #1656.

monarchdodra added a commit to monarchdodra/phobos that referenced this pull request Oct 25, 2013

Fix missing `void back(bool)` in Array!bool.Range
Forked from #1010

Also some cleanup:
changed ulong to size_t, some spaces, some Tuples, some "!(T)"=>"!T"
Collaborator

monarchdodra commented Jan 1, 2014

@andralex : Can you review this? I think it's important. That, and there's no reason for it to be left open a year... We need to formalize the requirement, and either pull or reject this.

Member

DmitryOlshansky commented Jan 25, 2014

Still waiting on @andralex input.

Member

DmitryOlshansky commented Mar 25, 2014

@monarchdodra needs rebase

Collaborator

monarchdodra commented Mar 26, 2014

Rebased.

Member

DmitryOlshansky commented Mar 26, 2014

Sadly still blocked on @andralex

Member

quickfur commented Jul 18, 2014

ping @andralex
Review, please. (Or delegate. ;-)) This PR has been sitting here for many moons.

Member

quickfur commented Sep 4, 2014

ping @andralex
Should somebody else take over the decision making here?

@ghost

ghost commented Sep 10, 2014

I see you removed the inout tricks (e.g. in hasMobileElements) which avoided problems with .init (IIRC). Was that issue fixed, so we can safely use .init now?

Edit: Nevermind, the diff view tricked me, there is an (inout int = 0) heading.

@ghost

ghost commented Sep 10, 2014

Auto-merge toggled on

@ghost

ghost commented Sep 10, 2014

This LGTM. We can revert it later if necessary, but it looks totally ok to me.

ghost pushed a commit that referenced this pull request Sep 10, 2014

@ghost ghost merged commit a460ba8 into dlang:master Sep 10, 2014

1 check passed

default Pass: 10
Details

I am uneasy about doing such change without @andralex but it doesn't seem he will ever pay attention to it so there are no other options.

@ghost

ghost commented Sep 10, 2014

Yeah, only when things break loudly do we get a response. But this is a pull from 2012, it was time to go.

Collaborator

monarchdodra commented Sep 10, 2014

Awesome thanks.

Member

quickfur commented Sep 10, 2014

Finally!! Thanks, Andrej!

This issue was closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment