Added Runtime Field Getting and Setting

[JackStouffer]

Added a new function setAttribute, which allows someone to set an attribute of a type based by accessing the member with a string at runtime.

This is useful when the member being changed can change due to some external factors. For example, I have a library that parses strings for timezone info. The string can contain information for regular UTC offsets or DST offsets, which are both attributes of an internal result type. The info on which to set is only available at runtime. A possible way to refactor such an example is to have a general offset variable and hold some state in the type as to which type of offset the value is referring to, but IMO this is much cleaner.

[wilzbach]

That's pretty sweet! Nice!

[tsbockman]

setAttribute is not an appropriate name for this in D, since attribute has a different meaning. A direct alternative would be setProperty or setField.

But, I think it would be better to design a general-purpose solution to the problem of runtime dispatch, instead of a narrow solution that can only set fields. (I will probably follow this up with a proof-of-concept in a little while.)

[JackStouffer]

Updated code to setField

[tsbockman]

Updated code to setField

You should update the docs, as well.

[tsbockman]

Below is a generalization of your work.

It supports properties as well as fields, and allows getting, setting, or accessing by reference (where possible). The main thing it is missing is support for (non-property) member functions.

private template rtPropDispatch(bool set, Prop, bool refProp, Obj, bool refObj)
{
    private enum propMix = (set? `` : `return `) ~ `mixin ("obj." ~ mName)` ~ (set? ` = value; return;` : `;`);
    private enum dispMix = (set? `void` : (refProp? `ref Prop` : `Prop`)) ~ ` rtPropDispatch(` ~
        (refObj? (!set && refProp? `return ` : ``) ~ `ref ` : ``) ~ `Obj obj, string name` ~
        (set? `, ` ~ (refProp? `ref ` : ``) ~ `Prop value` : ``) ~ `)
    {
        ` ~ (set? `void` : (refProp? `ref ` : ``) ~ `Prop`) ~ ` checkType(string mName)()
        {
            if (!__ctfe) assert (0);
            ` ~ propMix ~ `
        }

        foreach(mName; __traits (allMembers, Obj))
        {
            static if(__traits(compiles, checkType!mName()))
            {
                if(mName == name)
                {
                    ` ~ propMix ~ `
                }
            }
        }

        assert (0, Obj.stringof ~ " has no " ~ (set? "assignable" : (refProp? "lvalue " : "")) ~ "property \"" ~ name ~ "\" matching " ~ Prop.stringof);
    } `;
    mixin (dispMix);
}

pragma(inline, true)
{
    ref Prop refProp(Prop, Obj)(Obj obj, string name)
        if ( is (Obj == class))
    {
        return rtPropDispatch!(false, Prop, true, Obj, false)(obj, name);
    }
    ref Prop refProp(Prop, Obj)(return ref Obj obj, string name)
        if (!is (Obj == class))
    {
        return rtPropDispatch!(false, Prop, true, Obj, true)(obj, name);
    }

    auto getProp(Prop, Obj)(auto ref Obj obj, string name)
    {
        enum refObj = !is (Obj == class) && (Obj.sizeof > 16);
        return rtPropDispatch!(false, Prop, false, Obj, refObj)(obj, name);
    }
    void getProp(Prop, Obj)(auto ref Obj obj, string name, out Prop value)
    {
        enum refProp = !is (Prop == class) || (Prop.sizeof > 16);
        enum refObj = !is (Obj == class) || (Obj.sizeof > 16);
        value = rtPropDispatch!(false, Prop, refProp, Obj, refObj)(obj, name);
    }

    void setProp(Prop, Obj)(auto ref Obj obj, string name, auto ref Prop value)
    {
        enum refProp = !is (Prop == class) || (Prop.sizeof > 16);
        enum refObj = !is (Obj == class) || (Obj.sizeof > 16);
        rtPropDispatch!(true, Prop, refProp, Obj, refObj)(obj, name, value);
    }
}

(I haven't tested this much; it's just a proof-of-concept.)

[ghost]

I see a small flaw. The original function (not the tsblockman's one) is actually always a template and is not reusable, let's say as a global setter for an aggregate type in particular (because of auto ref).

For example this test does not compile.

@safe unittest
{
    class Foo {int bar, baz;}
    auto foo = new Foo();
    auto intSetter = &setField!(Foo,int);
    intSetter(foo, "baz", 0);
    intSetter(foo, "bar", 1);
    assert(foo.baz == 0);
    assert(foo.bar == 1);
}

Maybe it's possible with two overloads ?

[schuetzm]

About the naming: The documentation is still inconsistent, it still speaks about attributes. I think "member" is a more common name. Even if you choose not to change the name, it's a good idea to at least mention the word "member" in the documentation to make it easier to find.

[tsbockman]

@schuetzm

I think "member" is a more common name.

That is a more general term, which also encompasses functions / methods, properties, aliases, nested types, etc. Currently, this PR only works with actual member variables, which are properly called "fields".

However, I would like to see it expanded to at least cover getting and setting properties as well, as right now it seems unnecessarily narrowly focused.

[JackStouffer]

Ok, so I updated this using the code from @tsbockman (thanks!), and I made the following changes:

• I didn't see much use in the version of getFeild with the out parameter, so I removed it
• had to remove pragma(inline) because dmd was failing to inline very simple calls
• In order to make this @nogc, I had to modify the custom error message to be slightly less helpful. See https://issues.dlang.org/show_bug.cgi?id=15100

[tsbockman]

@JackStouffer

I didn't see much use in the version of getFeild with the out parameter, so I removed it

EDIT: The out version can infer the Prop from the type of value, which could be convenient.

had to remove pragma(inline) because dmd was failing to inline very simple calls

DMD seems to have suffered a major regression in inlining capability recently. It's not just my code above (which did inline OK when I wrote it) - checkedint got messed up recently also, to the extent that it's about 50% slower on DMD than it was a month ago.

[JackStouffer]

Why is the function call impure on 32bit systems?

[tsbockman]

@JackStouffer I have submitted some corrections to your branch, mostly cosmetic in nature: JackStouffer#1

Why is the function call impure on 32bit systems?

Attribute inference is failing for rtPropDispatch() for some reason. EDIT: It's not a compiler bug; see my comment below.

If it is manually marked pure, the unit tests will pass on 32-bit. However, this is not a real solution because real-world property methods aren't always pure. So, we still need to get inference working somehow.

[JackStouffer]

@tsbockman Thanks for your changes!

[tsbockman]

@JackStouffer

Why is the function call impure on 32bit systems?

OK I figured it out now, and it's not a compiler bug, after all. Starting from the failing test:

class A
{
    int bar = 42;
}

auto a = new A();
int val = a.getField!int("bar"); // why is getField() impure?

By manual inlining and template instantiation, I determined that the getField() call is equivalent to this:

int getField()(A obj, string name)
{
    if ("bar" == name)
    {
        return obj.bar;
    }

    static if (is(size_t : int)) // true on 32-bit, but not on 64-bit
    {
        if ("toHash" == name)
        {
            /* toHash() is a virtual function inherited from Object,
               which does not guarantee it to be pure. */
            return obj.toHash;
        }
    }

    assert (0, "A has no property matching int");
}

This is an expression of a more general problem with runtime dispatch: only the intersection of the restrictive attributes (pure, @safe, nothrow, etc.) for each of the possible matches can be applied to getField() or any similar function.

Some possible solutions:

1. Just let attribute inference do its thing - don't test support for pure, @safe, nothrow, or @nogc.
2. Exclude some or all of the members of Object from the search: toString(), toHash(), opCmp(), opEquals(), Monitor, factory()
3. Provide some means of manually annotating getField(), refField(), and setField() with restrictive attributes as needed. (Incompatible properties will be removed from the list of possible matches.)

I believe that (3) is the best option. Because restrictive attributes are transitive, (1) would severely limit the usefulness of this PR. (2) would simplify use and probably be "good enough" for many cases, but in the general case it has all the same problems as (1).

[tsbockman]

Also, I finally remembered why I included the out overload of getField(): t can infer the type of Prop from the type of value, which could be convenient. I will illustrate in a unit test.

[tsbockman]

@JackStouffer I have submitted another minor PR: JackStouffer#2

(This does not fix the 32-bit unit tests. I want to think about the best way to do that some more.)

[tsbockman]

I started a forum thread to see if anyone has a good idea about how to implement solution (3) to the attribute problem: http://forum.dlang.org/post/rqydvgscbrjkrrvkbsnj@forum.dlang.org

[JackStouffer]

I don't know of a solution that doesn't uglify the code other than just to let attribute inference do it's thing.

Conceptually, 90% of the time property functions are going to be @nogc nothrow pure @safe anyway, so if I just add a bugs: section in explaining the situation, then I think it's fine to just go as is.

[tsbockman]

if I just add a bugs: section in explaining the situation, then I think it's fine to just go as is.

No, because toHash() and toString() both have extremely common return types, and currently will mess up any call where Obj is a class and size_t or string is implicitly convertible to Prop. That is, after all, why your unit tests are failing on 32-bit...

I think a better compromise is to only support pure @safe property methods. Have a look at my new pull request: JackStouffer#3

[tsbockman]

Assuming this passes the auto tester, I am satisfied with this now from a functional perspective.

My remaining concerns are minor, and are not blockers:

1) The current naming scheme is a bit awkward - the functions are called _Field() (except for rtPropDispatch()), but the template parameter is Prop. This is unnecessarily confusing. I see two reasonable fixes:

a) Rename rtPropDispatch to rtFieldDispatch and Prop to Fld, or
b) Rename _Field() to _Prop() as I originally suggested.

I would prefer (b) because I tend to think of "property" as a superset of "field" - but either would be OK. Let's just make it consistent, one way or the other.

2) The documentation is kind of repetitive. In particular, should we combine the docs for the two getField() overloads?

[tsbockman]

This looks good on the auto tester now - both 64- and 32-bit are passing.

[JackStouffer]

@Hackerpilot @schuetzm @bbasile Pinging people who commented on this, this is ready to review.

[tsbockman]

@JackStouffer Any comment on my bike shedding above?

[JackStouffer]

@tsbockman I was an idiot and forgot to pull before I forced pushed :/

Can you send a PR with your brach changes again?

[tsbockman]

@JackStouffer Please rebase your setattr branch on the latest master for Phobos. Afterwards I can resubmit my fixes.

(For some reason I can't get the unit tests to pass, otherwise.)

[JackStouffer]

@tsbockman done

[ghost]

My previous comment was related to the fact that the function generates, in hard, the code to get each member with a common type (because the member name is a run-time parameter). But because of auto ref it was actually always a template. So I'd suggest to add a unittest that verifies that for example a single delegate to getField!int can be used to get the int fields of two variables in an aggregate.

[ghost]

Why Feild and not Field ? Is it a typo or am I missing something ?

[tsbockman]

@bbasile That's just a type-o. It got fixed earlier, but the change got undone accidentally. It will be fixed again shortly.

[ghost]

I see, anyway it seems that GH is malfunctioning right now, The doc builder displays Pull request SHA mismatch. Never seen that before.

[JackStouffer]

@tsbockman I manually pulled your setattr_tsb branch into my branch, but Github seems to be having problems right now, so it's not updating.

[tsbockman]

@JackStouffer The version I force-pushed about 10 minutes ago is the one you want. I'll make an actual pull request for it if GitHub doesn't acknowledge your manual pull soon.

[tsbockman]

Pull request: JackStouffer#4

[JackStouffer]

For some reason, I can't get the remote branch to sync to my local one despite the messages from git saying that everything is working. I am going to close this PR and use a new branch.

I think this has something to do with the GH outage, but I don't know how this would be fixed if that is the case.

[tsbockman]

@JackStouffer Just fork my setattr_tsb branch when you start over, then.

[JackStouffer]

It's not just this branch that seems to be not updating, my entire Phobos repo refuses to update at all :/

Hopefully this is just a GH backend problem that will be fixed soon, or else I might have to recreate my Phobos repo, which would mean having to close all of my open PRs.