Issue 6178 - Struct inside the AA are not init correctly

[9rnsr]

http://d.puremagic.com/issues/show_bug.cgi?id=6178

If the AA value setting invokes opAsign method, it would need additional runtime const to distinguish construction and assignment.

Following is the implemented behavior.

1. If the setting is identity assignment (the AA value type is equal to toe rhs type):

   struct S1 { void opAssign(S1 rhs) {} }
   S1[int] aa1;
   aa1[1] = S1();  // <--

   1. If the key doesn't exist in aa yet, so it should work as initializing.
   2. If the key already exists in aa, so it should work as assignment (== opAssign call).

2. If the setting is not identity assignment (the AA value type is not equal to toe rhs type):

   struct S2 {
      this(int val) {}
      void opAssign(int val) {} }
   S2[int] aa1;
   aa[1] = 1;  // <--

   1. If the key doesn"t exist in aa yet, it should work as initializing, with implicit constructor call.

      S2 tmp = 1;  // Rewrite as: S2 tmp = S2(1);
      aa[1] = tmp;  // blit copy

   2. If the key already exists in aa, so it should work as assignment (== opAssign call).

The case #2 is necessary to allow using @disable this() structs as AA values.

[9rnsr]

This compiler change requires small phobos fix: dlang/phobos#1554

[ghost]

If the setting is not identity assignment (the AA value type is not equal to to rhs type):
If the key doesn"t exist in aa yet, it should work as initializing, with implicit constructor call.

I didn't even know this was already calling opAssign. Personally I would get rid of this implicit construction feature altogether, it can cause user bugs too easily:

struct S
{
    // expensive ctor call
    this(int) { }
}

int[int] a1;
S[int] a2;


a1[1] = 1;  // ok
a2[1] = 1;  // oops, meant to use 'a1', but the compiler didn't catch the error

The above should really catch the bug and force you to write a[2] = S(1) if really intended. I don't see what an implicit ctor call buys us except confusion and performance issues. This is a compile-time language that's supposed to catch these sort of issues.

Also, allowing an implicit ctor call becomes a special case for structs, whereas it won't work for classes:

class C
{
    this(int) { }
}

C[int] aa;
aa[1] = 1;  // denied

[9rnsr]

I didn't even know this was already calling opAssign. Personally I would get rid of this implicit construction feature altogether, it can cause user bugs too easily:

The a2[1] = 1; case would be rejected becauseS does not have void opAssing(int) method.

Also, allowing an implicit ctor call becomes a special case for structs, whereas it won't work for classes:

Good point out. I didn't think about class with opAssing case. With current dmd, following code will cause range violation in runtime, because the non-identity assignment will always evaluated as "read index access + call opAssign on the read object".

class C { void opAssign(int) {} }
C[int] aa;
aa[1] = 1;  // range violation

If the struct of the AA value could not perform implicit constructor call, to follow the behavior of the class case would be better. I'll update this PR to introduce the behavior.

---

I designed the implicit constructor call on AA value setting for the value semantics structs, e.g. std.bigint.BigInt.

BigInt[string] aa;
aa["val"] = 1;  // construct BigInt(1) and store it in AA
aa["val"] = 2;  // call opAssign on existing BigInt value in AA

In above case, explicit BigInt construct aa["val"] = BigInt(1); would introduce unnecessary heap allocation. Most of value semantics structs, opAssign is designed to reduce such construction cost.
Therefore, certainly it is a special behavior for structs of AAs, but I still think it is useful in many cases.

[monarchdodra]

@9rnsr : I don't understand what the "alias this" has to do with anything. The way I see it, when you write:

ENTRY[KEY] aa;
aa[key] = value;

One of two things can happen:

1. There is no ENTRY associated with key, in which case, a new ENTRY needs to be constructed from value.
2. There is already an ENTRY associated with key, in which case, value is assigned to that ENTRY.

Given the above, the two requirements become:

1. ENTRY needs to be constructible from the type of value (implicitly or explicitly, that is another question).
2. ENTRY needs to be assignable from the type of value.

In both cases, I don't see how the alias this comes into the equation.

Your code would mean that:

struct T
{
    int i;
    alias i this;
}

void main()
{
    T a = T(5); //Construction OK.
    a = 5; //Asignement OK.
    T[int] aa;
    aa[0] = 5; //Nope, sorry (huh? But why?)
}

This restriction doesn't make sense to me.

[monarchdodra]

In regards to the whole "implicit construction" vs "assignment" issue, I think a point could be that : aa[key] = value can insert a new entry IFF the type of value is the type of key. If value is some "third type", then it can only trigger assignment.

EG:

struct S
{
    int i;
    void opAssign(int){}
}

void main()
{
    S[int] aa;
    aa[0] = S(5); //Fine, insert a new S at location 0.
    aa[0] = 4; //Fine, assign 4 to the entry at 0.
    aa[1] = 1; //Nope, throw an error.
}

This might be breaking change, but it is the only behavior that I think makes sense long term. D does not support implicit construction, so I don't see why AA's should make any exception in that regard.

That might be breaking change though, but it's the only behavior I can see that supports implicit insertion on non-existing, while not tricking users with implicit conversion.

[9rnsr]

One of two things can happen:

1. There is no ENTRY associated with key, in which case, a new ENTRY needs to be constructed from value.
2. There is already an ENTRY associated with key, in which case, value is assigned to that ENTRY.

Right.

Given the above, the two requirements become:

1. ENTRY needs to be constructible from the type of value (implicitly or explicitly, that is another question).
2. ENTRY needs to be assignable from the type of value.

Correct. And "assignment through alias this" cannot satisfy the condition #1.
The reason is just same as why following code won't be accepted.

struct T { string s; int i; alias i this; }
T t = 1;   // t cannot be initialized through alias this, because s would be left in uninitialized status

[9rnsr]

@monarchdodra D already supports implicit construction when the declared variable type is explicitly give.

struct S { this(int) {} }
S s = 1;  // this(int) is invoked implicitly

I had thought about this problem in long term. My conclusion is, the AA value setting (aa[key] = value;) is one of the anomaly in D syntax/semantic design. As you say, it invokes both initialization and assignment in one expression, depends on the runtime context. Therefore I think its behavior will also become peculiar.

[monarchdodra]

Correct. And "assignment through alias this" cannot satisfy the condition #1.
The reason is just same as why following code won't be accepted.

struct T { string s; int i; alias i this; }
T t = 1;   // t cannot be initialized through alias this, because s would be left in uninitialized status

Right, but what I'm saying is that this has nothing to do with alias this. T is simply not constructible form int, end of story. I don't see your example as being any different from:

struct T { string s; int i; void opAssign(int); }
T t = 1;

[9rnsr]

Updated PR for the case that the AA value struct is not constructible from foreign value.

struct S { int n; void opAssign(int) {} }
S[int] aa;
aa[1] = 2;  // 

In above, aa[1] = 2; is always rewritten as aa[1].opAssign(2);, and if the key does not exist in aa, it will throw RangeError in runtime.

[9rnsr]

@monarchdodra See the latest update.

[ghost]

struct S { this(int) {} }
S s = 1;  // this(int) is invoked implicitly

I forgot about that, and I dislike that feature. It does however allow you to do cute things, for example:

struct S { this(int, int) { } }
struct F { int x, y;  }

void main()
{
    F f = { 1, 2 };
    S s = f.tupleof;
}

I don't know if anyone ever used it like that, I mean it's not that hard to use S(f.tupleof).

[monarchdodra]

@9rnsr : I pulled your phobos fix, because I thought it was cleaner that way anyways.

I still don't understand what "alias this" has to do with AA. An entry needs to be constructible from "rhs", and/or be assignable. I really don't see how/why an alias this would interfere, and I think it would be wrong for the compiler to even mention it.

If AA was a library type (which, arguably, it should be, and if not, it not, it needs to at least be able to behave that way), then it might look something like this:

struct AA(Key, T)
{
    auto opIndexAssign(Key key, U value)
    {
        if (keyIsInThis())
            getRefValueAtKey(key) = value; //[HERE]
        else
        {
            //(Up to debate on exact behavior we want)
            static if (is(Unqual!T == Unqual!U)
                emplaceAtKey(key, value);
            else
                throwKeyNotFoundError();
        }
    }
}

The only place I see where alias this might interfere is at [HERE], and even then, I don't see why it isn't a legal call to do an assignment via said alias this?

[9rnsr]

@monarchdodra OK, I changed my mind, and organized essential rewriting rules.

1. For identity assignment:
   • If the key does not yet exist in AA, the assignment would become construction.
   • Otherwise, the assignment would become normal assignment.
2. For non-identity assignment:

   • If opAssign or alias this could be used:

     The assignment would become normal assignment. If the key does not exist yet, the indexing access throws RangeError. However, if implicit constructor call is possible with the rhs, the value is used for construction. Then RangeError won't be thrown.

   • Otherwise, "type mismatch" error will be reported in compilation.

I updated commits based on the modified rule.

[monarchdodra]

@monarchdodra OK, I changed my mind, and organized essential rewriting rules.

I like it, but there's still one thing I'm unsure of:

Your code seems to to be creating new keys as soon as an Entry can be constructed from a rhs (in which case an implicit construction happens). I think that is very lenient, and the condition should just be if (is(Arg : Entry))

EG:

struct S {int i; alias i this;}

void main()
{
    S[int] aa;
    aa[1] = 1; //Should this throw?
}

Your use cases seem to imply that this would work (via construction)? I don't think it should create a new Key. int is not S. My use case "clearly" (IMO) request assignment, not insertion. But maybe that would break existing code? I really don't know at this point.

An alternative behavior would be in case of assignment, to insert T.init first, and then (always) assign over that (Note: I think this is the current behavior, or at least, the "supposed" current behavior? That would make more sense to me anyways). In any case, I still think that under no circumstances should an AA do implicit construction.

[9rnsr]

Your use cases seem to imply that this would work (via construction)?

No. alias i this; in S cannot support constructing S from int value (To do it, you need to add conversion constructor this(int) {}). Therefore the line aa[1] = 1; is always invoke normal assignment. If the key 1 does not exist in aa, it will throw RangeError in runtime.

An alternative behavior would be in case of assignment, to insert T.init first, and then (always) assign over that

It's wrong. If the AA value type T is nested or @disable this() struct, T.init will supply (phisically and/or logically) invalid object. Therefore that won't work in safe code.

[monarchdodra]

It's wrong. If the AA value type T is nested or @disable this() struct, T.init will supply (phisically and/or logically) invalid object. Therefore that won't work in safe code.

Right. I'm not a fan of this solution anyways, so that works for me.

It's wrong. If the AA value type T is nested or @disable this() struct, T.init will supply (phisically and/or logically) invalid object. Therefore that won't work in safe code.

That's why my struct has a int i; member. S is constructible from an int, eg: S s = S(5); (ins't it)? Or does your change specifically require the existence of an actual constructor?

Ether way, I still find that:

struct S
{
    this(int){}
    int i;
    alias i this;
}

void main()
{
    S[int] aa;
    aa[1] = 1; //This doesn't throw?
}

I still find it weird here that the AA takes the liberty of constructing an item and inserting it into a new key.

[9rnsr]

That's why my struct has a int i; member. S is constructible from an int, eg: S s = S(5); (ins't it)?

You use struct literal syntax S(5) there. I meant that S is not implicitly constructible from int, eg: S s = 5;

I still find it weird here that the AA takes the liberty of constructing an item and inserting it into a new key.

I believe the behavior is useful, eg for BigInt or other value semantics structs.

[monarchdodra]

Alright. I can't say I'm a fan of "implicit ctor", but I get your point of view.

In any case, this fixes and formalizes behavior, so I'm definitely OK with this pull. Your unittests make sense to me, and behave in ways that make sense.

So, LGTM.

[9rnsr]

Added test case for issue 10970 with small glue layer bug fix.

[9rnsr]

@WalterBright and @andralex , could you review this? This PR determines AA value setting behavior, therefore your reviewing would be necessary.

[9rnsr]

Add test case for issue 10595.

[denis-sh]

@9rnsr, may I ask you to also add docs? Missing docs issue: Issue 11104.

[9rnsr]

@denis-sh Thanks. I'll fix the issue.

[denis-sh]

@9rnsr, I also have a [silly?] question for a long time. Is it supposed there is a single implicit constructable rule which is used in all these cases:

T t = x;
f(x); // void f(T);
S s = S(x); // struct S { T t; }

except the first case is rewritten to T(x) in case !is(Unqual!T == Unqual!(typeof(x)))
I'm not sure how to formulate the issue but I suppose the documentation lacks exact implicit construction rules. If so, could you file an issue about it?

[MartinNowak]

One reason why opIndexAssign has this dual behavior is to avoid double lookup.
Now this pull adds a second lookup and we should try to avoid this.
How about adding a bool flag to _aaGetX which returns whether the key was present or not?