https://issues.dlang.org/show_bug.cgi?id=14125

--- Comment #68 from hsteoh@quickfur.ath.cx ---
(In reply to Dicebot from comment #66)
[...]
> Wrapper is marked @trusted incorrectly but that is intentional. @trusted here is not used to tell function can actually be trusted but to keep everything else plain @safe.

I think this is precisely what Walter & Andrei are objecting against.


> Alternative solution is to invert it:
> 
> S readText(S = string)(in char[] name) @trusted if (isSomeString!S)
> {
>     () @safe {
>         import std.utf : validate;
>         auto s = read(name);
>     } ();
> 
>     auto result = cast(S) s;
> 
>     () @safe {
>         validate(result);
>     } ();
> 
>     return result;
> }
> 
> Which also solves same problem but it result in creation of much more wrappers and makes reading the source even harder (even despite the fact I cheated here and used lambdas instead of nested functions).

This would be the pedantically-correct version of the code. However, it is even worse in readability than the current code.

Then there's Andrei's PR that contains a 50+-line @trusted function. I had a headache trying to figure out whether it was truly @trusted -- it's wayyyy too big to be adequately reviewed in a satisfactory way. And this review effort has to be incurred every single time somebody so much as touches a single line in that function, because @trusted disables all compiler safety checks and therefore we can never be too sure whether the change has introduced subtle breakage in safety.

Not to mention, if any line in the 50+-line function is a call to another function, then every time any one of *those* functions change (e.g., they were @safe before but now an unrelated PR has made them @system), you have to re-review the whole 50 lines again. Plus ALL other @trusted functions that may have called those functions. This is clearly impractical beyond trivial textbook examples.

I would truly love to know how Walter/Andrei propose to solve this maintenance nightmare, since I'm clearly missing something that's blatantly obvious to them.

--

https://issues.dlang.org/show_bug.cgi?id=14125

--- Comment #69 from Dicebot <public@dicebot.lv> ---
Sure, I don't hold any bad feeling about it - won't be the first time something very uncomfortable for me happens in language upstream :) Only thing I wanted to clarify is that existing code is not written that way out of ignorance or reviewer sloppinness - it was an attempt of active Phobos reviewers to address a specific problem we encountered.

Having a different vision of how this needs to be designed is OK, but original NG thread implied we did our "job" badly and that wasn't really true.

--

https://issues.dlang.org/show_bug.cgi?id=14125

--- Comment #70 from Andrei Alexandrescu <andrei@erdani.com> ---
Loosely related: https://github.com/D-Programming-Language/druntime/pull/1157. That and more like it is definitely good to do.

--

https://issues.dlang.org/show_bug.cgi?id=14125

--- Comment #71 from David Nadlinger <code@klickverbot.at> ---
(In reply to Andrei Alexandrescu from comment #61)
> Well I just did - std.file does count for real-world code. It's just not right.

Hold on a second. I'm suggesting that trying to clean up the std.array functions might lead to an insight or two (interplay between the need for @system code and template attribute inference), and you reply by claiming that std.file is bad code?

I never tried to convince you that all real-world code would unambiguously benefit from finer-grained @trusted-ness. But there is a good amount of code where, as I wrote earlier, "[its] conceptual equivalent is useful right now". Generic range algorithms that do some @system optimizations internally, such as std.array, are an example.

This discussion would be much easier if you didn't assume that I don't know what I'm talking about. In fact, I'm rather confident that I have a better understanding of @safe-ty than most other people. :) You are just not seeing a part of the problem that is there, right now, in real world generic code.

Whatever the solution to this issue will be—nested functions as currently used in parts of Phobos, @trusted lambdas, @trusted blocks or something else entirely—, discussing design decisions doesn't make a lot of sense without a good grasp of the problem domain.

I'm rather certain that you calling std.array a disaster area is a consequence of that. So, please consider how the need for attribute inference might influence the picture before we discuss this any further. Once we agree on a non-disastrous solution for the cases where the safety of a function depends on a template parameter we can discuss how we might or might not want to apply it as a general coding guideline in situations like std.file.

--

https://issues.dlang.org/show_bug.cgi?id=14125

--- Comment #72 from Andrei Alexandrescu <andrei@erdani.com> ---
(In reply to hsteoh from comment #68)
> Then there's Andrei's PR that contains a 50+-line @trusted function. I had a headache trying to figure out whether it was truly @trusted -- it's wayyyy too big to be adequately reviewed in a satisfactory way.

50 lines is very little code, and code that interfaces with low-level libraries and optimizes the bejesus out of system calls and memory allocations - obviously there must be some manual verification going on, and I don't see it as onerous.

It also replaces code that is in no way more provably safe because it uses escape hatches literally almost at every line! How is that drivel better when it has functions that allow escaping addresses and adding pointers and all that stuff?

I am sorry I am not buying this line of argumentation for one second. https://github.com/D-Programming-Language/phobos/pull/2963 replaces utter drivel with sensible code.

--

https://issues.dlang.org/show_bug.cgi?id=14125

--- Comment #73 from Andrei Alexandrescu <andrei@erdani.com> ---
(In reply to David Nadlinger from comment #71)
> (In reply to Andrei Alexandrescu from comment #61)
> > Well I just did - std.file does count for real-world code. It's just not right.
> 
> Hold on a second. I'm suggesting that trying to clean up the std.array functions might lead to an insight or two (interplay between the need for @system code and template attribute inference), and you reply by claiming that std.file is bad code?

Prolly some misunderstanding in the going back and forth.

> I never tried to convince you that all real-world code would unambiguously benefit from finer-grained @trusted-ness. But there is a good amount of code where, as I wrote earlier, "[its] conceptual equivalent is useful right now". Generic range algorithms that do some @system optimizations internally, such as std.array, are an example.
> 
> This discussion would be much easier if you didn't assume that I don't know what I'm talking about.

How in the world did you acquire that idea?

> In fact, I'm rather confident that I have a better understanding of @safe-ty than most other people. :)

This is fantastic. I think the right way to go here is to leverage your good understanding in finding better solutions. It takes me all of three seconds to look at std.file.read and figure out that's definitely a bad way to go about things.

> You are just not seeing
> a part of the problem that is there, right now, in real world generic code.

Well here I'm again counting on your excellent understanding of the topic for good explanations and proposed solutions.

I'm not being sarcastic. If you have a good grasp over the matter at hand, there must be a way you can prove that by concrete proposals and solutions, as in DIPs and code and great pull requests (as opposed to relativelt unproductive general conversation). Even though I am not as creative as you, I'm pretty sure I can identify a brilliant solution when I see it. I think you'd agree the code that https://github.com/D-Programming-Language/phobos/pull/2963 fixes is quite the opposite of it. Please do come with something better.

> Whatever the solution to this issue will be—nested functions as currently used in parts of Phobos, @trusted lambdas, @trusted blocks or something else entirely—, discussing design decisions doesn't make a lot of sense without a good grasp of the problem domain.
> 
> I'm rather certain that you calling std.array a disaster area is a consequence of that. So, please consider how the need for attribute inference might influence the picture before we discuss this any further. Once we agree on a non-disastrous solution for the cases where the safety of a function depends on a template parameter we can discuss how we might or might not want to apply it as a general coding guideline in situations like std.file.

For generic functions safety would be deduced (which is indeed an area that needs improvement - a very high-impact potential). There is of course the possibility of forking the implementations by using template constraints and then using different attributes for the forks.

--

https://issues.dlang.org/show_bug.cgi?id=14125

--- Comment #74 from Walter Bright <bugzilla@digitalmars.com> ---
(In reply to hsteoh from comment #68)
> Then there's Andrei's PR that contains a 50+-line @trusted function. I had a headache trying to figure out whether it was truly @trusted -- it's wayyyy too big to be adequately reviewed in a satisfactory way. And this review effort has to be incurred every single time somebody so much as touches a single line in that function, because @trusted disables all compiler safety checks and therefore we can never be too sure whether the change has introduced subtle breakage in safety.

See my review of std.array.join() upthread. The idea that one could mark a function with an unsafe interface as @trusted and thereby avoid having to check the rest of the code for safety is destroyed by that example. The rest was correct, but could not be checked for safety by the compiler. It had to be reviewed. The @trusted workaround only provided an illusion of safety. No work was avoided - extra work is caused because I no longer trust the code in std.array and it all needs reviewing again.

I propose a review rule - functions of the following patterns will be automatically rejected by reviewers:

1. any function that trivially wraps a call to an @system function so it can be marked as @trusted. For example:

    @trusted void* trustedMalloc(size_t n) { return malloc(n); }

2. any function that is a trivial wrapper around an operation that would otherwise be flagged as unsafe, for example:

    @trusted void* incPtr(void* p) { return p + 1; }

--

https://issues.dlang.org/show_bug.cgi?id=14125

John Colvin <john.loughran.colvin@gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |john.loughran.colvin@gmail.
                   |                            |com

--- Comment #75 from John Colvin <john.loughran.colvin@gmail.com> ---
Walter and Andrei are completely right here.

If you can't factor out the @system code to a function providing a truly safe interface (marked with @trusted), then the code clearly depends on its surrounding context to make it safe. So *all* of that code needs to be manually verified with the same scrutiny, together with the core bit that actually appeared to be @system.

It becoming a maintenance nightmare is just unveiling the true difficulty of safely using @system code, as opposed to papering over it.

It might become good practice in robust @trusted code to add static asserts to ensure that changes to @system (explicit or inferred) further down the call tree aren't accidentally missed. static assert(isTrusted!func) or similar.

--

https://issues.dlang.org/show_bug.cgi?id=14125

--- Comment #76 from Steven Schveighoffer <schveiguy@yahoo.com> ---
After reading this quite long thread (and I'm not going to CC myself because of this), I want to summarize the viewpoints here. I think I see that everyone is in slight agreement, but does not realize the other's *complete* viewpoint.

1. We all agree that a public function such as:

@trusted auto tmalloc(size_t x) {return malloc(x);}

Is no good, should never pass review.

2. There are occasions where inside a very lengthy function one wishes to be called from @safe code, that a small portion of code which uses unsafe functions, but in a safe manner, is needed. One would wish that in such a function all the actual @safe calls are still checked by the compiler, and any additions to the @safe portion should be checked for future maintainers.

3. A static nested function inside such a function is a license to commit sin at will. In other words, such a function allows abuse by current and future maintainers of that function. Therefore, even nested functions like the one in point 1 above should probably be rejected. However, @trusted static nested functions that do NOT leak memory details can be allowed.

4. A possible restriction (but not bulletproof) is to use an immediately-called lambda to execute a trusted call. The issue here is that it won't be inlined. Hence the desire to use static nested functions.

5. There is a general issue that adding valid @trusted calls inside a @safe function is simply allowed in review without too much consideration for what it means for future maintenance.

-------------

Now, point 4 is not bullet proof. Walter brought up a case where it is not, and it's not hard to see it can be abused. When a @trusted call affects variables inside a @safe function, it can mean that depending on the calls, any code that then *uses* those variables, even in a safe way, is suspect. This means that @trusted must extend to all those uses. Such a position makes for an incorrect assumption about the entire function.

However, I think even worse is the idea that we add a static callable @trusted function nested inside a @safe function, that can be called in an unsafe way. Such a function cannot guarantee calls to it are safe. A lambda is much better because review of its uses involves one line. But only if its details do not leak!

So I would say, as a review requirement, we should limit internal @trusted calls ONLY to lambdas, unless the static nested function does not leak memory details. And inside those lambdas, the code should not affect any variables in @safe code in a way that has the potential for abuse. I think this is doable, because you can ask that question objectively by reading the one call. It doesn't matter if code *doesn't* use that variable in an un@safe way, just the *potential* of using it is enough to cause the review to fail.

There is the issue with not inlining lambdas. Tough. Get that fixed, it's not worth corrupting memory to save some performance.

That's my $.02. I'll see about reviewing std.file.read and try and apply this methodology to fix the issues.

--

https://issues.dlang.org/show_bug.cgi?id=14125

--- Comment #77 from Andrei Alexandrescu <andrei@erdani.com> ---
Thanks Steve for the nice summary!

--