std.AutoHashMap allows string keys

[SpexGuy]

I think this is a bug, feel free to close if this is intentional. The autoHash function contains this check, which seems like it is meant to disallow string keys in AutoHashMap:

if (comptime meta.trait.isSlice(Key)) {
    comptime assert(@hasDecl(std, "StringHashMap")); // detect when the following message needs updated
    const extra_help = if (Key == []const u8)
        " Consider std.StringHashMap for hashing the contents of []const u8."
    else
        "";

    @compileError("std.auto_hash.autoHash does not allow slices (here " ++ @typeName(Key) ++
        ") because the intent is unclear. Consider using std.auto_hash.hash or providing your own hash function instead." ++
        extra_help);
    }
}

However, before running this code, it does this check:

if (comptime trait.hasUniqueRepresentation(K)) {
    return Wyhash.hash(0, std.mem.asBytes(&key));
} else {
     var hasher = Wyhash.init(0);
     autoHash(&hasher, key);
     return hasher.final();
}

trait.hasUniqueRepresentation(..) hits this case for slices:

.Pointer => return true,

So autoHash never gets compiled for the case where the key is a string, and the compile error does not trigger.

[andrewrk]

cc @Sahnvour

[Sahnvour]

Seems like a bug indeed.

[SpexGuy]

Is hasUniqueRepresentation supposed to return true for slices? Are they guaranteed to have no padding?

[Sahnvour]

I think it's pretty safe to assume they don't have padding in practice, but afaik there is no guarantee about it.
Either way, the check for []const u8 as keys should be in the AutoHashMap code and not in autoHash.

[andrewrk]

Slices currently do not have well defined memory layout.

[indocomsoft]

Hi! I'm interested in taking this issue. However, I'm not sure if it makes sense to emit @compileError when using []const u8 as K of an AutoHashMap since it is possible the intent is to use the (pointer) identity of the slices as the key. Is there any possible way to generate a warning instead?

[SpexGuy]

There is no way to generate a warning, but I think a compile error is the right thing here. AutoHashMap is just a wrapper function which picks the hash function automatically and then calls HashMap. For things like slices, where it's ambiguous whether the slice contents should be hashed or the slice pointer and length, the auto-picking functionality should fail and the user should be forced to call HashMap directly with their desired hash function. The error message should point out (as it currently does) that this is the way to work around it if you actually intended to hash the pointer and length.

[indocomsoft]

Ah gotcha, so this issue is actually more that std.AutoHashMap should not allow slices as key. I'll submit a PR in a bit!

[SpexGuy]

Sounds great! Keep in mind that this also applies to struct keys that contain slice fields.

[indocomsoft]

@SpexGuy I tried to change autoHash such that it also rejects struct keys that contain slice fields, but it seems that there are existing code in src/ (I assume the self-hosted compiler) that uses autoHash on structs containing slices: https://gist.github.com/indocomsoft/a29593a6e78408105ed92d843bf45cc4

[Vexu]

That looks wrong, I think you might be hitting #7532.

[indocomsoft]

@Vexu my bad, it appears that I forgot to mark expressions as comptime