Issue 23473: Improvement: Implement single-argument __traits(getOverloads) form.

[FeepingCreature]

https://issues.dlang.org/show_bug.cgi?id=23473

Rationale:

At the moment, when looking at an overload set passed via an alias parameter to a template, there is no way of extracting the individual functions making up the overload set. In the ordinary case, you can use a hack like __traits(getOverloads, __traits(parent, sym), __traits(identifier, sym)), but this fails as soon as the overload set contains alias renames or other modules. But __traits(getOverloads) internally looks at a symbol anyways, so we just redefine the single-argument form (currently an error) to pass that symbol directly.

Should I make a spec PR first?

[dlang-bot]

Thanks for your pull request and interest in making D better, @FeepingCreature! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

• My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
• My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
• I have provided a detailed rationale explaining my changes
• New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

---

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✗	23473	enhancement	Need a way to disassemble an overload function without referencing a parent

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + dmd#14631"

[RazvanN7]

The behavior in the presence of imported overload sets needs to be defined:

// a.d
void foo(int);
void foo(string);

// b.d
void foo();

// c.d
import a;
import b;

__traits(getOverloads, foo);   // which one?!

[RazvanN7]

Also, please add "Fix" in the commit message so that the bot auto-closes the issue if this gets merged.

[RazvanN7]

@FeepingCreature I've tested a bit locally and this seems to be a problem only if the overload set is defined and used in the same module. For example, this works:

// b.d
void first(int) { }

// c.d
void second(string) { }

// a.d
import b : foo = first;
import c : foo = second;

// d.d

import a;

void main()
{
    alias overloads = __traits(getOverloads, a, "foo");
    static foreach(ov; overloads)      
        pragma(msg, __traits(identifier, ov));
}

So if you know where the overload set is defined, things work correctly.

In this light, I would argue that we do not need this addition. getOverloads needs to be pointed out where to
pick up the overload set because that offers maximum flexibility. Instead of adding more complexity to the language
we can simply pinpoint the file where the overload is constituted.

[FeepingCreature]

So a template that takes an alias parameter now needs to be passed the calling module and a string??

I think you're falling into C++ thinking a bit here. You shouldn't ask "can we do this with what we already have", but "is this good or is this bad?" I think doing it that way is bad. Nobody looks at "Yeah you can't write templ!foo, you need to write templ!(net.foo.CurrentModule, "foo")" and thinks "Yeah, this is a good, well-designed language I should recommend to others."

[RazvanN7]

I agree, it's just that I wonder if this form of getOverloads is not susceptible of introducing weird edge cases.

I guess the question is: how often do we find ourselves in the situation presented in the bug report. If it happens a lot, then I guess a case could be made for this addition. If it's not that often, then maybe we should just stick with what we have and accept that you need to jump through some extra loops in some rare cases.

cc @WalterBright @atilaneves

[FeepingCreature]

To give some extra context, my usecase is that I want to pass an overload set to the alias parameter of a function and apply it to sumtype.match "as if it were a bunch of lambdas I've passed individually". Basically, the goal is to use match's lambda selection to implement a SFINAE-in-userspace.

In effect, this will look like foo.subscribe!(getKey, T) ... alias getKey = a => a.key; alias getKey = a => a.value.key; etc, each overload listing one possible way of extracting a key from a subtype of T. Very elegant! However, to do that I need to resolve getKey back into its constituent alias declarations.

This is how the code looks right now:

auto getKey(T)(T value)
if (__traits(hasMember, T, "key"))
{
    return value.key;
}

And repeat. Workable, but elegant by no means whatsoever.

That said, my argument is less "I can't do that unless I have this feature." I don't need need this feature. My argument is this: an overload just is a symbol. Why does __traits(getOverloads) not operate on them? I don't get why the construction of this trait takes on this extra verbosity and particularity.

[atilaneves]

Overload set rules are complicated, and because of that and the fact that I didn't come up with the ones for D, I'll have to leave this one to @WalterBright

[pbackus]

Basically, the goal is to use match's lambda selection to implement a SFINAE-in-userspace.

Slightly off-topic: I'm not sure that replacing D's built-in overload selection with match's handler selection is actually a great idea here. Unlike D's native overload selection, match uses the order of the handlers to resolve ambiguous cases. Using it like this would make your code sensitive to the order in which __traits(getOverloads) lists the individual overloads, which in practice is declaration order but officially is unspecified and could vary between compiler releases.

[FeepingCreature]

Offtopic, but I mean, that's something that can be fixed in code - if there's a way to access the overload members.

[FeepingCreature]

I just ran into an issue where I'd be able to make really good use of this, again. What's blocking it, what can I do?

[thewilsonator]

A rebase and a response from @WalterBright by the looks of it, failing the latter, just the former.

I would have expected a larger test case, but I suppose the feature is narrow enough that that is all that is required to cover it.

[FeepingCreature]

If you have any ideas for more tests, I'm all ears. It really is fairly simple.

Rebased. It's a bit annoying of a diff because it adds an indentation.

I would like some feedback from Walter because I don't think the stylistic approach is very good.

[RazvanN7]

@FeepingCreature Please add this test case:

// a.d
void foo(int);
void foo(string);

// b.d
void foo();

// c.d
import a;
import b;

__traits(getOverloads, foo);   // which one?!

And also clarify the behavior.

[FeepingCreature]

Thanks a bunch! That testcase actually surfaced an issue; which to fix I had to adjust the behavior of D overload resolution in general, so now I really want Walter to review before merging.

Shouldn't have a user-visible impact, but let's see what the testsuite says.

[RazvanN7]

Shouldn't this be an error? Why is the first import chosen?

[FeepingCreature]

It's not. __traits(getOverloads, baz) is a tuple of three functions; two from the first import, one from the second. (The order is a bit weird, but shouldn't matter.)

[ibuclaw]

How does it get its order? Is it each overload as they are added to the OverloadSet ?

[FeepingCreature]

Yeah, order of iteration in overloadApplyRecurse.

[ibuclaw]

Did this really need to be broken up into two lines?

Just viewing with whitespace off, having nothing technical to say, so just making suggestions to make the total number of changes down to a minimum.

[FeepingCreature]

D code style says hard 120 limit.

[ibuclaw]

🤷

(master) $ wc -L compiler/src/dmd/*.d  | sort -hr
    340 compiler/src/dmd/dmangle.d
    271 compiler/src/dmd/dinterpret.d
    269 compiler/src/dmd/func.d
    266 compiler/src/dmd/dcast.d
    265 compiler/src/dmd/expressionsem.d
    257 compiler/src/dmd/toctype.d
    235 compiler/src/dmd/dtemplate.d
    230 compiler/src/dmd/dmacro.d
    223 compiler/src/dmd/ctfeexpr.d
    211 compiler/src/dmd/doc.d
    204 compiler/src/dmd/dsymbolsem.d
--- and 70 more files where that came from ---

[FeepingCreature]

It's not great lol, but I figured I shouldn't actively make it worse.

[RazvanN7]

cc @WalterBright @atilaneves as these is a new language addition.

[FeepingCreature]

ping

(Sure would have been good to have this now that the __traits(getAttributes) deprecation is forcing lots of __traits(getOverloads) usage...)

[WalterBright]

I'm confused about what this means. Each overload of foo in a different module forms a different overload set:

module a;
void foo();
void foo(int);

module b;
void foo();

There are two overload sets here, the one for a and one for b. How is this going to work with this change?

[WalterBright]

Overload sets can also have as members other overload sets.

I can't tell from the bug report, the documentation, or the comments, what this is going to do.

[FeepingCreature]

If you're in a module where you see both a and b, and you specify foo, you consider all three as valid overloads. It should use the lexical scope of the identifier token. Ie. the foo in __traits(getOverloads, foo) behaves just the same as if you instantiated a template template templ(alias foo) {} with templ!foo.

[WalterBright]

An overload set is something specific in D, and is not just an aggregation of all the instances of a name that is in scope. I.e. it is not a tuple.

It cannot behave the same, as templ!foo will give an error if more than one foo exists.

[FeepingCreature]

I'm confused, because this works:

void foo(int a) {}
void foo() {}
void bar(alias x)() { x(5); x(); }

void main() {
    bar!foo();
}

This also works if you split the foos into two separate modules.

[WalterBright]

It sounds like this is not retrieving overload sets at all, it is retrieving all the members of all the overload sets with the given name. Is this correct?

I'm not sure why this would be useful, as the point of having overload sets is to not confuse the members of one with the members of another, i.e. the anti-hijacking overload rules. With this change, functions that just happen to have the same name but are in different modules with utterly different uses will be lumped together.

[WalterBright]

Overload sets can also have as members other overload sets.

I can't tell from the bug report, the documentation, or the comments, what this is going to do.

[WalterBright]

dim < 1 || 3 < dim

[FeepingCreature]

done

[WalterBright]

Isn't this change going to impact other uses of this function?

[FeepingCreature]

Yes, maybe? I don't understand the overload logic here. It seems that this function could stuff an entire overload set into the callback, which then presumably split it up further. I suspect there isn't a semantic difference here between "the callback splits up an overload set" and "the callback is called multiple times with the functions in an overload set". And the fact that buildkite is green seems to bear this out.

(This change is the primary reason I wanted your eyes on this.)

[FeepingCreature]

I'm not sure why this would be useful, as the point of having overload sets is to not confuse the members of one with the members of another, i.e. the anti-hijacking overload rules. With this change, functions that just happen to have the same name but are in different modules with utterly different uses will be lumped together.

Oh, is that how it works? If you have three modules, a, b, c, where c imports a and b, and all three have a foo respective, then there's three overload sets for "foo"? Do these all resolve on a lookup to "foo" and then get filtered down?

My basis for the model here is if you call template!foo generally, that has to end up with one indistinguishable overload set... right? Or does it result in one symbol consisting of three overload sets in a next chain? (My understanding of DMD's lookup logic is very shallow.)

[WalterBright]

what happened to f? It's used later on.

[FeepingCreature]

It's set to null way above and filled in directly with the parameter symbol in the one-argument case.

[WalterBright]

Use e.ident == Id.getVirtualFunctions || e.ident == Id.getVirtualMethods instead, as it's much more robust than the nots. Even better, give the deprecation message earlier and get rid of the getVirtualFunctions case.

[FeepingCreature]

This is confusing in the diff, because the indentation changes. The deprecation check is unrelated to the if: I've rewritten it a bit to avoid != regardless. The single-parameter getOverloads gets to skip this entire block because it already gets f directly.

[WalterBright]

Why is sym declared here, when its first use is on line 1059?

[FeepingCreature]

Because it's filled in the else branch for the Id.getOverloads && dim == 1 test.

[FeepingCreature]

It sounds like this is not retrieving overload sets at all, it is retrieving all the members of all the overload sets with the given name. Is this correct?

The thing I'm aiming for is: "Given a symbol (usually resolved from an identifier), retrieve all the concretely-callable things (functions, aliases, structs with opCall), ie. the overload leaves, that would be considered if you tried to call this symbol at this location."

I thought this was what getOverloads was already doing.

[WalterBright]

I thought this was what getOverloads was already doing.

It was not recursively expanding overload sets. For example, if this were pulled as it is, then getOverloads would provide a tuple of overloads with the overload sets information removed, meaning it could not be used as a substitute for overload resolution. The anti-hijacking information will be lost along with the overload sets.

This is why I question the utility of the proposed feature. It will lump together functions that were never designed to be overloaded with each other, they just happened to have the same name.

[FeepingCreature]

But if they're referenced by name in the current module, aren't they by definition overloaded with each other (just by virtue of having the same name)? That's how I've been using them. I don't understand when you'd want to use getOverloads to split one symbol up into groupings on the basis of modules, when this is not something you could ever do via a function call. Like, I don't understand the usecase at all here.

Example, paraphrased from actual production code:

import foo.Decoding : decode;
import bar.Decoding : decode;

void setup() {
    ...
    // `decode` is one symbol combining the overload set from `foo.Decode` 
    // and `bar.Decode`, with origin indistinguishable.
    // When would you want RestService to tease them apart by module, but not by individual function?
    // The whole goal here is to glom them together into one undifferentiated ball.
    auto service = new RestService!(IService, encode, decode)(new ClientImpl(config.address));
    ...
}

[WalterBright]

But if they're referenced by name in the current module, aren't they by definition overloaded with each other (just by virtue of having the same name)?

See: https://dlang.org/articles/hijack.html

[FeepingCreature]

Ah, I see. I wasn't aware of the additional wrinkle that exactly one set had to match. That makes the design make a lot more sense!

So should/could getOverloads do something like...

module A;
void foo(long);

module B;
void foo(int);
void foo(float);

module C;
import A, B;
static assert(is(Parameters!(__traits(getOverloads, foo)[0]) == long));
static assert(is(Parameters!(__traits(getOverloads, __traits(getOverloads, foo)[0])[0]) == int));

As long as I can eventually get at all the leaves, it should do for my purposes.

[FeepingCreature]

Removed the change in func.d. Instead, recognize when we're seeing an overloaded function in __traits(getOverloads) and recurse into it.

This is a bit iffy: it only affects implicit overloads created by importing the same name multiple times. Explicit overloads, created with alias name = foo.name; alias name = bar.name;, will already recurse into every member. Because I have no good way to detect when overloadApply decided to do this, I just remove double visits to the same function. Theoretically, the alias I add to the result tuple should be an overload, so I can do the double __traits(getOverloads) I mentioned in the comment, but it isn't and I don't understand why.

(Though I'm honestly fine with this hack, because I don't think anyone else understands alias/overload management either.)

[FeepingCreature]

ping

[WalterBright]

module A;
void foo(long);

module B;
void foo(int);
void foo(float);

module C;
import A, B;

I don't understand what __traits(getOverloads, foo) should do with this that would be useful. If it recurses into the overload sets, information about which overload sets they go into will be lost, and hence there will be no way to reconstruct it. You'll have foo(long), foo(int), foo(float), and what use will that be? module A was not designed with any consideration for foos in module B, and vice versa. The foos may be for utterly different purposes.

[FeepingCreature]

Spoken frankly: it seems to me you are giving a lot of priority to this way of avoiding overload collisions, whereas from my point of view the originating module of a symbol is basically never relevant. I don't think this has ever mattered to me, and honestly I'm not sure it's ever mattered to anyone else - observe Buildkite remaining green after the overloadApplyRecurse change that broke the whole thing. I want this because the one case I care about resolving overloads is when I'm trying to inspect an overload tuple passed to a template alias parameter, and all the functions I'd pass this way are either disjunctive or not resolved by overloading anyways.

For background, the motivation is about serialized, where I'm using overloads to pull together encode/decode helpers from a bunch of different modules (from git submodules) and stuff them into a single encodeJson/decodeJson call. Now everything is good for encoding, because encode helpers are necessarily narrowing; I can just declare them of WireType encode(InternalType) and get a nice healthy overload that I can just try to call with my internal type. But decode helpers are widening; they'd have to be, for instance, InternalType1 decode(JSONValue); InternalType2 decode(JSONValue);. So I can't use overloading here anyways; I couldn't even declare them, because they'd collide on declaration, not on call. So right now I do InternalType1 decode(T : InternalType1)(JSONValue), etc., and I have to do __traits(compiles, decode!T) to check if I have a matcher. However, see my serialized dconf talk, "__traits(compiles) is Satan" - the __traits(compiles) also fails if there's a compile error inside the decode, which can be hard to notice.

So when I saw __traits(getOverloads) it occurred to me there was a better way to do it:

InternalType1 decode()(JSONValue value) {}
InternalType2 decode()(JSONValue value) {}

and so on. Overloads cleanly, and I can just use __traits(getOverloads) to tease them apart, instantiate each with !(), look at the return type with ReturnType and bypass the inadequate-for-my-purposes overload resolution entirely. Except, no, because see above. Hence this PR.

I don't want to emphasize my usecase too heavily. This is meant more as an illustrative example for why someone could want this. The reason I actually went ahead and did the PR is: if you see a trait called getOverloads, you - atleast, I - assume that this is possible. It's a symbol, it's the most natural thing for a function to be. You have a thing, and you just introspect it with the introspection API, __traits(getOverloads, thing). The thing the trait actually does, rather, takes a lengthy explanation and the specific behavior regarding module sources that justifies it doesn't seem to even be used in practice (where buildkite can see it). Least surprise this ain't.

If you want overload behavior, you just call it! Why would you be taking the overload set apart at all if you weren't interested in the properties of its leaves?

[tgehr]

Buildkite is obviously not going to fail by you removing a way that code could fail to type check. This does not show what you say it shows. In any case, is it not confusing to have a __trait(getOverloads, ...) that returns overload sets rather than overloads?

[FeepingCreature]

Ah yeah, good point, sorry.

As per the test, __traits(getOverloads) returns every individual leaf right now.