schveiguy / io

Core I/O functionality
Boost Software License 1.0
25 stars 9 forks source link

Fix invalid override for `Throwable.toString` #51

Closed MoonlightSentinel closed 3 years ago

MoonlightSentinel commented 3 years ago

The overriding functions assumed stricter qualifiers (@safe / scope) for the callback that are not guaranteed by the definition in Throwable.

The bug was hidden by this issue, see dlang/dmd#13267.


Example:

struct Store
{
    const(char)[] buffer;

    void escape(in char[] test) @system
    {
        buffer = test;
    }
}

unittest
{
    IOException ioe = new IOException(String("Test"));
    Throwable t = ioe;

    Store s;
    // ioe.toString(&s.escape); // Rejected by the old signature...
    t.toString(&s.escape);      // .. but still callable via virtual function call
}
PetarKirov commented 3 years ago

@schveiguy I'm not sure if Martin is active these days. Do you happen to have merge rights?

schveiguy commented 3 years ago

I have merge rights, this is essentially my project now. I need to investigate the CI failure.

MoonlightSentinel commented 3 years ago

The CI failure is a chicken-egg problem because the current implementation fails without the bugfix. Will try to find a different solution that works for both

schveiguy commented 3 years ago

I think the issue is that Martin got a bit safe-happy, but I don't think it's ever needed. A @safe delegate casts to a @system delegate just fine.

schveiguy commented 3 years ago

Ugh, travis is no longer working, and I don't have admin rights to this. I may have to fork this to my user and move registration of the project in code.dlang.org. It's not good enough to have CI only running on Windows.

schveiguy commented 3 years ago

Perhaps the way to fix the underlying problem is to add a @safe override on Throwable. It can just call the regular one by default.

schveiguy commented 3 years ago

Ugh, no it can't. It needs to be marked @safe as well, to make any sense. So it would have to repeat the implementation. We really need that mechanism to say that a function calls its delegate and so should absorb the attributes of it.

MoonlightSentinel commented 3 years ago

Guess one could do this:

void toString(scope void delegate(scope const char[]) @safe) @safe
{
      // Actual implementation
}

void toString(scope void delegate(scope const char[]) @system dg) @system
{
    // Forward to the first one
   // @safe functions are only expected to be safe when called with safe values IIRC
   scope void delegate(scope const char[]) @safe wrapper = (line) @trusted => dg(line);
   this.toString(wrapper);
}
MoonlightSentinel commented 3 years ago

Could you create a new tag for BuildKite?

schveiguy commented 3 years ago
@safe functions are only expected to be safe when called with safe values

Yes, this is essentially a poor-man's implementation of said attribute-coloring feature. But in the case of Throwable, it doesn't know what a derivative is going to do.

It also removes from the narrative that D can provide memory safety if the standard practice is to remove safety for practicality.

Could you create a new tag for BuildKite?

Will do.

MoonlightSentinel commented 3 years ago

Yes, this is essentially a poor-man's implementation of said attribute-coloring feature.

True, a language solution would be nice.

Will do.

Thanks!