Bug 2954 – [tdpl] Allow to set associative array key value only using expression AA key type is constructable from

This program compiles and prints "Abc": import std.stdio; void main() { uint[string] hash; char[] a = "abc".dup; hash[a] = 42; a[0] = 'A'; writeln(hash.keys); } It should not compile because char[] is obviously not convertible to string. By this I am also reiterating the necessity to make associative arrays a true library type and have the compiler rewrite the literals and the type name to use that library type.

This test case, from bug 1934, is part of the same issue: index expressions for AAs don't have proper type checking. In the case below, it's not converting the string literal into a char[3], and consequently, bad code generation results. Both asserts fail. void main() { char[char[3]] ac; char[3] c = "abc"; ac["abc"]='a'; assert(ac[c]=='a'); char[dchar[3]] ad; dchar[3] d = "abc"d; ad["abc"d]='a'; assert(ad[d]=='a'); }

http://www.dsource.org/projects/dmd/changeset/749 This fixes Andrei's bug, but Don's is a different bug.

The fix for bug 2684 was incorrect; it was too general. We can't skip the implicit conversion in all cases where the arrays are equality comparable. For example, this code: char[char[3]] ac; dchar[3] d = "abc"d; ac[d] = 'w'; gives a poor error message: bug.d(6): Error: array equality comparison type mismatch, dchar[3u] vs char [3u] instead of "cannot implicitly convert" PATCH: (1) Add this function to cast.c /*********************************** * See if both types are arrays that can be compared * for equality without any casting. Return !=0 if so. * This is to enable comparing things like an immutable * array with a mutable one. */ int arrayTypeCompatibleWithoutCasting(Loc loc, Type *t1, Type *t2) { t1 = t1->toBasetype(); t2 = t2->toBasetype(); if ((t1->ty == Tarray || t1->ty == Tsarray || t1->ty == Tpointer) && t2->ty == t1->ty) { if (t1->nextOf()->implicitConvTo(t2->nextOf()) >= MATCHconst || t2->nextOf()->implicitConvTo(t1->nextOf()) >= MATCHconst) return 1; } return 0; } (2) expression.c line 8580 case Taarray: { TypeAArray *taa = (TypeAArray *)t1; + /* We can skip the implicit conversion if they differ only by + * constness (Bugzilla 2684, see also bug 2954b) + */ + if (!arrayTypeCompatibleWithoutCasting(e2->loc, e2->type, taa->index) ) - if (!arrayTypeCompatible(e2->loc, e2->type, taa->index)) { e2 = e2->implicitCastTo(sc, taa->index); // type checking } type = taa->next;

The fix still isn't right, as it fails at compile time on line 10.

(In reply to comment #4) > The fix still isn't right, as it fails at compile time on line 10. It works for me. Hmm. I think this might also be relying on the patch for bug 5218, which I still have active in my local copy. Sorry about that -- it needs to be included as well.

(In reply to comment #5) > I think this might also be relying on the patch for bug 5218, which I still > have active in my local copy. Sorry about that -- it needs to be included as > well. The patch for bug 5218 enables it to compile, but it fails at runtime with: core.exception.RangeError@test(6): Range violation

Why is it marked as fixed? If we are talking about original testcase, just replace `hash[a]` with `const ca = a; hash[ca]` and the program will work as in description. I see no fundamental difference here. Currently implemented "fix": the compiler checks whether a key as an array and, if it is, checks whether its elements are mutable, if mutable, it complains "...can only be assigned values with immutable keys" (note even the error message is incorrect, not-mutable check and `immutable` in error). IMO, the issue is associative arrays accept non-`immutable` keys and those keys can later be changed. E.g. keys of pointers, classes, associative arrays, and structs/unions with mutable indirection. The whole issue is terrible and current inconsistent compiler behaviour with incorrect error message make situation even worse. The only visible solution is to disallow any associative array element set except with immutable key.

Thanks, indeed it wasn't fixed. Current test code: import std.stdio; void main() { uint[string] hash; char[] a = "abc".dup; const ca = a; hash[ca] = 42; a[0] = 'A'; writeln(hash.keys); } Accessing lvalues in a hash table must be done with a type assignable to the key type. Rvalue lookup may be done with types only comparable t the key type.

(In reply to comment #9) > ... > Accessing lvalues in a hash table must be done with a type assignable to the > key type. Rvalue lookup may be done with types only comparable t the key type. What does it mean "Accessing lvalues" and "Rvalue lookup"? It sounds like a meaningless terminology mix for me. Anyway ability to set `V[K]` AA key using `expr` should compile iff `K x = expr` compiles i.e. `K` is constructable using `expr`. As for getting AA keys we have a complete mess too so I filed Issue 12492. Also I changed this issue title to be precise.

Which of the following cases should/shouldn't be valid? Case 1: int[string] aa; char[] key; aa[key] = 1; Case 2: int[string] aa; char[] key; if (aa[key] == 1) { ... } auto p = key in aa; It seems to me that we should allow case 2, but prohibit case 1.

(In reply to hsteoh from comment #11) > Which of the following cases should/shouldn't be valid? > > Case 1: > > int[string] aa; > char[] key; > aa[key] = 1; This shouldn't compile, although we ought to make it work in the future (e.g. by defining and using a protocol). The problem with this is efficiency: int[string] aa; char[] key = ...; aa[key.idup] = 1; // Alternatively aa[to!string(key)] = 1; This is correct but less efficient than it could - it compulsively allocates a new string even when not needed (i.e. the key is already there). > Case 2: > > int[string] aa; > char[] key; > if (aa[key] == 1) { ... } > auto p = key in aa; > > > It seems to me that we should allow case 2, but prohibit case 1. Yah, lookup should always work with any type that allows comparison.

Bump. A user in Learn recently ran into this (even though the thread itself is about a different question): https://forum.dlang.org/thread/[email protected] The AA's indexing operations should: 1) If the key type is a pointer or a class instance, only accept pointers to immutable and immutable references respectively. 2) If the key type is an array, accept non-immutable arrays, but .idup them if the operation results in insertion (i.e. [], require(), update()) Side observation: what is this 'blocker' bug blocking, exactly? It's approaching it's 10th birthday...

Look at the history. Apparently it blocked issue #1934, but that has somehow been fixed since, so this bug isn't a blocker for it after all? In any case, yeah, this is a stupid long-standing bug that has taken far too long and still no fix in sight. :-/

This oddity just came up with a related bug in std.net.curl (to be fixed shortly). I can't believe this is still a problem, almost 15 years later! For my 2 cents: 1. aa[idx] = value; should only accept idx that is implicitly convertible to the key type. 2. Other operations such as `idx in aa` or `aa[idx]` (which are not setting values/keys) should be fine as long as the key type can convert to the idx. We also lack some sort of mechanism to say "look up using this key, and if not present, use this *conversion routine* to convert the key before assigning the new value". `aa.update` only allows different paths for the value. But maybe this is such a niche problem that it's not worth adding a mechanism.

Niche or not, this problem ought to be fixed. It's been how long now, and we still haven't gotten our act together on AA's. Lookups should be permissive and allow types that are comparable with the key type (e.g., using a char[] to lookup a T[string] should be fine). Assignment, however, must be much stricter because of aliasing issues; it should only allow types implicitly convertible to the key type (it's illegal to assign a char[] key to T[string], because if the key is subsequently modified via the mutable reference, the AA breaks.)

(In reply to hsteoh from comment #16) > Niche or not, this problem ought to be fixed. It's been how long now, and we > still haven't gotten our act together on AA's. You may misunderstand, what I'm talking about is an update-like function, but allows you to tell it how to store the key as well as the value. It's not as easy to model as the current `update` function, because on create, you need to give it both key and value. so something like: `void updateWithKey(V[RealKey] aa, RealKey function(LookupKeyType k) makeKeyOnCreate, V function() valueOnCreate, V function(RealKey) valueOnUpdate)` This issue itself is not niche and needs to be fixed as the P1 that it is. I was just talking about a piece of missing functionality to avoid double lookups in this situation (wanting to only do an expensive thing -- e.g. allocate -- when creating a new key/value pair).

THIS ISSUE HAS BEEN MOVED TO GITHUB https://github.com/dlang/dmd/issues/17992 DO NOT COMMENT HERE ANYMORE, NOBODY WILL SEE IT, THIS ISSUE HAS BEEN MOVED TO GITHUB