Fix Variant to support structs with "length" field of inappropriate type

[yglukhov]

Fixed handling of a struct with no "length" field defined but with opDispatch of inappropriate type.

[Orvid]

This is intended to support opDispatch in place of a length field, so shouldn't the opDispatch method in question return whatever type Variant expects for a length? Shouldn't this struct still be rejected as invalid?

[yglukhov]

Yes, if opDispatch!("length") returns some T : size_t, it will be used.
But if not, the sample still should compile and throw VariantException in runtime, if length is being accessed. At least this is how i understand this.

Shouldn't this struct still be rejected as invalid?
I think that it should be compiled as valid, because Variant does not require types to declare "length" property. Moreover, "length" property may not be semantically applicable to any type, that implements opDispatch.

Maybe i should extend unittest with the case where length is handled correctly in opDispatch?

[Orvid]

Extending the unittest would be a good idea.

[Kapps]

Adding an assertThrown to check that a VariantException is thrown upon accessing Variant(S).length would be good to have. I'm not sure what Phobos convention is for the assert(__traits(compiles)) in the unittest, whether it's better to have it as a static assert or not (or if it even matters; seems to be done either way in different places).

Besides that, LGTM.

[Orvid]

Provided that it's OK to have a type with only a void opDispatch accepted as a length property (in my view it shouldn't be), and that the unittest is extended to test both the void and proper return type, I'll concur with Kapps, it LGTM.

[Kapps]

To clarify, the point of the pull is not to make opDispatch accepted for length. The problem was that previously if "T.length" compiled, it would use it and enable Variant.length with "return T.length". But with this void opDispatch, T.length would always compile and thus allow Variant.length, however return T.length would not as void can not be returned as ptrdiff_t. This pull makes it so it checks if T.length is both valid and returns something implicitly convertible to ptrdiff_t, and if it does not not, throws a runtime exception as expected. Previously using Variant(Foo) where Foo is a type that contains opDispatch or a length property not implicitly convertible to ptrdiff_t would result in compiler errors.

This is not a change in behaviour, it simply fixes a bug that caused compiler errors when using Variant with certain types.

[yglukhov]

Ok, the test is there. Kapps, thanx for interpreting =).

[monarchdodra]

Needs rebase.

[yglukhov]

Done.

[monarchdodra]

According to documentation:

/** If the $(D_PARAM VariantN) contains an (associative) array,
  * returns the length of that array. Otherwise, throws an
  * exception.
  */

Wouldn't the correct fix "simply" be to always throw when A isn't an (associative) array? It seems that's how it's done for opIndex, for example.

Given the type SWithLength, i'd expect v.length to just throw. The correct call should be v.get!SWithLength.length.

That's how I understand the documentation anyways. Somebody else more savvy with variants?

[yglukhov]

Ahh, that's a good point. Should we fix variant to support array-like structs/classes, changing opIndex, opIndexAssign, documentation, etc? Or should we just throw exception in case of not-an-array value in length(), as you suggest? Actually, such change may break the code, depending on this behavior, but in both cases the behavior would be more "right" than current.

[yglukhov]

Ok, i've changed length() to support only (associative) arrays. This should be ok as a bug fix, while support for array-like structs/classes should probably be done as an extra feature.

[Kapps]

I hadn't noticed that the documentation states that length (and similar operators) are not allowed if it's not an array. It is worth noting that the previous implementation did allow this, so this could technically break code, but since indexing and opApply were not allowed, I don't imagine much code relied on length being available (particularly with the documentation saying it's not allowed).

Regardless, the following works in the latest release and would not after this pull with the fix:

import std.stdio, std.variant;

struct FooContainer {
    @property size_t length() {
        return 3;
    }
}

void main() {
    Variant v = FooContainer();
    assert(v.length == 3);
}

I do find it a bit odd that opIndex/opApply/length/etc are defined to only work for arrays as opposed to any container that defines them though, but this isn't really the place for that.

[yglukhov]

So.. anything else we could do here?

[Kapps]

Any updates on this? The implementation looks fine, the only concern would be breakage, but all this does is makes the implementation conform to the documentation. The actual breakage should be negligible, if any, because it allowed length yet not opIndex/other operators, and the documentation says it's not allowed anyways.

While I believe that Variant should allow these operations on user-defined types as opposed to only built-in arrays, that's outside the scope of this pull. This pull makes things more consistent and in line with the documentation, along with the intended compile-time bug fix.

[yglukhov]

@Kapps, are you at the DConf? Lets meet! =)

[Kapps]

@yglukhov Unfortunately I wasn't able to make it out to DConf. I'm just watching the live-stream.

[yglukhov]

Ah, thats pity.

[JakobOvrum]

As std.variant is probably going to be replaced with a newer implementation at some point, I don't think we need to update all operations to handle UDTs, but this breakage seems gratituous.

I think a more reasonable fix would be:

static if (is(typeof(zis.length) : long))

[Kapps]

Do you have any details about this replaced with a new implementation? I've never heard anything about this, and I'm not sure if I really see the benefit.

[yglukhov]

I've got no details, but i'd like to introduce std.variant replacement, after my optional monitors accepted. My replacement will be done in a more OOP way, ctfe friendly, and zero-size variants will occupy less space, than current ones. Because of monitors my current variant impl occupies more space than it should. It will take some time though to complete it, so i still suggest pulling this PR =).

[yglukhov]

Juuust a little bump. I agree with @JakobOvrum that the first commit was better, checking length to be typeof(return). This would be a bit more forward compatible.

[Kapps]

I prefer the first commit as well, but unless Variant is expanded to allow user-defined types for all array functions, it may as well be consistent and actually conform with the documentation. Regardless, either implementation is perfectly fine and I would be quite surprised if any code breaks with the second commit.

Regardless, it just needs a decision and to be pulled, it's rather silly for this pull to be open for 3 months when neither solution is controversial.

[Kapps]

Seems to have merge conflicts.

[yglukhov]

Rebased and squashed.

[mihails-strasuns]

I like the version that matches the documentation ;)

[mihails-strasuns]

Auto-merge toggled on