Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable getSymbolsByUDA to retrieve private members. #3827

Merged
merged 1 commit into from
Nov 29, 2015

Conversation

rcorre
Copy link
Contributor

@rcorre rcorre commented Nov 24, 2015

Resolves #15335: getSymbolsByUDA fails if type has private members.
Generous (ab)use of mixins allows getSymbolsByUDA to reference symbols
without trying to access them, allowing it to inspect private members
without failing.

Testing this required private symbols defined outside of std.traits.
This adds std.internal.test.uda, which defines a struct containing
private members that is used in a unittest added to std.traits.

My previous implementation, #3817, simply ignored private members.

@rcorre rcorre force-pushed the getSymbolsByUDA_private2 branch 2 times, most recently from 523b755 to d70817d Compare November 24, 2015 03:44
// HasPrivateMembers has, well, private members, one of which has a UDA.
import std.internal.test.uda;
static assert(getSymbolsByUDA!(HasPrivateMembers, Attr).length == 2);
static assert(getSymbolsByUDA!(HasPrivateMembers, Attr).length == 2);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is duplicated.

@rcorre rcorre force-pushed the getSymbolsByUDA_private2 branch from d70817d to b493564 Compare November 27, 2015 13:36
@rcorre
Copy link
Contributor Author

rcorre commented Nov 27, 2015

@Hackerpilot: removed the duplicate test line, thanks!

.format(names[0]));
}

enum hasSpecificUDA(string name) = mixin("hasUDA!(symbol.%s, attribute)" .format(name));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor formatting complaint: remove the space between the " and the ..

Resolves #15335: getSymbolsByUDA fails if type has private members.
Generous (ab)use of mixins allows getSymbolsByUDA to reference symbols
without trying to access them, allowing it to inspect private members
without failing.

Testing this required private symbols defined outside of std.traits.
This adds std.internal.test.uda, which defines a struct containing
private members that is used in a unittest added to std.traits.
@rcorre rcorre force-pushed the getSymbolsByUDA_private2 branch from b493564 to 79fd6aa Compare November 28, 2015 12:14
@Hackerpilot
Copy link
Member

Auto-merge toggled on

Hackerpilot added a commit that referenced this pull request Nov 29, 2015
Enable getSymbolsByUDA to retrieve private members.
@Hackerpilot Hackerpilot merged commit d1d1292 into dlang:master Nov 29, 2015
@rcorre rcorre deleted the getSymbolsByUDA_private2 branch November 29, 2015 13:14
mixin("alias toSymbols = AliasSeq!(symbol.%s);".format(names[0]));
else
mixin("alias toSymbols = AliasSeq!(symbol.%s, toSymbols!(names[1..$]));"
.format(names[0]));
Copy link
Member

@MartinNowak MartinNowak Aug 10, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yikes, this does not work. Not all type names are correctly printable and you always use an alias instead of a mixin string to get them.
Could we somehow replace this with __traits(getMember, Type, name)?
Or maybe we have to change the API to return a bunch of strings like __traits(allMembers, Type) and leave it to the people to access the fields.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we somehow replace this with __traits(getMember, Type, name)

Only if we change the API to only retrieve publically accessible members (even if those members are visible in the caller's module)

Or maybe we have to change the API to return a bunch of strings like __traits(allMembers, Type)

I don't think this matters as we'd still be using a mixin for hasSpecificUDA. Get rid of that mixin, and we can only support members visible to std.traits.

I'm not completely sure I understand the problem here, can you elaborate or give an example?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, there are deprecation warnings even with the code as-is. I think we'll have to give up on exposing members that std.traits can't access. Which could really limit the usefulness of this. Users will have to implement their own getSymbolsByUDA in every module they want to use it (if they want it to return non-public members with the UDA).

Copy link
Member

@MartinNowak MartinNowak Aug 12, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only if we change the API to only retrieve publically accessible members (even if those members are visible in the caller's module)

I think __traits(getMember) should bypass visibility checks, b/c it's a meta programming tool and similar use cases are needed in many places.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I mixed that up with using formatted strings to rebuild the type, which doesn't work in many places.

auto test()
{
    struct S
    {
    }
    return S();
}

pragma(msg, typeof(test()).stringof);

But here you're just accessing the members of sth. which are always named.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A mixin template might be viable alternative as well.

Copy link
Member

@schveiguy schveiguy Aug 12, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think __traits(getMember) should bypass visibility checks, b/c it's a meta programming tool and similar use cases are needed in many places.

I'm not sure this is the best idea. __traits(getMember, x, "foo") is as easy to type as x.foo and can have disastrous consequences (if you mess with private data, you can screw up the expected invariants for a type).

I think the best mechanism is to allow access to private symbols if you have the alias already. That is, I can alias my private symbol, hand it off to some other module, and because it now has that key, it can unlock the data. This allows the module to retain control over private data.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

think the best mechanism is to allow access to private symbols if you have the alias already

I'd like this as well, but it won't be useful in this particular case. getSymbolsByUDA only gets the 'parent' symbol, so we still wouldn't be able to access private child symbols.

A mixin template might be viable alternative as well.

I think that would work, but it is a bit uglier on the calling end.

rcorre added a commit to rcorre/phobos that referenced this pull request Aug 10, 2016
Due to more strict accessibility rules, getSymbolsByUDA can no longer
return non-public members, as they are not visible to `std.traits`.

See discussion in dlang#3827.
rcorre added a commit to rcorre/phobos that referenced this pull request Aug 10, 2016
Due to more strict accessibility rules, getSymbolsByUDA can no longer
return non-public members, as they are not visible to `std.traits`.

See discussion in dlang#3827.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants