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

First draft of DIP to introduce a small syntax to improve UDAs #196

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

maxhaton
Copy link
Member

This DIP introduces a new SpecialKeyword to act as a default initializer to make UDAs more powerful and expressive.

@Bolpat
Copy link
Contributor

Bolpat commented Nov 11, 2020

As I understand it, __ATTRIBUTE__ will resolve to an array of declarations that contains the declarations a UDA is applied to. (Correct me if I'm mistaken.) It will only be useful when used as a default value.

First of all, the name is misleading. I had trouble understanding what it intends to do, always wondering what attributes it returns. If it returns an array, i.e. possibly multiple values, choose a plural-sounding name. Secondly, it doesn't resolve to attributes at all. It resolves to attached declarations, so it should have DECLARATIONS or DECLS in its name. I thought of some names, but __ATTACHED_TO_DECLARATIONS__, although being correct, sounds off and __DECLARATIONS_ITS_ATTACHED_TO__, while very descriptive, is a little lengthy and informal sounding. (I'd go with the latter as a working title and name, because to the reader of the DIP it will be an aid.)

The main problem this DIP will have is justifying the language change, since it's behavior can be replicated by a library and it's not even very difficult. Fast compilation speed is a good argument if it can be supported by data. Ideally, a well-made implementation of the proposal lets you directly compare speed. But even if you don't have one, might be convincing enough to merely compare iterating the a whole module's declarations recursively in search for an attribute versus not doing so.

DIPs/1NNN-MHH.md Outdated Show resolved Hide resolved
DIPs/1NNN-MHH.md Show resolved Hide resolved
DIPs/1NNN-MHH.md Outdated Show resolved Hide resolved
DIPs/1NNN-MHH.md Outdated Show resolved Hide resolved
DIPs/1NNN-MHH.md Outdated Show resolved Hide resolved
DIPs/1NNN-MHH.md Outdated Show resolved Hide resolved
DIPs/1NNN-MHH.md Show resolved Hide resolved
DIPs/1NNN-MHH.md Outdated Show resolved Hide resolved
@maxhaton
Copy link
Member Author

  1. It is a bad name, I just chose ATTRIBUTE in my implementation, so I copied the tests into the document.

  2. The behaviour cannot be easily done with a library, getting the symbol the UDA attached to is possible but it's slow, requires additional annotations to walk recursively (efficiently), and on top of that really doesn't play nice across libraries e.g. my WIP benchmarking library does this as a library but if you try to use it as a dub package it currently doesn't work. It's extremely temperamental and also makes debugging the library a complete pain.

My current implementation actually adds a field to (currently Scope although I'm not convinced it's the right place) the compiler data structures to keep track of the UDA then walks upwards from the expression the default initializer is in - there is no searching algorithm to compare against (so it's O(1) vs O(n) in the general case). My implementation doesn't properly conform to the dip in some non-obvious situations because it doesn't check while it iterates up the scopes, but I will push it when it actually conforms.

The functionality could probably go into traits, but it would be much more complicated to implement (the logic to resolve a default initializer already exists within the compiler whereas writing a new trait might be a lot more complicated due to having to start the resolution from deeper within the bowels of the compiler) and make traits even more bloated that it already is.

@Bolpat
Copy link
Contributor

Bolpat commented Nov 12, 2020

You probably should include an Alternatives section to go into the details why the following pattern has too much drawbacks. That pattern isn't obvious because one wouldn't expect to be able to use a declared symbol as a template argument to a UDA that is attached to that symbol. But, well, it can:

template attribute(declarations...)
    if (declarations.length > 0)
{
    /* implementation */
}

// On module scope:
@attribute!func int func() { return 1; }

//In a struct:
struct S
{
    @attribute!(x, y, z)
    int x, y, z;
}

If needed, a recursive iteration can verify that @attribute is used properly (i.e. attatched to exactly the symbols its template parameter refer to). That need not be executed every time, but for debugging or some similar circumstances. If proper use of attribute means that @attribute!(decls) is attached to and only attached to decls then at least one direction can be verified efficiently by attribute itself:

struct attribute(declarations...)
    if (declarations.length > 0)
{
    import std.meta : allSatisfy, ApplyRight;
    import std.traits : hasUDA;
    static assert (allSatisfy!(ApplyRight!(hasUDA, attribute), decls));
    // Even without expensive recursive iteration, we can get some
    // confidence that `attribute` is used properly.
    /* further implementation */
}

Notice that one has to make attribute a struct and cannot use a mere template or enum template because with those, hasUDA won't work properly.

One drawback I see apart from the obvious DRY violation, is overloaded functions. Reiterating the example, if func is overloaded, @attribute!func catches all overloads, not only the one(s) it's attached to. Generally, it catches everything that is accessible by the name func in scope.

template attribute(decls...)
{
    import std.traits : isCallable, Parameters;
    static foreach (decl; decls)
    static if (isCallable!decl)
    static foreach (overload; __traits(getOverloads, __traits(parent, decl), __traits(identifier, decl)))
    {
        pragma(msg, __traits(identifier, decl), Parameters!overload);
    }
}

// On module scope:
@attribute!func int func() { return 1; }

// Intentionally untagged overload:
int func(int x) { return x; }

The inner workings of attribute print

func()
func(int)

i.e. it the tag catches both overloads while only the first one is intended to be caught. It can be coped with using another name for the offending overloads' declarations and aliasing them:

@attribute!funcImpl
int funcImpl() { return 1; }
alias func = funcImpl;

Then, the inner workings of the attribute template only show funcImpl().

If an overloaded function is tagged once per overload, that makes no difference as far as "execution" of the inner workings of the template is concerned: it is instantiated with the same parameter(s), so its inner workings will be run only once; as the DIP states, that can be mitigated (in practically all circumstances) by giving it a parameter initialized defaulted to __LINE__.

Improper annotation isn't always caught. Copy-paste errors can be likely:

@attribute!func1 int func1() { return 0; }
@attribute!func1 int func2() { return 0; }

Here, func2 isn't annotated properly and the static assert won't catch it since the func1 is annotated.

You may want to have a look at the code in action. Your part is arguing out why this isn't a good enough solution. Attack points might be that users of such a library really need to know about the overload case; misuse will not be caught by the compiler without an expensive recursive check.

@maxhaton maxhaton marked this pull request as draft November 13, 2020 12:54
@maxhaton
Copy link
Member Author

@Bolpat I shall although I was hoping it was obvious due to DRY

@maxhaton maxhaton changed the title First draft of DIP to introduce __ATTRIBUTE__ First draft of DIP to introduce a small syntax to improve UDAs Nov 16, 2020
@maxhaton
Copy link
Member Author

@Bolpat It's now __UDA_ATTACHED_TO_DECLS__ which I think is a good enough name. It's not much longer than PRETTY_FUNCTION.

@maxhaton maxhaton marked this pull request as ready for review November 16, 2020 12:27
@maxhaton
Copy link
Member Author

maxhaton commented Nov 16, 2020

@mdparker draft status is set. I think it's ready as it's ever going to be now.

@UplinkCoder
Copy link
Member

Interesting.
What is the rationale for wanting this feature?

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.

3 participants