@safety problems with JSONToken

[JakobOvrum]

JSONToken.string does not present a memory safe interface, as it may return a dangling reference if the token's kind is not a string.

[s-ludwig]

Do you mean in case assertions are disabled?

[JakobOvrum]

Well, assert is a sanity check for verifying a precondition. That means it's the caller's responsibility to make sure the token is actually a string before accessing the string property; assertion failure means there is a bug in the caller's code. This is not necessarily bad design, but at the very least the precondition must be documented.

I don't think it's a good choice here though, as it precludes @safe. The caller would have to verify memory safety manually and use @trusted:

string safeString(JSONToken token) @trusted
{
    return token.kind == JSONToken.Kind.string? token.string : null;
}

I think a better design is for the accessor properties to validate the input with enforce. This precludes nothrow, but maybe it could be supported with an alternative interface:

JSONToken token;
auto str = token.get!string("default");
assert(str == "default");

Either way I think @safe is more important than nothrow.

What do you think?

[s-ludwig]

I guess it depends on how much assert is assumed to guarantee that the condition is actually met (i.e. is @safe stronger or weaker than assert). Walter recently even had the idea to let the compiler use the condition of an assert statement to draw such conclusions, so that in code like this could be made valid:

int index(int[] array, size_t idx) @safe {
    assert(idx < array.length);
    return array.ptr[idx]; // compiler knows that there is no out-of-bounds access
}

I don't think that this is a good idea, but that's a different topic.

What I used to do in C++ was to write a safe fallback path after an assertion, so that the release build would degrade gracefully:

assert(ptr != NULL);
if (ptr == NULL) { log error and return some default value }

The same could be applied here, too, to make the method a valid @trusted method even in release builds:

assert(m_kind == Kind.string);
return kind == Kind.string ? m_string : JSONString.init;

Of course, converting the assert to an in contract is a good idea in any case. I'll do that now.

I'm not sure about using exceptions. Adding two ways to access the values complicates the interface, but of course it also has its merit. Maybe this would be a good point to raise in a wider community review discussion.