Replace AssociativeArray struct with UFCS methods

[yebblies]

No description provided.

[MartinNowak]

Nice.
There is also some work on redoing the templated AA (see here).
I don't think we should merge this before 2.065 is out. It looks somewhat disruptive and might break valid code.

[yebblies]

Shouldn't we merge it right away, since we're at the beginning of a release cycle? And this of course.

[yebblies]

There is also some work on redoing the templated AA (see here).

We've been waiting for the templated AA to be fixed for years. Now that we have UFCS for AAs, we can go back to a sane implementation without breaking code. There is no reason this should be a problem for re-doing the templated version, in fact I suspect it will help.

[MartinNowak]

Yes, it won't be a problem for the templated version, just wanted to get you informed.

[MartinNowak]

Shouldn't we merge it right away, since we're at the beginning of a release cycle? And this of course.

The idea was to focus on stability and release at end of december (Agenda).
While this is a good step forward it looks like it comes with a regression.
I'll review the dmd change tomorrow.

[yebblies]

I think this is important enough that we should do it as soon as possible. It has been far too long already, and a lot of the ctfe stuff that is tested in the dmd pull doesn't currently work. (although some of it may have worked before the templated changes)
What regression? Do you mean 9807? I would consider it fixing an accepts-invalid.
Thanks for taking a look!

[IgorStepanov]

@yebblies Why you want to remove AssociativeArray structure?
I'm working under opposite solution. I want to implement templated AssociativeArray and all those methods (opIndex et c.) using new hashOf implementation #663. However current language state doesn't allow some AA operation on user types. Typical example:

int[string][string] aa;
aa["aaa"]["bbb"] = 42;

Second row add new elem to aa with key "aaa" and add "bbb" to aa["aaa"] right after that. User type can't do this trick now.

Thus we need to stay some _aaXXX functions. I want to add special handle field instead of TypeInfo _keyti to AA implementation, which point to functions table like vtbl. This table unique for different AssociativeArray instances and store _aaXXX like functions.

private struct Ftable
{
   extern(C) void* function getRvalueX(void*, void*);
   //... 
}

struct AssociativeArray(K,V)
{
   V* opBinaryRight(string op)(K key) if(op == "in") // key in aa
   {
      ... 
   }

   extern(C) static void* getRvalueX(void* aa, void* pkey)
   {
       return *cast(K*)pkey in *cast(AssociativeArray*)aa;
   }
   static immutable optable_ = Ftable(&getRvalueX, );

   immutable Ftable* optable = optable_; //really will be in AssociativeArrayImpl 
   AssociativeArrayImpl impl;
}

extern(C) inout(void)* _aaGetRvalueX(inout void* p, in TypeInfo, in size_t, in void* pkey)
{
   return (cast(Ftable*)p).getRvalueX(p, pkey);
}

All those _aaXXX will stay but their bodies will be trivial (one row).
This soultion stay extern(C) interface for compiler but move AA implementation to good typesafe D style. This AA will be able to constructing at compile time and most of operation will be evaluatable without compiler help.
In future we will be able to remove _aaXXX calls generation. I hope, I will show first implementation of this AA at end of this week.
Have you some objections aganist this strategy?

[yebblies]

I'm not against improving the AA situation, but the current implementation is wrong. The compiler should not have two types that are aliased internally. The addition of AssociativeArray was meant to simplify AA code in the compiler, but it instead resulted in a lot of new special cases.

I think it's a far more stable solution to extend AAs via UFCS, as this doesn't violate the type system in the same way. I fully expect all AA magic can be translated to UFCS methods if needed.

Does your new AA implementation have any horrible hacks like the old one? eg *cast(Value[Key]*)(&p);

I don't think your incoming changes should block this. If they are sufficient, this can easily be reverted. I would of course prefer to not bring back any of this half-builtin nonsense.

[ibuclaw]

The compiler should not have two types that are aliased internally.

+1 - and I'd be extremely happy to remove all special casing for this in the backend code. It's not fun at all to run into bugs where you have correct codegen sent from the front-end. :o(

[IgorStepanov]

Does your new AA implementation have any horrible hacks like the old one? eg cast(Value[Key])(&p);

Now I work on independent AssociativeArray. It haven't ugly casts. However I have questions about integration with dmd. Can we set A[B] to full alias AssociativeArray!(A,B)? I mean that the compiler should consider AssociativeArray!(A,B) and A[B] types identical. In that case I can perform all AA built-in properies as trivial user-type methods (At first sight). There is one exception: User type can't perform opIndex like built-in AA. (I've written about it). I'll stay trivial _aaGetX and _aaGetRvalueX while we don't solve this issue.
I hope, I'll create PR for the next two days.

[yebblies]

Having the compiler think AssociativeArray!(A,B) and A[B] are the same is most of the current problem. I'm interested to see how you do it without fudging the type.

[IgorStepanov]

@yebblies See #676 please

[MartinNowak]

Having the compiler think AssociativeArray!(A,B) and A[B] are the same is most of the current problem. I'm interested to see how you do it without fudging the type.

The problem here is, that the compiler treats Value[Key] as special type and some of it's functions as intrinsics.
The idea is that Value[Key] becomes a rewrite for AssociativeArray!(Key, Value) while removing most of the compiler internal logic.

[yebblies]

The problem here is, that the compiler treats Value[Key] as special type and some of it's functions as intrinsics.
The idea is that Value[Key] becomes a rewrite for AssociativeArray!(Key, Value) while removing most of the compiler internal logic.

The issue with this is providing correct error messages. I don't think I've seen a solution to this yet.

[ibuclaw]

@yebblies can you rebase (in your own time, of course :) - think I might try this out in gdc.

[yebblies]

Done.

[MartinNowak]

Breaks cases where the property is called.

int[int] foo;
auto len = foo.length();
auto keys = foo.keys();

[ibuclaw]

@dawgfoto - I never realised that people actually use () for the .length and .keys properties??

[MartinNowak]

The issue with this is providing correct error messages. I don't think I've seen a solution to this yet.

If you say that Value[Key] is syntax sugar for AssociativeArray!(Key, Value) the error message isn't wrong ;).
But maybe we should pretty-print this type in dmd's errror messages.

[yebblies]

If you say that Value[Key] is syntax sugar for AssociativeArray!(Key, Value) the error message isn't wrong ;).
But maybe we should pretty-print this type in dmd's errror messages.

Of course, the spec doesn't say that and nobody expects that. If V[K] was simply an alias then most of these problems would go away. There's more to AA error messages than printing the correct type. Implicit conversions, foreach errors, etc have special cases for AAs.

[MartinNowak]

Can you add a deprecated alias so that the tests pass and code that explicitly used AssociativeArray doesn't break.

alias AssociativeArray(Key, Value) = Value[Key];

[MartinNowak]

Taking a delegate won't work with UFCS, but I guess it has little impact.

[yebblies]

Added alias.

[MartinNowak]

Doesn't work with AA pointers.

int[int]* p;
auto keys = p.keys;

You'll need to add overloads.

Key[] keys(T : Value[Key], Value, Key)(T* aa)
{
    return (*aa).keys();
}

Value[] values(T : Value[Key], Value, Key)(T* aa)
{
    return (*aa).values();
}

Value get(T : Value[Key], Value, Key)(T* aa, Key key, lazy Value defaultValue)
{
    return (*aa).get(key, defaultValue);
}

[MartinNowak]

This will also need a phobos fix (dlang/phobos#1741).

[CyberShadow]

Guys, not sure if this is related but I can't build Druntime from git any more.

src\core\thread.d(2880): Error: not a property this.m_all.keys
src\core\thread.d(2908): Error: not a property this.m_all.keys

Edit: ah, it was caused by an old -property in my build script. If not even Druntime supports it any more, I suggest removing the switch from the compiler entirely.

[denis-sh]

As Martin posted 3 months ago:

Breaks cases where the property is called.

The issue isn't fixed but the pull is merged breaking builds with -property switch as Vladimir noted. Filed regression 12136.

[CyberShadow]

This pull request (or dlang/dmd#2856) introduced a regression:
https://d.puremagic.com/issues/show_bug.cgi?id=12167

[WalterBright]

We need to revert this. We have 3 regressions with AAs in 2.065:

https://d.puremagic.com/issues/show_bug.cgi?id=12167
https://d.puremagic.com/issues/show_bug.cgi?id=12220
https://d.puremagic.com/issues/show_bug.cgi?id=12221

[yebblies]

We need to revert this.

No, we don't. Even with those regressions we're still better off. I will fix those in the next couple of days.

[MartinNowak]

Please let's figure out how to make a smooth transition.
There is way more AA rework planned for 2.066 (http://wiki.dlang.org/Agenda#library_AA) and I'd prefer to finish this within one release cycle to avoid having the UFCS AAs as intermediate step.

[MartinNowak]

We have 3 regressions with AAs in 2.065:

Those are regression in master, nobody was so crazy to merge this into 2.065.

[CyberShadow]

This pull request (or dlang/dmd#2856) introduced a regression:
https://issues.dlang.org/show_bug.cgi?id=14626