[WIP] fix issue 7804 - Allow __traits(getMember) as a BasicType #8031

ghost · 2018-03-14T12:08:49Z

To allow alias AggrMember = __traits(getMember, Aggr, "member"); The BasicType grammar must be changed, since it's used by AliasDeclarationY.

BasicType:
    BasicTypeX
    . IdentifierList
    IdentifierList
    Typeof
    Typeof . IdentifierList
    TypeCtor ( Type )
    TypeVector
+    Traits

As a bonus, declarations work with a __traits as type.
Another more limited option would have been to change the grammar of AliasDeclarationY:

AliasDeclarationY:
    Identifier TemplateParametersopt = StorageClassesopt Type
+    Identifier TemplateParametersopt = StorageClassesopt Traits
    Identifier TemplateParametersopt = FunctionLiteral

For now, only Types are handled. A second part will allow this for Symbols.

dlang-bot · 2018-03-14T12:08:51Z

Thanks for your pull request and interest in making D better, @bbasile! 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 annotated coverage diff directly on GitHub with CodeCov's browser extension
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
✓	7804	enhancement	Cannot alias __traits directly

jacob-carlborg · 2018-03-14T16:06:35Z

src/dmd/parse.d

+            }
+            else if (strcmp(te.ident.toChars, "getMember") != 0)
+            {
+                error("invalid `__traits`, only `getMember` can be aliased and not `%s`",


Should this really be handled by the parser?

Yes. It's a semantic thing but it can be handled eagerly.

AndrejMitrovic · 2018-03-14T20:19:59Z

Nice work on handling such an ancient issue!

jacob-carlborg · 2018-03-15T07:01:39Z

src/dmd/dmangle.d

    Tslice       : '@',
    Treturn      : '@',
    Tvector      : '@',
+    TTraits      : '@',


I think this should be Ttraits to follow the naming conventions.

RazvanN7

Cool addition! Thanks!

RazvanN7 · 2018-03-16T09:35:19Z

src/dmd/parse.d

+        {
+            AST.TraitsExp te;
+            if (AST.Expression e = parsePrimaryExp())
+                te = cast(AST.TraitsExp) e;


How about :

AST.Traits te = cast(AST.TraitsExp)parsePrimaryExp();

RazvanN7 · 2018-03-16T09:45:00Z

src/dmd/parse.d

+                    te.ident.toChars);
+                t = new AST.TypeError;
+            }
+            else t = new AST.TypeTraits(loc, te);


Brackets for if/else bodies should be consistent (use {} for all or for none).

RazvanN7 · 2018-03-16T09:48:33Z

src/dmd/hdrgen.d

        buf.writestring(t.dstring);
    }

+    override void visit(TypeTraits t)


A similar visit method should be added in the ParseTimeTransitiveVisitor [1] otherwise the tree traversal will be truncated.

[1] https://github.com/dlang/dmd/blob/master/src/dmd/transitivevisitor.d

RazvanN7 · 2018-03-16T09:50:33Z

src/dmd/mtype.d


+/**
+ */
+extern (C++) final class TypeTraits : Type


Please add some documentation for this class : why is it needed, when it is created and what information is kept in each field. Hope that is the intent with the empty comments 👍

Indeed i planned to do it but forgot to.

ghost · 2018-03-16T13:43:43Z

First part, handling of types, is done now. Will start the second soon.

WalterBright · 2018-03-23T20:27:02Z

src/dmd/parse.d

+                // error already emitted while parsing primary
+                t = new AST.TypeError;
+            }
+            else if (strcmp(te.ident.toChars, "getMember") != 0)


strcmp must die. Really, everyone, please stop using it! Use if (te.ident == Id.getMember).

Would be good to approve sth. like #7893 then, s.t. we can start using DStrings everywhere.

WalterBright · 2018-03-23T20:28:13Z

src/dmd/typesem.d

+                result = t.addMod(mtype.mod);
+        //if (result) printf("TypeTraits found to be %s\n".ptr, result.toChars);
+        if (result is null)
+            mtype.error(loc, "`%s` cannot be resolved to a valid type",


definitely need a test case for this

WalterBright · 2018-03-23T20:29:30Z

src/dmd/mtype.d

    Tvector,
    Tint128,
    Tuns128,
+


no purpose to blank line

WalterBright · 2018-03-23T20:30:45Z

src/dmd/hdrgen.d

+    override void visit(TypeTraits t)
+    {
+        //printf("TypeBasic::toCBuffer2(t.mod = %d)\n", t.mod);
+        visit(t.exp);


need test case

How is tested hdrgen ?

WalterBright · 2018-03-23T20:31:25Z

src/dmd/mtype.d

 }

+/**
+ * This type is a shell containing a TraitsExp that can be


This type is a is completely redundant and can be elided.

wilzbach · 2018-03-23T20:31:53Z

(@reviewers: don't forget that this requires a PR to dlang.org with the spec update before this can be merged)

WalterBright · 2018-03-23T20:32:10Z

src/dmd/mtype.d

+    {
+        this.loc = loc;
+        this.exp = exp;
+        super(Ttraits);


super call should go first.

WalterBright · 2018-03-23T20:32:44Z

src/dmd/mtype.d

+    {
+        TypeTraits tt;
+        TraitsExp te = cast(TraitsExp) exp.syntaxCopy();
+        tt = new TypeTraits(loc, te);


Declare tt here, not on line 5250

WalterBright · 2018-03-23T20:36:58Z

src/dmd/astbase.d

        }
    }

+    extern (C++) class TypeTraits : Type


Needs an entry in visitor.d, too.

If i do so i get a conflict with the entry in transitivevisitor.d

WalterBright · 2018-03-23T20:48:48Z

src/dmd/parse.d

            t = parseVector();
            break;

+        case TOK.traits:


Also needs entry in isBasicType(), parseMixin(), parseStatement(), parsePrimaryExp() and parseUnaryExp(). Basically, just look at where TOK.typeof_ appears, which is very similar.

WalterBright · 2018-03-23T20:50:08Z

I approve of the concept, but this needs much work. It'll need 100% test coverage. It also needs a changelog entry, and a spec change.

ghost · 2018-04-18T22:56:18Z

I give the hand if someone wants to continue. It's not too hard to finish but i don't want to work on this anymore.

TurkeyMan · 2018-05-07T18:21:37Z

Is there anyone that knows how to kick this across the line, or is this dead? :(

ghost · 2018-11-10T17:55:13Z

This was almost finished actually, the second part didn't require to add a new AST node.
Unfortunately i have to open another PR because i have forced pushed.

ghost requested a review from WalterBright as a code owner March 14, 2018 12:08

dlang-bot added the Review:WIP Work In Progress - not ready for review or pulling label Mar 14, 2018

dlang-bot added the Severity:Enhancement label Mar 14, 2018

jacob-carlborg reviewed Mar 14, 2018

View reviewed changes

jacob-carlborg reviewed Mar 15, 2018

View reviewed changes

RazvanN7 requested changes Mar 16, 2018

View reviewed changes

WalterBright reviewed Mar 23, 2018

View reviewed changes

src/dmd/mtype.d Outdated

Tvector,

Tint128,

Tuns128,

Copy link

Member

WalterBright Mar 23, 2018

Choose a reason for hiding this comment

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

no purpose to blank line

WalterBright reviewed Mar 23, 2018

View reviewed changes

wilzbach added the needs dlang.org PR label Mar 23, 2018

WalterBright reviewed Mar 23, 2018

View reviewed changes

src/dmd/mtype.d Outdated

{

this.loc = loc;

this.exp = exp;

super(Ttraits);

Copy link

Member

WalterBright Mar 23, 2018

Choose a reason for hiding this comment

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

super call should go first.

WalterBright reviewed Mar 23, 2018

View reviewed changes

JinShil added Review:Needs Changelog A changelog entry needs to be added to /changelog Review:Needs Spec PR A PR updating the language specification needs to be submitted to dlang.org and removed needs dlang.org PR labels Mar 23, 2018

fix issue 7804 - Allow __traits(getMember) as a BasicType

fc9b9f2

ghost closed this Apr 18, 2018

ghost mentioned this pull request Nov 10, 2018

fix issue 7804 - Allow __traits(getMember) as a BasicType #8938

Merged

ghost mentioned this pull request Nov 18, 2018

fixup for enhancement 7804 - allow __traits(getMember) as basic type #8945

Merged

This pull request was closed.

Uh oh!

[WIP] fix issue 7804 - Allow __traits(getMember) as a BasicType #8031

[WIP] fix issue 7804 - Allow __traits(getMember) as a BasicType #8031

Uh oh!

Conversation

ghost commented Mar 14, 2018 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dlang-bot commented Mar 14, 2018

Bugzilla references

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost Mar 14, 2018 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndrejMitrovic commented Mar 14, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RazvanN7 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Mar 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilzbach Mar 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilzbach commented Mar 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WalterBright commented Mar 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Apr 18, 2018 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TurkeyMan commented May 7, 2018

Uh oh!

ghost commented Nov 10, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

ghost commented Mar 14, 2018 •

edited by ghost

Loading

ghost Mar 14, 2018 •

edited by ghost

Loading

wilzbach Mar 23, 2018 •

edited

Loading

WalterBright commented Mar 23, 2018 •

edited

Loading

ghost commented Apr 18, 2018 •

edited by ghost

Loading