Bug 13114 – old opCmp requirement for AA keys should be detected for classes

Like https://issues.dlang.org/show_bug.cgi?id=13074, the same problem exists for classes. For example: import std.stdio; import std.conv; class C { this(int x, int y) pure {this.x = x; this.y = y;} int x; int y; //override bool opEquals(Object other) { if(auto o = cast(C)other) {return x == o.x;} return false;} override int opCmp(Object other) { if(auto o = cast(C)other) {return x < o.x ? -1 : x > o.x ? 1 : 0;} return -1;} override hash_t toHash() const { return x; } override string toString() const { return "{" ~ x.to!string ~ ", " ~ y.to!string ~ "}";} } void main() { const c1 = new C(1,1); const c2 = new C(1,2); const c3 = new C(2,1); const c4 = new C(2,2); bool[const(C)] arr; arr[c1] = true; arr[c2] = true; arr[c3] = true; arr[c4] = true; writeln(arr); } Under 2.065, this prints: [{1, 1}:true, {2, 1}:true] Under git head (even after the fix for issue 13074), this compiles and prints: [{1, 1}:true, {1, 2}:true, {2, 1}:true, {2, 2}:true] Note, there are going to be cases that we cannot detect. For example, bool[Object] would compile perfectly fine, but if you used a C instance as a key, it would have the same problem. For that, we would need a runtime check. I don't know if it's worth making a fix for that, it would really entail never allowing overriding opCmp without also overriding opEquals. That may be a bridge too far. But we should still fix the easy case (I hope it's easy!)

Further update, I think there is a valid case for allowing redefining of opCmp and not opEquals -- if you are not changing the equality portion of opCmp. For instance: class C { this(int x, int y) pure {this.x = x; this.y = y;} int x; int y; override bool opEquals(Object other) { if(auto o = cast(C)other) {return x == o.x;} return false;} override int opCmp(Object other) { if(auto o = cast(C)other) {return x < o.x ? -1 : x > o.x ? 1 : 0;} return -1;} override hash_t toHash() const { return x; } override string toString() const { return "{" ~ x.to!string ~ ", " ~ y.to!string ~ "}";} } class ReverseC : C { override int opCmp(Object other) { if(auto o = cast(C)other) {return x > o.x ? -1 : x < o.x ? 1 : 0;} return -1;} } ReverseC should be able to be an AA key, because the opCmp calculation for equality did not change. This can be an AA key under 2.065, and under the new compiler/druntime. I think we cannot actually fix this issue. I don't know what to do here. CC'ing major players on this issue to make sure it gets noticed.

> I think we cannot actually fix this issue. I don't know what to do here. > CC'ing major players on this issue to make sure it gets noticed. Compiler cannot detect all invalid cases. We should give up that do things perfectly. https://github.com/D-Programming-Language/dmd/pull/3757

Commits pushed to master at https://github.com/D-Programming-Language/dmd https://github.com/D-Programming-Language/dmd/commit/411fbfea24e5a3c8f8833ee62a6bc5409c7cac41 fix Issue 13114 - old opCmp requirement for AA keys should be detected for classes https://github.com/D-Programming-Language/dmd/commit/1efe1ad230aae55b341a81c2418596c2e636d0d4 Merge pull request #3757 from 9rnsr/fix13114 Issue 13114 - old opCmp requirement for AA keys should be detected for classes

Commit pushed to 2.066 at https://github.com/D-Programming-Language/dmd https://github.com/D-Programming-Language/dmd/commit/b742abf6321f3fedc11d8c5f87b3b6310695a228 Merge pull request #3757 from 9rnsr/fix13114 Issue 13114 - old opCmp requirement for AA keys should be detected for classes

Commit pushed to master at https://github.com/D-Programming-Language/dmd https://github.com/D-Programming-Language/dmd/commit/b742abf6321f3fedc11d8c5f87b3b6310695a228 Merge pull request #3757 from 9rnsr/fix13114