On Friday, 6 February 2015 at 23:02:54 UTC, Zach the Mystic wrote:
> This solution appeals to me greatly. It pinpoints precisely where unsafe code can generate; it catches unintended safety violations in all @trusted code outside @system blocks, as requested by many people so far; it makes systems programming highly visible, with redundancy at the function signature and at the unsafe code itself. I really think it's spot on!

Even a call to a @system function inside a @trusted function must occur inside a @system block. It's that strict.

On Friday, 6 February 2015 at 23:25:02 UTC, Walter Bright wrote:
> I suspect that such a feature would simply lull people into a false sense of security in that merely tagging an unsafe cast with @system and the compiler accepting it is good enough.
>
> My evidence for this is how @trusted was used in Phobos.

How is adding @system to some operations *in addition to* adding @trusted to the function declaration more likely to lull people into a false sense of security than just adding @trusted right now?

Let me also point out that the cases where the @system block equivalent is used right now (like in std.file, or the trustedXyz functions in std.array) are NOT the ones that actually have safety bugs in them (such as std.array.uninitializedArray or std.uuid.randomUUID). The two latter examples were actually written in your preferred style.

David

On Friday, 6 February 2015 at 23:25:02 UTC, Walter Bright wrote:
>> This solution appeals to me greatly. It pinpoints precisely where unsafe code
>> can generate; it catches unintended safety violations in all @trusted code
>> outside @system blocks, as requested by many people so far; it makes systems
>> programming highly visible, with redundancy at the function signature and at the
>> unsafe code itself. I really think it's spot on!
>
> I suspect that such a feature would simply lull people into a false sense of security in that merely tagging an unsafe cast with @system and the compiler accepting it is good enough.
>
> My evidence for this is how @trusted was used in Phobos.

You do realize that our proposal *tightens* security, with no loosening at all? No code which currently fails to compile will start compiling with this proposal. This is literally a breaking change which does nothing but cause errors in existing code - for the explicit purpose of making all code safer, which it will do, possibly dramatically.

On 2/6/2015 3:34 PM, David Nadlinger wrote:
> On Friday, 6 February 2015 at 23:25:02 UTC, Walter Bright wrote:
>> I suspect that such a feature would simply lull people into a false sense of
>> security in that merely tagging an unsafe cast with @system and the compiler
>> accepting it is good enough.
>>
>> My evidence for this is how @trusted was used in Phobos.
>
> How is adding @system to some operations *in addition to* adding @trusted to the
> function declaration more likely to lull people into a false sense of security
> than just adding @trusted right now?
>
> Let me also point out that the cases where the @system block equivalent is used
> right now (like in std.file, or the trustedXyz functions in std.array) are NOT
> the ones that actually have safety bugs in them (such as
> std.array.uninitializedArray or std.uuid.randomUUID). The two latter examples
> were actually written in your preferred style.

I've said that the usage of those functions was not actually buggy, what was wrong about them was that they required review of the surrounding supposedly safe context. I.e. they produced a false sense of safety. I fear the @system blocks, even if only allowed in @trusted functions, would produce a similar illusion of safety.

I agree with Andrei in that I do not believe that reviewing a @trusted function, line by line, for safety is necessarily some sort of maintenance nightmare. If it is, then a refactoring should be considered to encapsulate the unsafe code in a smaller, simpler manner.

I.e. let's make an effort to use @trusted correctly, and then see where we stand.

Scott Meyer's excellent article (a classic):

  http://www.drdobbs.com/cpp/how-non-member-functions-improve-encapsu/184401197

describes this most eloquently. (Just substitute "private members" with "trusted code".)

On Fri, Feb 06, 2015 at 04:04:48PM -0800, Walter Bright via Digitalmars-d wrote: [...]
> I agree with Andrei in that I do not believe that reviewing a @trusted function, line by line, for safety is necessarily some sort of maintenance nightmare. If it is, then a refactoring should be considered to encapsulate the unsafe code in a smaller, simpler manner.
[...]

This does not take into the account the fact that a @trusted function may call other, non-@trusted, functions. When one of those other functions changes, the @trusted function necessarily needs to be reviewed again.

However, under the current implementation, this is not done because when the other, non-@trusted, function is modified, nobody thinks to re-review the @trusted function. They may not even be in the same module. There is no mechanism in place to raise a warning flag when a @trusted function's dependencies are modified. Thus, the proof of safety of the @trusted function has been invalidated, but trust continues to be conferred upon it.


T

-- 
Let's call it an accidental feature. -- Larry Wall

On 2/6/2015 4:29 PM, H. S. Teoh via Digitalmars-d wrote:
> This does not take into the account the fact that a @trusted function
> may call other, non-@trusted, functions. When one of those other
> functions changes, the @trusted function necessarily needs to be
> reviewed again.

That's correct.

> However, under the current implementation, this is not done because when
> the other, non-@trusted, function is modified, nobody thinks to
> re-review the @trusted function. They may not even be in the same
> module. There is no mechanism in place to raise a warning flag when a
> @trusted function's dependencies are modified. Thus, the proof of safety
> of the @trusted function has been invalidated, but trust continues to be
> conferred upon it.

When the interface to an @system function is changed, all uses of that function have to be reviewed, whether one thinks of it or not. This is part of the review process. Having @system blocks does not alter that.

On 2/6/15 3:21 PM, weaselcat wrote:
> On Friday, 6 February 2015 at 23:02:54 UTC, Zach the Mystic wrote:
>
>> No, at least three of us, Steven, H.S. Teoh and myself have confirmed
>> that we've moved beyond requesting @trusted blocks. We are no longer
>> requesting them. We are requesting *@system* blocks, which can only
>> appear in @trusted and @system functions. Any unsafe code appearing in
>> a @trusted function which is not inside a @system block is an error.
>> We've changed the name, but I think it will make a world of difference
>> regarding how you will look at it. Marking '@system' code inside a
>> @trusted function is exactly what is requested. Your message about
>> '@trusted' being only acceptable as an interface has been heard. There
>> will be no @trusted blocks, only @system blocks, which are the exact
>> same thing, except they can only appear in @trusted and @system
>> functions.
>>
>> This solution appeals to me greatly. It pinpoints precisely where
>> unsafe code can generate; it catches unintended safety violations in
>> all @trusted code outside @system blocks, as requested by many people
>> so far; it makes systems programming highly visible, with redundancy
>> at the function signature and at the unsafe code itself. I really
>> think it's spot on!
>
> this sounds interesting, is anyone going to make a DIP for it?

Consider the previous code:

https://github.com/D-Programming-Language/phobos/blob/accb351b96bb04a6890bb7df018749337e55eccc/std/file.d#L194

that got replaced with:

https://github.com/D-Programming-Language/phobos/blob/master/std/file.d#L194

With the system proposal we're looking at something like:

version (Posix) void[] read(in char[] name, size_t upTo = size_t.max) @trusted
{
    import core.memory;
    // A few internal configuration parameters {
    enum size_t
        minInitialAlloc = 1024 * 4,
        maxInitialAlloc = size_t.max / 2,
        sizeIncrement = 1024 * 16,
        maxSlackMemoryAllowed = 1024;
    // }

    @system
    {
        immutable fd = core.sys.posix.fcntl.open(name.tempCString(),
            core.sys.posix.fcntl.O_RDONLY);
    }
    cenforce(fd != -1, name);
    scope(exit) core.sys.posix.unistd.close(fd);

    stat_t statbuf = void;
    @system
    {
        cenforce(trustedFstat(fd, trustedRef(statbuf)) == 0, name);
    }

    immutable initialAlloc = to!size_t(statbuf.st_size
        ? min(statbuf.st_size + 1, maxInitialAlloc)
        : minInitialAlloc);
    void[] result = uninitializedArray!(ubyte[])(initialAlloc);
    scope(failure) delete result;
    size_t size = 0;

    for (;;)
    {
        @system
        {
            immutable actual = core.sys.posix.unistd.read(fd, result.ptr + size),
                min(result.length, upTo) - size);
        }
        cenforce(actual != -1, name);
        if (actual == 0) break;
        size += actual;
        if (size < result.length) continue;
        immutable newAlloc = size + sizeIncrement;
        @system
        {
            result = GC.realloc(result.ptr, newAlloc, GC.BlkAttr.NO_SCAN)[0 .. newAlloc];
        }

    @system
    {
        return result.length - size >= maxSlackMemoryAllowed
            ? GC.realloc(result.ptr, size, GC.BlkAttr.NO_SCAN)[0 .. size]
            : result[0 .. size];
    }
}

We want to move D forward, folks. This is not it.


Andrei

On Saturday, 7 February 2015 at 01:41:19 UTC, Andrei Alexandrescu wrote:
> Consider the previous code:
> [...]
>     static trustedRead(int fildes, void* buf, size_t nbyte) @trusted
>     {
>         return core.sys.posix.unistd.read(fildes, buf, nbyte);
>     }
>     static trustedRealloc(void* p, size_t sz, uint ba = 0, const TypeInfo ti = null) @trusted
>     {
>         return GC.realloc(p, sz, ba, ti);
>     }
>     static trustedPtrAdd(void[] buf, size_t s) @trusted
>     {
>         return buf.ptr+s;
>     }
>     static trustedPtrSlicing(void* ptr, size_t lb, size_t ub) @trusted
>     {
>         return ptr[lb..ub];
>     }

First of all, these little @trusted functions are made obsolete by the new system. They should certainly be omitted.

>     @system
>     {
>         immutable fd = core.sys.posix.fcntl.open(name.tempCString(),
>             core.sys.posix.fcntl.O_RDONLY);
>     }

Next, you have to realize that @system blocks are like 'try' blocks: You don't need brackets if there's only one statement in them.

Here's how I would rewrite what you have written using the new method:

version (Posix) void[] read(in char[] name, size_t upTo = size_t.max)
@trusted
{
    import core.memory;
    // A few internal configuration parameters {
    enum size_t
        minInitialAlloc = 1024 * 4,
        maxInitialAlloc = size_t.max / 2,
        sizeIncrement = 1024 * 16,
        maxSlackMemoryAllowed = 1024;
    // }

    @system immutable fd = core.sys.posix.fcntl.open(name.tempCString(),
            core.sys.posix.fcntl.O_RDONLY);
    cenforce(fd != -1, name);
    scope(exit) core.sys.posix.unistd.close(fd);

    stat_t statbuf = void;
    @system cenforce(trustedFstat(fd, trustedRef(statbuf)) == 0, name);

    immutable initialAlloc = to!size_t(statbuf.st_size
        ? min(statbuf.st_size + 1, maxInitialAlloc)
        : minInitialAlloc);
    void[] result = uninitializedArray!(ubyte[])(initialAlloc);
    scope(failure) delete result;
    size_t size = 0;

    for (;;)
    {
        @system immutable actual = core.sys.posix.unistd.read(fd,
                result.ptr + size, min(result.length, upTo) - size);
        cenforce(actual != -1, name);
        if (actual == 0) break;
        size += actual;
        if (size < result.length) continue;
        immutable newAlloc = size + sizeIncrement;
        @system result = GC.realloc(
                result.ptr, newAlloc, GC.BlkAttr.NO_SCAN)[0 .. newAlloc];
    }

    @system return result.length - size >= maxSlackMemoryAllowed
        ? GC.realloc(result.ptr, size, GC.BlkAttr.NO_SCAN)[0..size]
            : result[0 .. size];
}

Note that I just mechanically your @system blocks with the better form. I didn't arrange for them to be elegant. There's nothing wrong with encapsulating a @trusted sequence in a @system block with brackets, to aid future reviewers in identifying subsequent code thought to be affected by the @system statements. Also, few functions will have their @system blocks more or less evenly distributed throughout like the above function does.

The new proposal will never let you add an unsafe operation without your knowing it. It's definitely the way forward.

On Saturday, 7 February 2015 at 05:35:51 UTC, Zach the Mystic wrote:
> Note that I just mechanically your @system blocks with the better form.

...mechanically replaced, I mean, of course.

On Saturday, 7 February 2015 at 05:35:51 UTC, Zach the Mystic wrote:
> Here's how I would rewrite what you have written using the new method:
> ...
>     stat_t statbuf = void;
>     @system cenforce(trustedFstat(fd, trustedRef(statbuf)) == 0, name);

I didn't rewrite this because I didn't see the trustedXXX functions they referred to, but the basic rewrite would be:

@system cenforce(fstat(fd, reff(statbuf) == 0), name);

In other words, just copy the @system code directly without going through @trusted.