Nullable's `toHash` needs to infer its attributes

[jmdavis]

It looks like someone figured out that hashOf wasn't @safe when dealing with unions with overlapping fields, so it has a deprecation message when using in code marked as @safe which won't be @safe once the deprecation is switched to an error. And with such a type, Nullable's toHash cannot be @safe, but it's currently explicitly marked as @safe. So, Nullable's toHash needs to be fixed so that it infers @safe rather than being explicitly marked with it.

void main()
{
    import std.typecons;

    static struct S
    {
        union
        {
            long foo;
            int[] bar;
        }
    }

    Nullable!S s;
}

[jmdavis]

Actually, looking at this deeper. There are two issues here.

1. Nullable's toHash should be inferring its attributes. There is no guarantee that toHash on the type contained by Nullable works with const, nothrow, or @safe. No explicit attributes should ever have been put on Nullable's toHash.

2. There is currently a workaround inside of Nullable's toHash which uses typeid. However, this is inherently broken, because it bypasses the type's toHash, meaning that any code that triggers it is giving a hash which likely won't match the behavior of that type's opEquals, meaning that by using typeid like this, we are creating a bug for user code by giving it the wrong behavior.

This needs to pass

void main()
{
    import std.typecons;

    static struct S
    {
        int i;

        size_t toHash()
        {
            return 42;
        }
    }

    auto s = nullable(S.init);
    assert(s.toHash() == 42);
}

but it fails, because the hash that's given (at least on my system) is 1390208809.

Now, I'm guessing that this typeid hack was used to try to fix the attribute problem whereT in Nullable!T doesn't have the right attributes on its toHash to work with Nullable's toHash but without breaking code where it's used that's marked as @safe, nothrow, or const. But this "fix" introduced a serious bug into any code which actually uses toHash with Nullable but doesn't have the correct attributes.

Using git blame it looks like the "fix" was for https://issues.dlang.org/show_bug.cgi?id=19197, but issues.dlang.org isn't loading for me right now, and I don't see the associated e-mails for it in my account, so I don't know what was said on it.

Fixing this properly will cause breakage in existing code in the sense that any code which uses a const Nullable!T or Nullable!(const T) (or the same with immutable or inout) with a T.toHash that isn't const or inout will break. And it will cause any code marked with @safe and/or nothrow using Nullable!T where T.toHash isn't @safe/nothrow to break. However, that compilation error will be showing them a bug in their code that they need to fix. And code that isn't marked with @safe or nothrow - or which doesn't use const - which uses a Nullable!T where T.toHash isn't @safe / nothrow / const will simply have a bug fixed, because then its toHash will start working properly, and the lack of attributes on Nullable.toHash won't be a problem for that code, because it wasn't relying on it anyway.

[jmdavis]

And actually, const can't be inferred, so in addition to making it so that the attributes are inferred, we'll need to check in a static if whether hashOf works on the payload when it's const and then provide a const version if it compiles and a mutable version if it does not.

[jmdavis]

Well, I haven't fully figured out the situation here, but it's worse than I thought. Unfortunately, TypeInfo is still a thing, and AAs still use it, and it's really not designed to support templated anything. In the case of TypeInfo_Struct, it has a field called xtoHash, which is a function pointer to the toHash on the user-defined type, meaning that it cannot be templated. And unfortunately, TypeInfo_Struct's getHash will just happily hash the bits of the type if xtoHash is null - and if your toHash doesn't have a signature that which results in xtoHash being set, then you just get a hash of the bits and don't get any sort of error even though toHash has been explicitly declared and is being bypassed.

And to make matters worse, getHash on TypeInfo has the attributes @trusted pure nothrow const, implying that your type's toHash must be at least nothrow, const, and pure and that it will just incorrectly trust your code if it's not @safe. And since it looks like I can remove all of the attributes other than const from Nullable's toHash, and toHash will still be set (some of the time, but not all of the time for some reason; I still need to figure out what's going on there), that indicates that the type system is not necessarily being upheld here. Of course, no attribute should be required for toHash given that it's not reasonable in the general case to impose the restrictions that the various attributes do, but the fact that we currently have TypeInfo, which is a class, involved in this mess instead of having everything involved be templated makes dealing with this correctly hard; hopefully, once the AA implementation is fully templated, we can fix that). At least in the case of pure, TypeInfo's getHash doesn't have it, so in practice, pure probably doesn't matter here, but arguably, for toHash to work fully correctly with TypeInfo's getHash, a struct's toHash needs to be const @safe nothrow like Nullable's toHash currently is - which of course is a problem when the type cannot both have that signature on toHash and make it work correctly. And with Nullable, it can't necessarily work correctly and currently will quite happily give the wrong hash when it can't work correctly.

So, I'm not quite sure what to do here. Long term, if we can get the AA implementation fully templated, then we should be able to have a fully correct toHash on Nullable, but as things stand, I don't see how we can. At best, I think that I can make it have fewer attributes when it won't work correctly with all of const, @safe, and nothrow, but I don't think that I can get rid of const and still have it called from getHash, and while reducing the number of attributes will allow toHash to give the correct result, it will result in getHash violating the type system.