Skip to content

[tdpl] Multiple alias this#3998

Closed
IgorStepanov wants to merge 1 commit intodlang:masterfrom
IgorStepanov:multiple-alias-this
Closed

[tdpl] Multiple alias this#3998
IgorStepanov wants to merge 1 commit intodlang:masterfrom
IgorStepanov:multiple-alias-this

Conversation

@IgorStepanov
Copy link
Contributor

This PR introduces multiple alias this.
The old alias this implemenation doesn't imply alias this conflicts.
All alias this-es in old implementation are serializable: compiler successively applies a, a->aliasthis, a->aliasthis->aliasthis while right expression is not found.

This implementation allows tree of alias this subtypes.
Type can has a direct "alias this"-es and "alias this"-es, inherited from base classes/interfaces. Thus, there can be conflict between several matching "alias this"-es.
In common case compile should raise error if several "alias this"-es have found.
However, in the next situation conflict can be resolved:

struct Test9a
{
    int a;
    alias a this;
}

struct Test9b
{
    int a;
    alias a this;
}

struct Test9
{
    Test9a a;
    Test9b b;
    int c;
    alias a this;
    alias b this;
    alias c this;
}

void test()
{
    Test9 t9;
    int a = t9;
}

When we try to cast Test9 to int we can find several variants: t9.a.a, t9.b.a, t9.c.
However, t9.c is preferred, because Test9 author is explicitly pointed out that Test9 should be converted to int through c field. This magic works only if we have exact match. short a = t9; causes error.

This PR have a one limitation: it doesn't support multiple tuple alias this. It don't clearly understand how them should works. Multiple tuple alias this can be implemented in future in a separate PR.

@IgorStepanov
Copy link
Contributor Author

Strage bug on Win64. I can't repeat it on Linux64.
I will try to fix it on windows for a few hours. Sorry.

@MetaLang
Copy link
Member

If this PR is accepted, you will also be able to claim this bounty: https://www.bountysource.com/issues/1375081-tdpl-there-can-be-only-one-alias-this

@schuetzm
Copy link
Contributor

Timon Gehr suggested opThis as an alternative to alias this, you might want to have a look:
http://forum.dlang.org/thread/xjevtasyafnkvusprldp@forum.dlang.org?page=2#post-ld5c9r:2418lt:241:40digitalmars.com

But this debate was merely about syntax, and I don't see how there there could then be multiple opThis's, because it's not possible to overload on the return type.

@schuetzm
Copy link
Contributor

You write: "This magic works only if we have exact match." However, I think implicitly convertible types should be allowed, too. They work with the current implementation.

The documentation also requires it to be an error if multiple alias this's are eligible. Does your implementation do this?

struct S {
    int i;
    short s;
    alias i this;
    alias s this;
}

S s;
int a = s;       // ERROR: both i and s are eligible
short b = s;     // OK: only s comes into question

@IgorStepanov
Copy link
Contributor Author

I think implicitly convertible types should be allowed, too. They work with the current implementation.

They allowed, but direct alias this hides base-types aliasthises only if match is exact.
I made a mistake in the above example, but the example of the test is correct:

struct Test9a
{
    short a;
    string b;
    double d;
    alias a this;
    alias b this;
    alias d this;
}

struct Test9b
{
    int a;
    string b;
    alias a this;
    alias b this;
}

struct Test9
{
    Test9a a;
    Test9b b;
    int c;
    alias a this;
    alias b this;
    alias c this;
}

void test9()
{
    Test9 t9;
    t9.a.a = 1;
    t9.a.b = "1";
    t9.b.a = 2;
    t9.b.b = "2";
    t9.c = 3;

    string s = t9; //Wrong: t9.a.b vs t9.b.b conflict
    int a = t9;      // OK. will return t9.c (because t.c is direct alias this and exactly casted)
    long b = t9;   //Wrong: t9.c doesn't exactly casted to long and conflicts with t9.a.a and t9.b.a
    double c = t9; //Wrong.
}

However the next example are works:

struct Test9a
{
    short a;
    alias a this;
}

struct Test9b
{
    short a;
}

struct Test9
{
    Test9a a;
    Test9b b;
    short c;
    alias a this;
    alias b this;
    alias c this;
}

void test9()
{
    Test9 t9;
    t9.a.a = 1;
    t9.a.b = "1";
    t9.b.a = 2;
    t9.b.b = "2";
    t9.c = 3;

    int x = t9; //Ok, because all acceptable aliasthises have a same type (short) and direct aliasthis hides basetypes aliasthises.
}

The documentation also requires it to be an error if multiple alias this's are eligible. Does your implementation do this?

If I don't create mistake, this should works as you say. I'll add it to tests.

@IgorStepanov
Copy link
Contributor Author

If this PR is accepted, you will also be able to claim this bounty: https://www.bountysource.com/issues/1375081-tdpl-there-can-be-only-one-alias-this

Good news. Thanks =)

@IgorStepanov IgorStepanov force-pushed the multiple-alias-this branch 3 times, most recently from ab5aa74 to 0515fd8 Compare September 16, 2014 23:19
@IgorStepanov
Copy link
Contributor Author

All green. I've moved local struct-member static functions to global scope and it starts to work on windows.

The documentation also requires it to be an error if multiple alias this's are eligible. Does your implementation do this?

Yes, my implementation do it as you say. I've added a corresponding test.

src/aggregate.h Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use aliasThisSymbols or something. thises is awkward.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. Done.

@IgorStepanov IgorStepanov force-pushed the multiple-alias-this branch 2 times, most recently from f2dd288 to 7497d1a Compare September 19, 2014 18:33
@IgorStepanov
Copy link
Contributor Author

Ping

@quickfur
Copy link
Member

Wow. I have been waiting for this feature for a long time! Can't wait to see this merged. Ping @WalterBright ?

@9rnsr
Copy link
Contributor

9rnsr commented Sep 22, 2014

@IgorStepanov I'm not reviewing the code yet, but the atXXX functions looks very similar. Can't you merge them?

@IgorStepanov
Copy link
Contributor Author

I'm not reviewing the code yet, but the atXXX functions looks very similar. Can't you merge them?

Done

@IgorStepanov
Copy link
Contributor Author

@andralex Ping. Please comment the tests and conflict resolving semantic.

@IgorStepanov
Copy link
Contributor Author

@andralex ping

@MartinNowak
Copy link
Member

I don't see a strong use-case for the ambiguity resolution. So instead of adding a special rule, making the feature/language more complex, I would stick to the existing alias overload = func rule, i.e. it adds a function as overload and ambiguity is a failure.

@MartinNowak
Copy link
Member

In C++ there seems to be such a precedence rule for multiple inheritance, but GCC warns about it by default and clang just treats it as error.

@IgorStepanov
Copy link
Contributor Author

I don't see a strong use-case for the ambiguity resolution.

Example:

struct BigUint
{
   //fields, methods
   @property real getReal();
   alias getReal this;
}

struct BigInt
{ 
   BigUint data;
   int sign;
   //....
   @property real getReal(); same with BigUint.getReal but add a sign to value
   alias getReal this;
   alias data this;
}

struct BigDecimal
{
  BigInt integral;
  uint shift;
   //...
   @property real getReal();
   @property BigInt getBigInt(); //make check, decimal shift and return
   alias getReal this;
   alias getBigInt this;
}

May be this example is not best, but it reflect the idea. Base type declares that it is subtype of T, derived types support this subtyping, but add own behaviour.
Another example: root type in "alias-this"-hierarchy declarrs that it is subtype of PersistentObject.
PersistentObject is a object, which knows how to store another object to DB/file

struct Employee
{
    //
    @property PersistentObject getPO(); // save the Employee fields
    alias getPO this;
}

struct Manager
{
    Employee empl;
    alias empl this;
    //manager-specific data
    @property PersistentObject getPO(); // also save the manager-specific fields
    alias getPO this;
}

struct DB
{
   static void save(PersistentObject);
}
Employee e = ...
Manager m = ...

DB.save(e); //call Employee.getPO
DB.save(m); //call Manager.getPO

@IgorStepanov
Copy link
Contributor Author

Alias this is more then C++ multiple inheritance. And it can have semantic, different with C++ multiple inheritance.
Anyway, it's not to hard change current behaviour and I call all people to join to disscussion. BTW, in n.g. thread nobody comment or protest against this behaviour. :-(

Copy link
Contributor

Choose a reason for hiding this comment

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

a.a.bar and a.b.bar

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@IgorStepanov IgorStepanov force-pushed the multiple-alias-this branch from f7d6a99 to 2e82d1a Compare June 21, 2015 02:48
@IgorStepanov
Copy link
Contributor Author

@WalterBright @andralex
If you do not go into the fine details, have you fundamental objections aganist this implementation?
I've explained, why I've done so. I have something wrong?

@IgorStepanov
Copy link
Contributor Author

ping

@deadalnix
Copy link
Contributor

Ping ? I need this. Please @andralex and @WalterBright either merge this or propose actionable items to make this move forward. Right now there is not much to do to have this move forward and that is a problem.

@IgorStepanov
Copy link
Contributor Author

@andralex AFAIR, you planned to merge this until Jul, 8. Four days remaining:)

@IgorStepanov IgorStepanov force-pushed the multiple-alias-this branch from 2e82d1a to be75d6a Compare July 6, 2015 11:46
Copy link
Member

Choose a reason for hiding this comment

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

Errors shouldn't have any capitals in the message, only symbol names have that privilege. ;)

@ibuclaw
Copy link
Member

ibuclaw commented Jul 6, 2015

I agree with @WalterBright and @andralex - there definitely appears to be too much going on, some of which looks like needless noise - such as moving entire functions from expression.c to aliasthis.c with no notable benefit - some of which looks like overcomplicating what should be a modest addition.

When looking around at some of the areas that need changing, I was rather taken aback by the spaghetti code used by the existing alias this implementation. It must be said that the current design is terrible:

Lagain:
    /* 165 lines of code later after checking all other options to resolve the symbol. */
    if (ad && ad->alilasthis)
    {
        e1 = resolveAliasThis(sc, e1);
        goto Lagain;
    }

My feeling is that we must first untangle this unwieldy flow of code, only then will it become a "modest addition".

@IgorStepanov
Copy link
Contributor Author

@andralex Hello. I was out and will nd I will be absent for some time becauseI'm busy with graduate work at the university and is very busy at the job. I'll return to this work and other D works after a couple of months. Sorry. BTW, I have some ideas about this implementation.

@IgorStepanov
Copy link
Contributor Author

@yebblies @andralex
Hello. Sorry for a long non-availability.
I plan to start revival of this to February.
As I see, D frontend has been moved from cpp to D since the my last activity.
Have you any suggestion, how I may easy move this PR to D using conversion utilite?
What limitation of a frontend code in D?
Can I use foreach, AA or simple dynamic arrays, phobos?

@schuetzm
Copy link
Contributor

@IgorStepanov Welcome back!

There used to be a script for automatic conversion, but I can't find it anymore. There's also this page on the Wiki about porting PRs to the D version. I guess it's largely no longer relevant now (e.g. magicport.json is gone).

As for which D constructs are allowed: foreach is actually preferred over for, not sure about AAs and dynamic arrays. Phobos however isn't accepted.

@IgorStepanov
Copy link
Contributor Author

Ok. AA may be usefull if it acceptable.
This (C++) version has a magicport staff. I guess, I need to run magicport manually, get .d files and after that, try to merge with current master.
@yebblies can you write command to do magicporting?

@yebblies
Copy link
Contributor

The procedure in #4922 should work to convert this, once it's rebased onto the last C++ commit using the procedure from the wiki page.

Phobos and the GC are currently being avoided, while some code has been refactored to use foreach. auto and scope delegates.

I would advise doing a direct conversion before any refactoring.

@IgorStepanov
Copy link
Contributor Author

Thanks. Looks simpler that I expected. Will do it.

@adamdruppe
Copy link
Contributor

How is this going?

@lesderid
Copy link
Contributor

lesderid commented Sep 7, 2016

Any update on this?

@wilzbach
Copy link
Contributor

In the State of D survey, this was among the top five missed language features

image

https://rawgit.com/wilzbach/state-of-d/master/report.html

As this is still based on the C++ frontend, it might take a serious effort to revive it.
Though there's some automation - see #4922 (comment)

@ibuclaw
Copy link
Member

ibuclaw commented Mar 16, 2018

This implementation by and large was rejected, as we think that multiple alias this shouldn't be a big change to add support for.

Preliminary PRs may need to be made in order to make the existing alias this support grokable though. Which is probably why this ended up to unwieldy.

@PetarKirov
Copy link
Member

Closing as #8378 superseded this PR.

@PetarKirov PetarKirov closed this Aug 31, 2018
@PetarKirov PetarKirov added the Review:Phantom Zone Has value/information for future work, but closed for now label Aug 31, 2018
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.