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

UDAs for function arguments #4783

Closed

Conversation

IgorStepanov
Copy link
Contributor

Implementation of subj.

This topic was partially discussed ~year ago in NG.
The general idea:

class MyController
{
     View foo(@postParam("a") int p_a, @getParam("b") int p_b);
}

....
static if (is(typeof(MyController.foo) PAR == __parameters))
{
   enum attr_0 = __traits(getAttributes, PAR[0 .. 1]);
   enum attr_1 = __traits(getAttributes, PAR[1 .. 2]);
   static assert(attr_0 == postParam("a"));
   static assert(attr_1 == getParam("b"));
}

This feature is very usefull for different bindings generators like Web MVC or REST bindings like Java Spring MVC.


void test13()
{
static if (is(typeof(test12x) PT == __parameters))
Copy link
Contributor

Choose a reason for hiding this comment

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

test12x vs test13x?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Omg, this test was totally wrong. Fixed.

@schuetzm
Copy link
Contributor

I'm afraid I don't understand that static if() construction. typeof(MyController.foo) is probably something like View delegate(int, int). We would that ever be equal to __parameters?

Urgh... After some searching I found this:
http://forum.dlang.org/post/mailman.1197.1379014886.1719.digitalmars-d-learn@puremagic.com
I'm horrified 😱

@IgorStepanov IgorStepanov force-pushed the uda-for-function-arguments branch from 0c59983 to eebe354 Compare June 29, 2015 16:17
@IgorStepanov
Copy link
Contributor Author

I'm afraid I don't understand that static if() construction. typeof(MyController.foo) is probably something like View delegate(int, int). We would that ever be equal to __parameters?
Urgh... After some searching I found this:
http://forum.dlang.org/post/mailman.1197.1379014886.1719.digitalmars-d-learn@puremagic.com
I'm horrified 😱

Yes, __parameters result looks strange.

You should write __traits(identifier, PT[0 .. 1]) to get correct result. If you write __traits(identifier, PT[0]) you will get a __traits(identifier, int) result.
See phobos soultion for more info.
Anywhere, this feature should be wrapped into high-level phobos template like getArgumentUDAs!(func, <arg num>)

@Geod24
Copy link
Member

Geod24 commented Jun 29, 2015

Having to emulate UDAs on parameters is doable, although it's a pain.

I'd really like to have something in the language to support it (as long as it doesn't conflict with other features / grammar), but __parameters is an horrible hack, and so are some of the things built on top of it, such as ParameterDefaultValueTuple which is at the moment totally broken for meta programming purposes.

Having a magical tuple that can hold things not expressible in any other part of the language (and that vanishes as soon as you put them through a tuple) is already bad enough, please, don't put more magic in it.

@MetaLang
Copy link
Member

I think it's important to note that D can currently introduce new storage classes without breaking any code, due to the fact that UDAs on parameters are not allowed. This pull request would make that no longer true. For example, we could add a new storage class today and be confident that no code would be broken:

struct Test { /*...*/ }

void acceptRvals(@rval ref Test t); //@rval currently doesn't conflict with UDAs

Although I doubt that is much of a concern at this point.

@IgorStepanov
Copy link
Contributor Author

Having to emulate UDAs on parameters is doable, although it's a pain.

Yes, current __parameters interface looks like hack. However it is used for identifier or default value access now, and may be used for UDA access via a special Phobos template.
This PR is about UDA, not about __parameters. If another way to access to parameters data will be introduced in the language, this UDA will work through it.

I think it's important to note that D can currently introduce new storage classes without breaking any code, due to the fact that UDAs on parameters are not allowed.

The same point for global UDAs. However, when @nogc was being implemented, UDAs was exist and potentially may cause a conflict with the new attribute.

@9rnsr
Copy link
Contributor

9rnsr commented Jul 1, 2015

@IgorStepanov Additionally you need to tweak stripDefaultArgs in mtype.c. Because:

enum uda;
void foo(@uda int v) {}

alias x = X!(typeof(foo));

template X(F) {
    // in here, the function type F must not have any udas.
}

@IgorStepanov IgorStepanov force-pushed the uda-for-function-arguments branch from eebe354 to 8f7d239 Compare July 1, 2015 21:23
@IgorStepanov
Copy link
Contributor Author

@9rnsr Done. Also I've added the corresponding test.
@CyberShadow What is the trouble with doc tester? How can I fix it?

@CyberShadow
Copy link
Member

@CyberShadow What is the trouble with doc tester? How can I fix it?

Git repository corruption I think. Fixed for now, hopefully it won't recur

@IgorStepanov
Copy link
Contributor Author

@9rnsr Any other objections?

Git repository corruption I think. Fixed for now, hopefully it won't recur

Thanks.

if (stc == STCproperty || stc == STCnogc || stc == STCdisable ||
stc == STCsafe || stc == STCtrusted || stc == STCsystem)
{
error("@%s attribute for function argument is not supported", token.toChars());
Copy link
Contributor

Choose a reason for hiding this comment

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

Better wording: s/argument/parameter/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@9rnsr
Copy link
Contributor

9rnsr commented Jul 4, 2015

Excepting nitpicks for the diagnostic messages, the implementation looks good to me.

@IgorStepanov
Copy link
Contributor Author

Excepting nitpicks for the diagnostic messages, the implementation looks good to me.

I've fixed messages. Thanks!

@Geod24
Copy link
Member

Geod24 commented Jul 4, 2015

This PR is about UDA, not about __parameters. If another way to access to parameters data will be introduced in the language, this UDA will work through it.

My point was that as soon as it's introduced, we'll have to support it (unless it doesn't make it into a release). But since this would ease my life a lot, I won't complain too much :-)

Is there a corresponding documentation change open ?

Also, any chance you can add a test for the following scenarios:

  • C variadic parameters;
  • UDAs on the RHS of the identifier (fails with a meaningfull error message);
  • Taking aggregates / using a function return value (like in Vibe.d);
  • Types UDAs (including one with an enum Foo;);
  • UDAs on function / delegate parameters (including ones with @nogc or @safe);

@IgorStepanov IgorStepanov force-pushed the uda-for-function-arguments branch from 9be0e59 to 2e0b54f Compare July 5, 2015 12:14
@IgorStepanov
Copy link
Contributor Author

@Geod24

My point was that as soon as it's introduced, we'll have to support it (unless it doesn't make it into a release).

We will use phobos wrapper template in high-level code. Similar to ParameterIdentifierTuple and other. See the simple example of this template in the test runnable/uda.d.
I am also not happy with the current situation with __parameters, but if we start changing this situation now, we will finish this work near Christmas. Maybe. :)

Also, any chance you can add a test for the following scenarios:

Yes, done.

C variadic parameters;
UDAs on the RHS of the identifier (fails with a meaningfull error message);

See fail_compilation/udaparams.d

Taking aggregates / using a function return value (like in Vibe.d);
Types UDAs (including one with an enum Foo;);
UDAs on function / delegate parameters (including ones with @nogc or @safe);

See runnable/uda.d

Any other ideas?

Is there a corresponding documentation change open ?

It is not made. But I'll do it.

@IgorStepanov
Copy link
Contributor Author

@9rnsr I've changed parse.c and func.c code. (Added error for postfix uda declaration and passing uda for parameter VarDeclaration* in FuncDeclaration.
Is it ok?

@IgorStepanov IgorStepanov force-pushed the uda-for-function-arguments branch from 2e0b54f to 3dc677c Compare July 6, 2015 11:37
@IgorStepanov
Copy link
Contributor Author

@9rnsr Please, revalidate this. I've added two changes, declared in previous post.


if (token.value == TOKdotdotdot)
error("variadic parameter cannot has UDAs");
Copy link
Contributor

Choose a reason for hiding this comment

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

s/cannot has/cannot have/

@9rnsr
Copy link
Contributor

9rnsr commented Jul 6, 2015

Looks good excepting one diagnostic nitpick.

TEST_OUTPUT:
---
fail_compilation/udaparams.d(15): Error: variadic parameter cannot has UDAs
fail_compilation/udaparams.d(16): Error: variadic parameter cannot has UDAs
Copy link
Member

Choose a reason for hiding this comment

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

This is inconsistent with the error message on the very next line, which uses user defined attributes as opposed to UDAs. You should use either one or the other, but not both.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, will change to "user defined attributes".

@TheFlyingFiddle
Copy link

What's the status on this?

wilzbach added a commit to wilzbach/dmd that referenced this pull request Jan 2, 2018
wilzbach added a commit to wilzbach/dmd that referenced this pull request Jan 2, 2018
@wilzbach
Copy link
Member

wilzbach commented Jan 2, 2018

Revived in #7576

@wilzbach wilzbach closed this Jan 2, 2018
wilzbach added a commit to wilzbach/dmd that referenced this pull request Jan 8, 2018
wilzbach added a commit to wilzbach/dmd that referenced this pull request Jan 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants