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

fix Issue 23558 - add __traits(getModuleClasses [, module name]) #14699

Merged
merged 1 commit into from Feb 9, 2023

Conversation

WalterBright
Copy link
Member

Provide alternative to Object.factory(), see #14681

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @WalterBright!

Bugzilla references

Auto-close Bugzilla Severity Description
23558 enhancement add __traits(getModuleClasses [, module name])

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#14699"

@rikkimax
Copy link
Contributor

rikkimax commented Dec 15, 2022

I have to say, I'm not convinced by this approach for a couple of reasons. A filter approach that is more generic would be far better. __traits(getMembers, symbolOrModule, function/class/struct/union/field) would be a lot more useful and allow simplifying significant existing logic.

Use cases: registration of web routes, ORM's, CLI handling.

@WalterBright
Copy link
Member Author

@rikkimax if you or anyone else want to write this function, please do so!

@rikkimax
Copy link
Contributor

Traits at least are something I can do, so yeah it can go on the back burner after the refactor in dub to output a list of modules. Just a shame that we are introducing an ultra-specific trait when we know a more generic one will come eventually ;)

@thewilsonator thewilsonator added the Needs Spec PR A PR updating the language specification needs to be submitted to dlang.org label Dec 15, 2022
@WalterBright
Copy link
Member Author

There's not much point in doing a spec PR until this is approved.

@rikkimax
Copy link
Contributor

@rikkimax if you or anyone else want to write this function, please do so!

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

@ibuclaw
Copy link
Member

ibuclaw commented Dec 15, 2022

But this trait won't work in the same way as the current runtime moduleinfo equivalent, will it?

pragma(msg, __traits(getModuleClasses));  // A? 
static if (cond) class B {} 
pragma(msg, __traits(getModuleClasses)); // A, B? 
class A { } 
enum cond = true;

The above is only for illustration, there are likely better/actual examples where traits produce different results depending on ordering (a traits inside a struct members list), but I don't have a compiler to test at the moment.

@WalterBright
Copy link
Member Author

It produces:

tuple("B", "test2.A")
tuple("B", "test2.A")

@WalterBright
Copy link
Member Author

This works as is, but will have to be changed when #14708 is merged.

@WalterBright WalterBright force-pushed the getClassInfos branch 3 times, most recently from 3bec540 to 36a56a3 Compare February 7, 2023 00:23
@WalterBright WalterBright added the Ready To Merge Let's get this merged label Feb 7, 2023
compiler/src/dmd/traits.d Show resolved Hide resolved
compiler/src/dmd/traits.d Show resolved Hide resolved
@RazvanN7
Copy link
Contributor

RazvanN7 commented Feb 9, 2023

@WalterBright could you maybe add a changelog entry + a spec PR?

Copy link
Contributor

@RazvanN7 RazvanN7 left a comment

Choose a reason for hiding this comment

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

Needs changelog entry.

@WalterBright
Copy link
Member Author

@RazvanN7 ready to go

@RazvanN7 RazvanN7 merged commit 450ba04 into dlang:master Feb 9, 2023
@WalterBright WalterBright deleted the getClassInfos branch February 9, 2023 19:33
@adamdruppe
Copy link
Contributor

@rikkimax if you or anyone else want to write this function, please do so!

This has been done by several D programmers. It is an utterly trivial loop over the existing traits(allMembers).

@schveiguy
Copy link
Member

schveiguy commented Feb 10, 2023

This gives you the actual types (which is more useful). I wrote it in 5 minutes.

template AllClasses(alias mod) {
   import std.meta : AliasSeq;
   alias result = AliasSeq!();
   static foreach(m; __traits(allMembers, mod))
      static if(is(__traits(getMember, mod, m) == class))
         result = AliasSeq!(result, __traits(getMember, mod, m));
   alias AllClasses = result;
}

@WalterBright
Copy link
Member Author

Thank you, @schveiguy

The idea is to replace the Object.factory and Typeinfo_Class.find functions that rely on ModuleInfo. If you want to do a better PR with your code, please do.

@schveiguy
Copy link
Member

schveiguy commented Feb 13, 2023

My point was just that the trait you created is not necessary, and in fact, not very helpful. It's hard to turn a string into a type, even when you know the module it comes from (unless you want to use __traits(getMember), but you can't do that with this trait). The reason Object.factory works is because the compiler registers the types -- the strings are just used for lookup, and arbitrarily whatever the typeinfo decides to use, the important part that is needed is the storage of all the types. How do you plan to solve that?

What is needed is a complete solution of how to create a registration system. Ideally at compile-time, but if not, at least at runtime (keeping in mind that static constructors cannot be used as they add cycles to complex code).

@WalterBright WalterBright mentioned this pull request Feb 16, 2023
@adamdruppe
Copy link
Contributor

Let's revert this now before the useless baggage actually makes it into a release.

dkorpel added a commit to dkorpel/dmd that referenced this pull request Feb 17, 2023
RazvanN7 pushed a commit that referenced this pull request Feb 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Needs Spec PR A PR updating the language specification needs to be submitted to dlang.org Ready To Merge Let's get this merged
Projects
None yet
8 participants