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 21585 - add __traits(totype, string) to convert mangled typ… #12156

Merged
merged 1 commit into from
Mar 15, 2021

Conversation

WalterBright
Copy link
Member

…e string to an existing type

This is quite a bit simpler than its previous incarnation #11797

But it has a problem. @andralex wrote #11797 (comment) that __traits can return a type. Oops, it cannot, it only returns an Expression. So, there are two options:

  1. Return a string. This means to turn the string into a type requires a mixin which can fail if the symbols making up the type are not in scope. Besides, doing a mixin is expensive at compile time, especially when we already have the desired type.
  2. Return a TypeExp. TypeExp is a hack in the compiler to treat a type as if it were an expression. But oops, that doesn't work when a type is demanded by the grammar. The solution you can see in the test case is to pass the TypeExp through a template to convert it to an actual Type that can be used wherever a Type is in the grammar. This is what I chose. We can hide this wacky thing inside a nice template and put it in Phobos.

In the future we can perhaps enhance the grammar to extend Type to include __traits(toType, string).

I'll do a spec PR if this is approved.

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @WalterBright!

Bugzilla references

Auto-close Bugzilla Severity Description
21585 enhancement add __traits(totype, string) to convert mangled type string to an existing type

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

@BorisCarvajal
Copy link
Member

@WalterBright you can parse the trait as type with this little patch, no need to use templates.

BorisCarvajal@5eaea1c

@PetarKirov
Copy link
Member

As proposed in the previous PR, I think we should overload based on the last argument to this trait:

__traits(toType, string) :: string -> type
__traits(toType, arrayOfStrings) :: string[] -> (__traits(toType, strings[0]), __traits(toType, strings[1]), ... __traits(toType, strings[$ - 1]))

The motivation is that __traits(toType) would likely be used heavily in the inner loops of meta-programming templates and such it's important to optimize for the case of arrays of mangled type names. This way we can avoid one more levels of unnecessary template wrappers.

@WalterBright
Copy link
Member Author

@BorisCarvajal @PetarKirov these are both good ideas and would make good follow-on PRs.

Copy link
Member

@PetarKirov PetarKirov left a comment

Choose a reason for hiding this comment

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

Please add:

  • an additional fail test to cover all code paths
  • a test shows "importing" a private type by it's mangled name.
  • __traits(toType, magledNameOfTypeFromAnotherModule) - to test the limits of this feature - what happens if you "import" a type from a module that is not being compiled

Otherwise the current implementation looks good to me - simple, but powerful.

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.

The decoToType function would sit better in traits.d. It is the only place where it is called and therefore it can be made private. Plus, we get rid of the import in dmangle.d as traits.d already imports it.

src/dmd/dmangle.d Show resolved Hide resolved
src/dmd/dmangle.d Show resolved Hide resolved
src/dmd/traits.d Show resolved Hide resolved
src/dmd/traits.d Show resolved Hide resolved
src/dmd/traits.d Show resolved Hide resolved
src/dmd/traits.d Show resolved Hide resolved
@RazvanN7
Copy link
Contributor

Ping @WalterBright

@WalterBright WalterBright force-pushed the fix21585 branch 6 times, most recently from 2de2b9c to 63c0bf0 Compare March 14, 2021 05:44
@WalterBright
Copy link
Member Author

ping @RazvanN7

@rainers
Copy link
Member

rainers commented Mar 14, 2021

I'm afraid this will disappoint too easily, as the compiler needs to have seen the exact type in advance, e.g.

pragma(msg, __traits(toType, "Pi"));
int* p;
pragma(msg, typeof(p).mangleof);

yields

totype.d(1): Error: cannot determine `__traits(toType, "Pi")`
totype.d(1):        while evaluating `pragma(msg, __traits(toType, "Pi"))`
Pi

It passes if p is declared before the trait.

@rainers
Copy link
Member

rainers commented Mar 14, 2021

BTW: the example from the bugzilla entry doesn't compile:

__traits(toType, "i") x;         // declares `x` as having type `int`

results in

totype.d(2): Error: trait `toType` is either invalid or not supported as type

@WalterBright
Copy link
Member Author

the example from the bugzilla entry doesn't compile

That's because the Type template is needed, as in the test case in this PR.

@WalterBright
Copy link
Member Author

It passes if p is declared before the trait

That's right. Unfortunately, there's not an obvious way around the compiler has to see the type before it can demangle it.

@rainers
Copy link
Member

rainers commented Mar 14, 2021

That's because the Type template is needed, as in the test case in this PR.

I think toType is missing here: https://github.com/dlang/dmd/blob/master/src/dmd/typesem.d#L1762

@rainers
Copy link
Member

rainers commented Mar 14, 2021

Unfortunately, there's not an obvious way around the compiler has to see the type before it can demangle it.

That makes it very difficult to find a compelling use case for this addition.

Maybe it is possible to pass a type as a string to a CTFE function to generate a mixin that allows the mixin to be instantiated anywhere, even if the type is unknown at the point of instantiation. I think this should be explored before merging and should be demonstrated as a test case.

@WalterBright
Copy link
Member Author

@rainers I didn't know that dmd was changed to allow __traits to act as a type. So I fixed this PR per your suggestion.

@WalterBright WalterBright dismissed RazvanN7’s stale review March 15, 2021 05:47

addressed requested changes in the comments

* Type for succeeded
*/

public Type decoToType(const(char)[] deco)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not move this function to traits.d ? Then it can be made private.

Copy link
Member Author

Choose a reason for hiding this comment

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

Because traits.d is plenty large enough, and it would be best suited to be with the mangling code.

@maxhaton
Copy link
Member

I read the original thread, but I still don't think I see what the rationale is here?

@WalterBright
Copy link
Member Author

I still don't think I see what the rationale is here?

Provide symmetry to .mangleof. Trying to do this without compiler support would be tedious, slow and fragile.

@maxhaton
Copy link
Member

I still don't think I see what the rationale is here?

Provide symmetry to .mangleof. Trying to do this without compiler support would be tedious, slow and fragile.

OK. I was just making sure I hadn't missed some uber-useful thing buried in the thread/s somewhere. TypeExp seems a bit of a hack to my eye, is there a long term alternative in the compiler as-is?

@dlang-bot dlang-bot merged commit c720ddd into dlang:master Mar 15, 2021
@WalterBright
Copy link
Member Author

TypeExp has been there a long time, it is there to deal with the cases of ambiguity in the grammar.

@WalterBright WalterBright deleted the fix21585 branch March 16, 2021 01:19
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.

7 participants