Bug 14125 – @trusted nested helper functions in std.file

std.file.read defines a few @trusted functions e.g. at https://github.com/D-Programming-Language/phobos/blob/master/std/file.d#L259, for each of the smallest unsafe operation performed in the actual function itself. Far as I can tell those are for the purpose of making std.file.read itself @safe. The goal (making std.file.read @safe) is worthy and the approach, while fine in letter, is awfully bad in spirit. There is nothing to be gained by making each operation @trusted without due verification; and when it comes to it it's easier to actually verify the body of read() than building all that useless scaffolding. To my horror there's a lot of similar misguided work in std.file. The right solution here is to make those functions @trusted. The fact that such code got in - sometimes pulled by myself! - reveals a problem with our review process. We need much better scrutiny and quality control. And we need to sit down and undo the damage already done ASAP.

I didn't look closely at std.file, but I think the consensus over the past several months is that @trusted should not be used if avoidable, and if it cannot be avoided, it should be limited to small chunks of code, usually factored out as small lambdas in the main function body which is marked @safe. Example: ----- auto mySafeFunc(T...)(T args) @safe { int trustedHelper(int x) @trusted { // Here, it should be explained exactly why the particular value(s) of // x here will never trigger un-@safe behaviour in the following call: return potentiallyDangerousFunc(x); } doStuff(...); auto x = trustedHelper(...); doMoreStuff(); return result; } ----- The idea is that doStuff() and doMoreStuff() can be non-trivial, convoluted code, that we don't want to be reviewing every single time we review usage of @trusted; the actually-trusted bit of code should be confined to trustedHelper(), which is small enough that should the need arise, we can review it within a reasonable amount of time. I don't know if this is the best way to do it, but this particular construct is quickly becoming an idiom in Phobos.

Andrei, you may have been too quick to give a judgement without having a close look what actually happens there. All those @trusted functions exist there exact for the purpose of NOT marking everything as @trusted but keeping actual function @safe. Those are used in contexts where operation in general can't be verified to be @safe (and thus C binding can't be marked @trusted itself) but it is verified in specific context where it is called. Wrapping such call in trivial @trusted function allows to keep everything else @safe and enjoy compiler verification. I do immediately see that some of those posix functions could have been marked as @trusted in extern(C) binding itself - and should probably be removed to reduce visual noise and simplify maintenance. However, there is nothing fundamentally broken with that code in context of type system and language itself.

I would actually argue that the way it's implemented currently is better than what you're suggesting. If I'm understanding correctly, you want std.file.read to be @trusted rather than @safe. The way it's implemented currently limits the amount of code in the @trusted section significantly, and the rest is in the @safe version. This is done so that the only parts of the method that need to be @trusted are in @trusted blocks.

I would actually argue that the way it's implemented currently is better than what you're suggesting. If I'm understanding correctly, you want std.file.read to be @trusted rather than @safe. The way it's implemented currently limits the amount of code in the @trusted section significantly, and the rest is in the @safe version. This is done so that the only parts of the method that need to be @trusted are in @trusted blocks.

>I didn't look closely at std.file, but I think the consensus over the past several months is that @trusted should not be used if avoidable, and if it cannot be avoided, it should be limited to small chunks of code, usually factored out as small lambdas in the main function body which is marked @safe [...] >I don't know if this is the best way to do it, but this particular construct is quickly becoming an idiom in Phobos. As I said, there's something to be said about the letter of the "law" and its respective spirit. Anything good can be done in poor taste, and sadly std.file is chock full of things done in perfectly poor taste. We must fix it asap, it's a disaster area. If that is how D is meant to be used I don't want to use it. Look a this pile of dung: S readText(S = string)(in char[] name) @safe if (isSomeString!S) { import std.utf : validate; static auto trustedCast(void[] buf) @trusted { return cast(S)buf; } auto result = trustedCast(read(name)); validate(result); return result; } How exactly does that scaffolding help a four liner? It takes longer to read all that crap than to actually figure the function is correct! Or look at this one: ulong getSize(in char[] name) @safe { version(Windows) { with (getFileAttributesWin(name)) return makeUlong(nFileSizeLow, nFileSizeHigh); } else version(Posix) { static auto trustedStat(in char[] path, stat_t* buf) @trusted { return stat(path.tempCString(), buf); } static stat_t* ptrOfLocalVariable(return ref stat_t buf) @trusted { return &buf; } stat_t statbuf = void; cenforce(trustedStat(name, ptrOfLocalVariable(statbuf)) == 0, name); return statbuf.st_size; } } How in the world is all that scaffolding help anyone with anything? There is a place for the spirit. Clearly large unchecked swaths of @trusted code work against quality, but there's a limit to it that std.file went way beyond.

>Andrei, you may have been too quick to give a judgement without having a close look what actually happens there. I think I have a good understanding of what happened: a good intent has been applied in a way that got it thoroughly corrupted.

(In reply to Orvid King from comment #4) > I would actually argue that the way it's implemented currently is better > than what you're suggesting. > If I'm understanding correctly, you want std.file.read to be @trusted rather > than @safe. Correct. > The way it's implemented currently limits the amount of code in the @trusted > section significantly, and the rest is in the @safe version. This is done so > that the only parts of the method that need to be @trusted are in @trusted > blocks. I understand the intent. The moment I need to define dung like: static trustedRef(T)(ref T buf) @trusted { return &buf; } I know there's a problem with the application of the idiom.

"There is nothing to be gained by making each operation @trusted without due verification." Trust but verify? :) @trusted is one thing, but it's sad how little code uses @nogc in both std.file and std.stdio. Have a look at the latest discussion in http://forum.dlang.org/thread/[email protected]

(In reply to FG from comment #8) > "There is nothing to be gained by making each operation @trusted without due > verification." Trust but verify? :) > > @trusted is one thing, but it's sad how little code uses @nogc in both > std.file and std.stdio. Have a look at the latest discussion in > http://forum.dlang.org/thread/[email protected] Yah, good point but let's not derail this issue though. Thanks.

Consider the following code: @trusted void* trustedMalloc(size_t n) { return malloc(n); } @trusted void trustedFree(void* p) { free(p); } @safe void foo() { auto p = trustedMalloc(5); trustedFree(p); trustedFree(p); } foo() passes @safe checks, yet is able to corrupt memory. The fault is that the @trusted functions failed to encapsulate what they're doing and present a safe interface. @trusted functions must be reviewed to determine if they present a safe interface or not. Merely wrapping an unsafe operation is not good enough and must not pass review.

(In reply to Andrei Alexandrescu from comment #5) > S readText(S = string)(in char[] name) @safe if (isSomeString!S) > { > import std.utf : validate; > static auto trustedCast(void[] buf) @trusted { return cast(S)buf; } > auto result = trustedCast(read(name)); > validate(result); > return result; > } > > How exactly does that scaffolding help a four liner? It takes longer to read > all that crap than to actually figure the function is correct! I don't care how long it takes to read that crap. What is important is that if anyone later will add some @system line to that function by an accident, compiler will immediately complain about it. Correct comes first and pretty comes later. If you want that code to be more pretty, please forward this issue to DMD developers and show them how hardly usable @safe is sometimes in practice because compiler is simply not clever enough to reason that something is @safe (for example, declaring _all_ casts @system is clearly an overkill) We may move some of @trusted to actual core.stdc signatures, that is true. But once you start marking whole functions (even 4 lines long) as @trusted, you immediately destroy all trust in @trusted (pun intended) and becomes a non-feature. I completely refuse to accept your suggested coding style on this matter.

(In reply to Walter Bright from comment #10) > Consider the following code: > > @trusted void* trustedMalloc(size_t n) { return malloc(n); } > @trusted void trustedFree(void* p) { free(p); } > > @safe void foo() { > auto p = trustedMalloc(5); > trustedFree(p); > trustedFree(p); > } > > foo() passes @safe checks, yet is able to corrupt memory. The fault is that > the @trusted functions failed to encapsulate what they're doing and present > a safe interface. > > @trusted functions must be reviewed to determine if they present a safe > interface or not. Merely wrapping an unsafe operation is not good enough and > must not pass review. This is why such wrapper functions are wlays kept private and as long as possible - local to functions those are used in. It would help a lot if `() @trusted { foo(); }` lambdas could be 100% inlined - then those could be used instead to prevent accidentla reusage of wrapper in wrong context.

(In reply to Dicebot from comment #11) > I completely refuse to accept your suggested coding style on this matter. I'm sorry we've reached irreducible positions on this. I'm not suggesting - we need to fix this.

Do as you wish. I have removed myself from Phobos developer list due to inability to comply to demanded policies.

(In reply to Dicebot from comment #14) > Do as you wish. I have removed myself from Phobos developer list due to > inability to comply to demanded policies. If this is that important to you, understood. I trust you do what you think is best, as I do.

(In reply to Dicebot from comment #12) > It would help a lot if `() @trusted { foo(); }` lambdas could be 100% > inlined - then those could be used instead to prevent accidentla reusage of > wrapper in wrong context. This. Solves the visual noise problem while keeping the good parts (only mark trusted what really needs to be). In fact this has already been brought up in the "@trusted considered harmful" thread I started a couple of years (?) ago.

(In reply to Walter Bright from comment #10) > @trusted functions must be reviewed to determine if they present a safe > interface or not. Merely wrapping an unsafe operation is not good enough and > must not pass review. This isn't relevant to the issue discussed here. The helper functions in std.file are local to their callers. While I agree that the code looks odd and a visually less noisy alternative is to be preferred (e.g. lambdas), no @safe-ty issues are exposed to client code.

(In reply to David Nadlinger from comment #16) > (In reply to Dicebot from comment #12) > > It would help a lot if `() @trusted { foo(); }` lambdas could be 100% > > inlined - then those could be used instead to prevent accidentla reusage of > > wrapper in wrong context. > > This. Solves the visual noise problem while keeping the good parts (only > mark trusted what really needs to be). > > In fact this has already been brought up in the "@trusted considered > harmful" thread I started a couple of years (?) ago. Clearly there has got to be reason and good judgment to this all. We can't possibly decorate every other line of code with `() @trusted { ... }` and feel good about it. `@trusted` means "verified by hand to be safe", and planting it for arbitrary and arbitrarily small unsafe operations (such as pointer additions) hinders proving, doesn't help it. Any nice paint can be applied too thickly, and this is no exception.

(In reply to David Nadlinger from comment #17) > (In reply to Walter Bright from comment #10) > > @trusted functions must be reviewed to determine if they present a safe > > interface or not. Merely wrapping an unsafe operation is not good enough and > > must not pass review. > > This isn't relevant to the issue discussed here. The helper functions in > std.file are local to their callers. I find it relevant - abundant small @trusted functions that are obviously not safe unless the larger context is known is undesirable. > While I agree that the code looks odd and a visually less noisy alternative > is to be preferred (e.g. lambdas), no @safe-ty issues are exposed to client > code. Again: my point here is putting our heads together and doing some good judgment. It is strikingly obvious that the application of @trusted to std.file has transgressed into a monster of complexity.

It seems that there's a disconnect here between @trusted as marking a trusted *interface* (i.e., behind the interface is code that's potentially dangerous, but you cannot trigger the dangerous behaviour using that interface), and @trusted as marking a potentially dangerous *operation* that is actually safe because the surrounding context ensures that it is never used in an unsafe way. The former is for presenting an interface to the user that we can verify won't trigger unsafe behaviour; the latter is for preventing accidental breakage of the trusted code by careless change. What Walter said pertains to the former; what Dicebot said pertains to the latter. Neither are directly related to syntax, though, which is apparently what provoked this backlash. If it's the syntax that's the problem, then we should be looking at dedicated syntax for @trusted blocks inside functions, or always inlining lambdas that are immediately invoked. Putting std.file on the pedestal as the whipping boy isn't actually solving the core issues IMO.

(In reply to hsteoh from comment #20) > It seems that there's a disconnect here between @trusted as marking a > trusted *interface* (i.e., behind the interface is code that's potentially > dangerous, but you cannot trigger the dangerous behaviour using that > interface), and @trusted as marking a potentially dangerous *operation* that > is actually safe because the surrounding context ensures that it is never > used in an unsafe way. Well put. If I have a free-for-all trusted address taking at the top of a function: https://github.com/D-Programming-Language/phobos/blob/master/std/file.d#L198 then what good does that do to anybody? > The former is for presenting an interface to the user that we can verify > won't trigger unsafe behaviour; the latter is for preventing accidental > breakage of the trusted code by careless change. What Walter said pertains > to the former; what Dicebot said pertains to the latter. > > Neither are directly related to syntax, though, which is apparently what > provoked this backlash. If it's the syntax that's the problem, then we > should be looking at dedicated syntax for @trusted blocks inside functions, > or always inlining lambdas that are immediately invoked. Syntax is the least of the problems here. > Putting std.file on > the pedestal as the whipping boy isn't actually solving the core issues IMO. I see fixing std.file as an important and urgent matter. Phobos code is how industrial-strength code should be written, and should stand as an example of idiomatic use of D. Right now std.file is anything but a good example to follow.

(In reply to Andrei Alexandrescu from comment #21) > I see fixing std.file as an important and urgent matter. Phobos code is how > industrial-strength code should be written, and should stand as an example > of idiomatic use of D. Right now std.file is anything but a good example to > follow. To clarify: the larger matter here is fixing the review process to make sure there's enough scrutiny before things happen, not after.

(In reply to hsteoh from comment #20) > It seems that there's a disconnect here between @trusted as marking a > trusted *interface* (i.e., behind the interface is code that's potentially > dangerous, but you cannot trigger the dangerous behaviour using that > interface), and @trusted as marking a potentially dangerous *operation* that > is actually safe because the surrounding context ensures that it is never > used in an unsafe way. If code marked as @safe is not mechanically checkable as being safe, it must not be marked as safe. It should be marked as trusted. Trusted code is, by definition, checked to be safe by the user. Safe code is, by definition, checked by the compiler. Hence "and @trusted as marking a potentially dangerous *operation* that is actually safe because the surrounding context ensures that it is never used in an unsafe way" is not correct. It would be correctly written as "and @system as marking a potentially dangerous *operation* that is actually safe because the surrounding @trusted context ensures that it is never used in an unsafe way".

If that's not how @trusted was intended to be used, then fine, fix it. However, that still does not address Dicebot's concern about protecting against future change, which I think is a far more important issue. Currently, other than what's currently in std.file which apparently is not "proper use" of @trusted, there is no way to indicate which part of a larger function body actually contains the trusted operation. For the most part, trusted functions tend to only have a small core that's actually trusted, and everything else really ought to be enforced to be @safe. When reviewing the function, you want to only look at the relevant part, instead of reading the whole thing, otherwise nobody would bother reviewing any @trusted code, since it's too much work. Which defeats the purpose of @trusted. Furthermore, if the entire function body is @trusted, then any arbitrary change can introduce more unsafe operations to the function without any warning, and subtle bugs can creep into the code this way. This is the original rationale for factoring these one-liners into @trusted local functions -- that forces the main function body to conform to @safe requirements, and only those small core bits that actually need to be @trusted are marked as @trusted. Or, put another way, the "dung" construct you're railing against is a way of pointing out to reviewers "this is the bit that requires @trusted, everything else is @safe and you don't have to worry about that". It's a way of making the part that needs @trusted stand out. If this isn't "proper use" of @trusted, then fine, but we *do* need *some* way of restricting the scope of what's allowed in a @trusted function, especially in a large collaborative project like Phobos. Right now, marking the entire function @trusted is basically a free-for-all license that whatever garbage code you wish to throw into this function, you can do it and @safe code will still continue to work (even though it shouldn't). I mean, come on, if there's a large PR that touches 50 files and makes 5 changes in several 500-line functions, are you seriously expecting reviewers to have to read the entire 500 lines *and* understand all of its subtleties, just because it's @trusted? Most likely what will happen is that unsafe code will slip in without anyone noticing, and before you know it, the function is completely unsafe yet it's still marked @trusted. For this reason, @trusted needs to be kept as restricted as possible.

@Walter: mechanical verification of @safe is precisely what we're trying to leverage here. Given a large function body with 1 or 2 lines that need manual review, would you rather mark the entire function as @trusted (thus requiring review of the whole function *and* disabling mechanical @safe checks for the other 498 lines of code that the compiler *could* have verified automatically), or would you rather that the function is marked as @safe -- which imposes mechanical verification of safety -- with the exception of a small @trusted part that needs to be verified by hand?

@trusted is intended to encapsulate unsafe code, not rely on the caller to call @trusted code in a safe manner. Simply using @trusted to bypass checks, and rely on supposedly @safe code to then use those @trusted sequences correctly, defeats the purpose of @safe code. The way @trusted is being used in std.file makes the supposedly @safe code calling it uncheckable by the compiler. @safe code must be 100% mechanically checkable, not correct by convention.

For example, Line 213: static trustedCloseHandle(HANDLE hObject) @trusted { return CloseHandle(hObject); } All this is doing is redefining an @system function as being @trusted, without changing the interface to it, or checking, or doing anything at all to justify it now being trusted. It may be true that CloseHandle() actually is trustworthy - if that is the case, the declaration of CloseHandle() needs to change from @system to @trusted. On the other hand, if it is not trustworthy, papering it over with such a wrapper and relying on the caller to use it correctly by convention has the effect of totally defeating the compiler's ability to check code.

I agree that what's happening with std.file needs to be fixed. But still, the nagging issue is, how do we restrict the scope of @trusted inside a function body? This is pertinent because your mechanical verification of @safe hinges upon @trusted functions being actually trustworthy -- since @safe code can call @trusted functions no matter how unsafe the latter are, otherwise the mechanical verification proves nothing and is useless. If I have a @trusted function that only performs 1 or 2 potentially unsafe operations, I'd like to mark those specific operations as needing manual verification, while the rest of the function body ought to be under the scrutiny of the compiler under @safe requirements. The idea is, if somebody else comes along later and changes the code, I don't want him to introduce new unsafe operations to the function body -- at least, not without requiring the use of the same markup that designates said operations as unsafe. In other words, besides those 1 or 2 potentially unsafe operations, I want everything else in the function to be mechanically enforced to conform to @safe requirements. If any new unsafe operations are to be introduced to the function, I want them clearly marked as such so that they can be thoroughly scrutinized before the code change is accepted. Any new code that isn't thus marked ought to be rejected by the compiler outright as a potential breach of trust. Otherwise, once a function is marked @trusted, a careless code change can easily introduce safety violations without any warning, and then percolate throughout the entire @safe system.

One idea that occurred to me (though it may be a bit too late to implement) is that @trusted functions remain under @safe requirements except for @system blocks within the function body, e.g.: ----- // This is hypothetical syntax, the exact syntax is not important, // it's the idea behind it. int myTrustedFunc(int x) @trusted { int x = *cast(int*)null; // Compile error: unmarked unsafe operation in @trusted function @system { enum magicAddress = 0x900D1DEA; int y = *cast(int*)magicAddress; // OK, unsafe operation allowed in @system block } free(null); // Compile error: cannot call @system function outside @system block return ...; } ----- This way, reviewers know to scrutinize everything inside the @system block, while the code outside is mechanically verified not to introduce more @system operations to the function.

(In reply to hsteoh from comment #24) > If that's not how @trusted was intended to be used, then fine, fix it. > > However, that still does not address Dicebot's concern about protecting > against future change, which I think is a far more important issue. What is there right now is a false sense of security. There's no way I can convince myself that static trustedRealloc(void* p, size_t sz, uint ba = 0, const TypeInfo ti = null) @trusted { return GC.realloc(p, sz, ba, ti); } can actually be trusted by inspecting it. It's essentially a license for the code using it to call realloc and pretend it's safe. So the whole notion that "not one @system line can be added without the compiler protesting" is wrong. There's plenty of stuff that can be added that will pass compilation flag free and is completely unsafe - all in a @safe function because we gave ourself the illusion of safety at the wrong level of granularity. A trusted function is what it sounds: I look at it and I can vouch for its safety, even though the compiler cannot prove it. By availing myself of a trusted one-liner that is in fact completely unsafe, I can corrupt any amounts of @safe code with it. > Currently, other than what's currently in std.file which apparently is not > "proper use" of @trusted, there is no way to indicate which part of a larger > function body actually contains the trusted operation. I must note I disagree with the characterization here - quoted "proper use" and all that, as if it's some arbitrary party sanction. It's as if what's now in std.file is okay but not by the party line. No, it's not okay at all! It takes all of a few seconds to see it for what it is - an emperor's clothes issue. > For the most part, > trusted functions tend to only have a small core that's actually trusted, > and everything else really ought to be enforced to be @safe. When reviewing > the function, you want to only look at the relevant part, instead of reading > the whole thing, otherwise nobody would bother reviewing any @trusted code, > since it's too much work. Which defeats the purpose of @trusted. Trusted functions must be themselves small and cohesive enough to be verifiable manually. > Furthermore, if the entire function body is @trusted, then any arbitrary > change can introduce more unsafe operations to the function without any > warning, and subtle bugs can creep into the code this way. As said above that can easily happen today, and it's a lot worse because it's all under the pretense some safety is being enforced. > This is the > original rationale for factoring these one-liners into @trusted local > functions -- that forces the main function body to conform to @safe > requirements, and only those small core bits that actually need to be > @trusted are marked as @trusted. Or, put another way, the "dung" construct > you're railing against is a way of pointing out to reviewers "this is the > bit that requires @trusted, everything else is @safe and you don't have to > worry about that". It's a way of making the part that needs @trusted stand > out. Yes, that all is completely wrong. > If this isn't "proper use" of @trusted, then fine, but we *do* need *some* > way of restricting the scope of what's allowed in a @trusted function, > especially in a large collaborative project like Phobos. I think we're good as we are. I'm actually in wonder how such amazingly corrupted use resulted from notions that are simple and easy to use. I don't see how making matters more complicated would improve the state of affairs. > Right now, marking > the entire function @trusted is basically a free-for-all license that > whatever garbage code you wish to throw into this function, you can do it > and @safe code will still continue to work (even though it shouldn't). Apparently garbage code can be introduced easier in @safe functions by means of @trusted local functions that are not trustworthy at all. It's a net negative - instead of seeing @trusted on the function and thinking, "alright let's take a look", I see @safe at the top and nicely get lulled in a sense of security, just to see a few lines down that the function affords itself any amount of unsafety by means of @trusted functions impossible to trust. It's just amazing. > I > mean, come on, if there's a large PR that touches 50 files and makes 5 > changes in several 500-line functions, are you seriously expecting reviewers > to have to read the entire 500 lines *and* understand all of its subtleties, > just because it's @trusted? Most likely what will happen is that unsafe code > will slip in without anyone noticing, and before you know it, the function > is completely unsafe yet it's still marked @trusted. For this reason, > @trusted needs to be kept as restricted as possible. Please understand you're arguing for something much worse.

(In reply to hsteoh from comment #29) > One idea that occurred to me (though it may be a bit too late to implement) > is that @trusted functions remain under @safe requirements except for > @system blocks within the function body, e.g.: > > ----- > // This is hypothetical syntax, the exact syntax is not important, > // it's the idea behind it. > int myTrustedFunc(int x) @trusted { > int x = *cast(int*)null; // Compile error: unmarked unsafe operation in > @trusted function > @system { > enum magicAddress = 0x900D1DEA; > int y = *cast(int*)magicAddress; // OK, unsafe operation allowed in > @system block > } > > free(null); // Compile error: cannot call @system function outside > @system block > return ...; > } > ----- > > This way, reviewers know to scrutinize everything inside the @system block, > while the code outside is mechanically verified not to introduce more > @system operations to the function. No, please. Let's not make matters even more complicated and more opened to abuse.

(In reply to hsteoh from comment #28) > But still, the nagging issue is, how do we restrict the scope of @trusted > inside a function body? Agreed. This is especially necessary for template functions where the @trusted-ness of the whole function might depend on the template arguments. Many of them *need* something to the effect of what std.file is doing (or () @trusted {} ()) so that they can be @trusted at all, as it would be wrong to apply the attribute at the function level. Nota bene: Both Go and Rust offer finer granularity than whole functions for marking code as unsafe, the former using special intrinsics, the latter using "unsafe" blocks.

(In reply to hsteoh from comment #28) > But still, the nagging issue is, how do we restrict the scope of @trusted > inside a function body? By encapsulating the unsafe part in an @trusted local function. But that function must STILL present a safe interface. Merely wrapping it with "here is an unsafe operation" may fly with other languages, but it is not acceptable in D. Apply this rule: "If the compiler cannot verify memory safety in code marked @safe, then you did it wrong." This code, for example, flunks that rule: static trustedCloseHandle(HANDLE hObject) @trusted { return CloseHandle(hObject); }

This also flunks that rule: @safe T* func(T* p) { @trusted { p += 5; } return p; } If it is not clear why, please let me know.

@Andrei: any @safe function can call a @trusted function that may contain arbitrary unsafe operations. Just because something is marked @safe at the top guarantees nothing.

(In reply to Walter Bright from comment #33) > By encapsulating the unsafe part in an @trusted local function. But that > function must STILL present a safe interface. Merely wrapping it with "here > is an unsafe operation" may fly with other languages, but it is not > acceptable in D. May I suggest that you try to understand the issue at hand instead of repeating the same non-solution over and over again? For a small sampling of code that can't easily be rewritten to follow your demands, check std.array. (Again, "() @trusted {…}()" would arguably be the better solution, but that's an inliner problem waiting to be fixed.)

(In reply to hsteoh from comment #35) > @Andrei: any @safe function can call a @trusted function that may contain > arbitrary unsafe operations. Just because something is marked @safe at the > top guarantees nothing. This is a misunderstanding of what @trusted is. It's very important that we clear this up. Your misunderstanding seems to be that the CALLER of @trusted code must be careful to use it safely. This is incorrect. @trusted code needs to be reviewable for safety by ONLY looking at the @trusted code body. NOT the way the @trusted code is used. For example: @trusted void foo() { auto p = malloc(3); free(p); } is correct use of trust. The following is incorrect: @trusted void* tmalloc(size_t n) { return malloc(n); } @trusted void tfree(void* p) { return free(p); @safe void foo() { auto p = tmalloc(3); tfree(p); } Make sense?

(In reply to Walter Bright from comment #34) > This also flunks that rule: > > @safe T* func(T* p) { > @trusted { > p += 5; > } > return p; > } So does this: --- @trusted void claimsToBeSafe(ref T* p) { p += 5; } @safe T* func(T* p) { claimsToBeSafe(p); return p; } --- And this: --- @safe T* func(T* p) { @trusted void claimsToBeSafe(ref T* p) { p += 5; } claimsToBeSafe(p); return p; } --- What is your point? @trusted can be used incorrectly. Nothing new and exciting so far.

(In reply to David Nadlinger from comment #36) > May I suggest that you try to understand the issue at hand instead of > repeating the same non-solution over and over again? For a small sampling of > code that can't easily be rewritten to follow your demands, check std.array. std.array is a big module. You'll need to be much more specific.

(In reply to Walter Bright from comment #39) > (In reply to David Nadlinger from comment #36) > > May I suggest that you try to understand the issue at hand instead of > > repeating the same non-solution over and over again? For a small sampling of > > code that can't easily be rewritten to follow your demands, check std.array. > > std.array is a big module. You'll need to be much more specific. grep for "trusted[A-Z]". For example, trustedCast() in join().

(In reply to hsteoh from comment #35) > @Andrei: any @safe function can call a @trusted function that may contain > arbitrary unsafe operations. Just because something is marked @safe at the > top guarantees nothing. Agreed - and that's exactly why we shouldn't abuse it.

(In reply to David Nadlinger from comment #40) > (In reply to Walter Bright from comment #39) > > (In reply to David Nadlinger from comment #36) > > > May I suggest that you try to understand the issue at hand instead of > > > repeating the same non-solution over and over again? For a small sampling of > > > code that can't easily be rewritten to follow your demands, check std.array. > > > > std.array is a big module. You'll need to be much more specific. > > grep for "trusted[A-Z]". For example, trustedCast() in join(). Sigh, std.array has become another disaster area we need to clean.

(In reply to David Nadlinger from comment #38) > What is your point? > > @trusted can be used incorrectly. Nothing new and exciting so far. His point is that gainful use of @trusted is for safe interfaces with unsafe implementations, not make-believe that unsafe functions are actually safe. It's a very good point.

(In reply to David Nadlinger from comment #40) > grep for "trusted[A-Z]". For example, trustedCast() in join(). For reference: static U trustedCast(U, V)(V v) @trusted { return cast(U) v; } That's a fine example of what I'm talking about as a complete misunderstanding of what trust is supposed to be doing. It's putting the entire onus of using the cast correctly on the CALLER. It is not checkable without checking the supposedly @safe code that uses it, which has defeated the purpose of @safe. The use in join() looks like this (paraphrasing): @safe join() { ... safe code ... return trustedCast(); } I understand you want the ...safe code... to be checked for safety. Here's how to do it: @trusted join() { @safe { ... safe code ... } return cast(U) v; }

(In reply to Walter Bright from comment #44) > I understand you want the ...safe code... to be checked for safety. Here's > how to do it: > > @trusted join() { > @safe { > ... safe code ... > } > return cast(U) v; > } That's beautiful. Too bad it doesn't work :o).

I was a bit curious where these misunderstandings came from. Turns out, example code for C# 'unsafe' code presents not only unsafe code, but an unsafe interface: https://msdn.microsoft.com/en-us/library/aa288474(v=vs.71).aspx#vcwlkunsafecode_readfileexample public unsafe int Read(byte[] buffer, int index, int count) { int n = 0; fixed (byte* p = buffer) { if (!ReadFile(handle, p + index, count, &n, 0)) return 0; } return n; } Note that there's no guarantee that index is within the length of buffer[]. The poor sot cannot simply review Read(), he's got to review EVERY SINGLE CALLER of read(). Since this is taught as correct usage of 'unsafe', it's something we're going to have to regularly auger against.

(In reply to Andrei Alexandrescu from comment #45) > That's beautiful. Too bad it doesn't work :o). I did say I paraphrased it. I've come out against supporting @trusted { ... code ... } because it made it too easy to do bad things with it. But I did use it to make for a simpler example :-)

(In reply to Walter Bright from comment #47) > (In reply to Andrei Alexandrescu from comment #45) > > That's beautiful. Too bad it doesn't work :o). > > I did say I paraphrased it. I've come out against supporting > > @trusted { ... code ... } > > because it made it too easy to do bad things with it. But I did use it to > make for a simpler example :-) I think it would be funny if a @trusted function with fewer than two statements was automatically rejected by the compiler. Since an unverifiable, but @safe operation requires two actions, one to dive into unsafe territory, and a second one to dive back out, any sound use of @trusted *must* have two or more statements. Destroy! :-)

(In reply to Walter Bright from comment #44) > I understand you want the ...safe code... to be checked for safety. No. I want the "...safe code...", which is in fact "...code that might be safe depending on the template arguments...", to trigger attribute inference as usual, while the compiler should trust me on the potentially-unsafe-but-manually-verified part (such as the cast, or a temporary allocation, etc.). The hypothetical @safe blocks won't help there. @trusted blocks would. I seriously don't see how you could view the latter as more dangerous than applying the attribute to the whole function. In fact, the difference might even boil down to just setting STCtrusted on a smaller scope instead of the function scope. There is still a big, red "@trusted" token in the code to catch the reviewer's eye (well, the color obviously depends on your syntax highlighter settings). In fact, I think that @trusted blocks make it easier to write and review high-quality code, as they help to reduce the amount of lines that need to be reviewed. (Of course, having two @trusted blocks in what would otherwise be a three-liner might not be worth it in that regard, as Andrei pointed out.)

(In reply to Zach the Mystic from comment #48) > I think it would be funny if a @trusted function with fewer than two > statements was automatically rejected by the compiler. Since an > unverifiable, but @safe operation requires two actions, one to dive into > unsafe territory, and a second one to dive back out, any sound use of > @trusted *must* have two or more statements. Destroy! :-) I'm kidding, but unfortunately, it does seem like @trusted's purpose is never going to be easily communicated. Maybe something in the documentation saying that a @trusted function must have more than one action to be used correctly?

(In reply to Andrei Alexandrescu from comment #42) > Sigh, std.array has become another disaster area we need to clean. What about just giving it a try? You might even discover that there actually is a language issue to be solved here (assuming that the current situation is unacceptable).

(In reply to David Nadlinger from comment #51) > (In reply to Andrei Alexandrescu from comment #42) > > Sigh, std.array has become another disaster area we need to clean. > > What about just giving it a try? You might even discover that there actually > is a language issue to be solved here (assuming that the current situation > is unacceptable). This. Judging by this thread there seems to be a huge distance between language authors and people trying to actually use those language features in practice. It is hard to communicate when it is simpyl assumed that I don't understand how @trusted and @safe work (and are supposed to work) instead of addressing actual real-world problems that we encounter.

(In reply to David Nadlinger from comment #51) > (In reply to Andrei Alexandrescu from comment #42) > > Sigh, std.array has become another disaster area we need to clean. > > What about just giving it a try? Give what a try? The cleanup?

(In reply to Dicebot from comment #52) > (In reply to David Nadlinger from comment #51) > > (In reply to Andrei Alexandrescu from comment #42) > > > Sigh, std.array has become another disaster area we need to clean. > > > > What about just giving it a try? You might even discover that there actually > > is a language issue to be solved here (assuming that the current situation > > is unacceptable). > > This. > > Judging by this thread there seems to be a huge distance between language > authors and people trying to actually use those language features in > practice. It is hard to communicate when it is simpyl assumed that I don't > understand how @trusted and @safe work (and are supposed to work) instead of > addressing actual real-world problems that we encounter. Do you have any response to the counterarguments made to yours? I think it's rather clear how Walter and I stand, and also where you are wrong.

I don't see any counterarguments, only trying to explain what @trusted is suppposed to mean (which I am perfectly aware of). It does not address "break @trusted by adding code" concern which is a different thing - and which we were trying to fix by this idiom within available language limitations.

(In reply to Dicebot from comment #55) > I don't see any counterarguments, only trying to explain what @trusted is > suppposed to mean (which I am perfectly aware of). It does not address > "break @trusted by adding code" concern which is a different thing - and > which we were trying to fix by this idiom within available language > limitations. The illusion that @trusted wrappers around essentially unsafe operations make any difference has been shattered.

You shouldn't consider those wrapper really @trusted - they have _never_ supposed to be ones. It is simply a workaround for bunch of language limitations. I'd love to have something better to address the issue - not regress to worse solution because workaround was not good enough.

(In reply to Dicebot from comment #57) > You shouldn't consider those wrapper really @trusted - they have _never_ > supposed to be ones. It is simply a workaround for bunch of language > limitations. I'd love to have something better to address the issue - not > regress to worse solution because workaround was not good enough. We do have something better: @trusted functions that offer safe interfaces with unsafe implementations. This is the right thing to do. This whole fine-granularity of trusted mini-functions adding no safety is a gross abuse of a good notion.

(In reply to Andrei Alexandrescu from comment #53) > (In reply to David Nadlinger from comment #51) > > (In reply to Andrei Alexandrescu from comment #42) > > > Sigh, std.array has become another disaster area we need to clean. > > > > What about just giving it a try? > > Give what a try? The cleanup? Yes. Or think about why e.g. marking a template function that iterates over a range @trusted in its entirety would be almost always wrong, at least without explicitly constraining the range regarding @safe-ty of its primitives. To me, it seems rather obvious that @trusted blocks would be A Good Thing. To you and/or Walter, it seems to be clear that they are The Root of All Evil. I figure it would save us a lot of discussion if you had a look at real-world code where their conceptual equivalent is useful right now. (Just to be clear: Yes, nested functions that just pretend to be safe are ugly, but immediately invoked delegates or real @trusted blocks aren't an option right now. It's the concept that is important here.)

(In reply to Andrei Alexandrescu from comment #58) > (In reply to Dicebot from comment #57) > > You shouldn't consider those wrapper really @trusted - they have _never_ > > supposed to be ones. It is simply a workaround for bunch of language > > limitations. I'd love to have something better to address the issue - not > > regress to worse solution because workaround was not good enough. > > We do have something better: @trusted functions that offer safe interfaces > with unsafe implementations. This is the right thing to do. This whole > fine-granularity of trusted mini-functions adding no safety is a gross abuse > of a good notion. "those aren't the droids you're looking for" approach does not work on me, sorry. @trusted functions longer than few lines are inherently unmaintainable unless implementation is supposed to be read-only (like with bindings). Saying "no, you don't really have the problem" doesn't make it go away. Especially when I have already tried approach you suggest and experienced how it fails in practice.

(In reply to David Nadlinger from comment #59) > (In reply to Andrei Alexandrescu from comment #53) > > (In reply to David Nadlinger from comment #51) > > > (In reply to Andrei Alexandrescu from comment #42) > > > > Sigh, std.array has become another disaster area we need to clean. > > > > > > What about just giving it a try? > > > > Give what a try? The cleanup? > > Yes. Or think about why e.g. marking a template function that iterates over > a range @trusted in its entirety would be almost always wrong, at least > without explicitly constraining the range regarding @safe-ty of its > primitives. > > To me, it seems rather obvious that @trusted blocks would be A Good Thing. > To you and/or Walter, it seems to be clear that they are The Root of All > Evil. I just think they'd be more open to misuse and abuse. I do agree they can be used well, too. > I figure it would save us a lot of discussion if you had a look at > real-world code where their conceptual equivalent is useful right now. Well I just did - std.file does count for real-world code. It's just not right. > (Just to be clear: Yes, nested functions that just pretend to be safe are > ugly, but immediately invoked delegates or real @trusted blocks aren't an > option right now. It's the concept that is important here.) The concept is: best use of @trusted is to mark a safe function that cannot be automatically proven @safe. All that wrapping realloc as trustedRealloc and then cheering that we got safe code going is nonsense.

(In reply to Dicebot from comment #60) > (In reply to Andrei Alexandrescu from comment #58) > > (In reply to Dicebot from comment #57) > > > You shouldn't consider those wrapper really @trusted - they have _never_ > > > supposed to be ones. It is simply a workaround for bunch of language > > > limitations. I'd love to have something better to address the issue - not > > > regress to worse solution because workaround was not good enough. > > > > We do have something better: @trusted functions that offer safe interfaces > > with unsafe implementations. This is the right thing to do. This whole > > fine-granularity of trusted mini-functions adding no safety is a gross abuse > > of a good notion. > > "those aren't the droids you're looking for" approach does not work on me, > sorry. I don't see how this is pushing your argument further. Make a technical point and it'll be well appreciated. > @trusted functions longer than few lines are inherently unmaintainable > unless implementation is supposed to be read-only (like with bindings). > Saying "no, you don't really have the problem" doesn't make it go away. > Especially when I have already tried approach you suggest and experienced > how it fails in practice. I see the good code in std.file has gone bonkers.

(In reply to David Nadlinger from comment #49) > No. I want the "...safe code...", which is in fact "...code that might be > safe depending on the template arguments...", to trigger attribute inference > as usual, while the compiler should trust me on the > potentially-unsafe-but-manually-verified part (such as the cast, or a > temporary allocation, etc.). There's simply no way that: static U trustedCast(U, V)(V v) @trusted { return cast(U) v; } can pass muster as trusted code. Trusted code must provide a safe interface, and that template does not. For example, it could be used to convert ints to pointers. The trusted part needs to go up a level in the logic. Looking at join(), it is used thus: static U trustedCast(U, V)(V v) @trusted { return cast(U) v; } return trustedCast!RetType(result); To fix this, I'd start with a rewrite to: static RetType myCast(typeof(result) result) { return cast(RetType)result; } That isn't right, either, but at least it is a step forwards in being constrained to only have to deal with two types instead of any types. I.e. we must not allow converting ints to pointers, or pointers to ints to pointers to doubles, etc. I.e. in order to be safe, result must be safely convertible to RetType. Where's the check for that? If I look upcode, I see: auto result = uninitializedArray!(RetTypeElement[])(length); Whoa! We have result[] being an array of garbage, which is then pretended to be safely convertible to perhaps an array of pointers? Examining the code further, we see an array to initialize result[]. How do we know that every member of result[] is initialized exactly once? Examining the code, I see that we are TRUSTING that code to do the right thing. I.e. the whole thing is, literally, @trusted code. Pretending to infer it as @safe is wrong, it is not @safe code, and not checkable as @safe code. It's @trusted. Also, uninitializedArray() is marked as @trusted. Returning arrays of pointers that consist of unknown garbage cannot be @trusted. uninitializedArray() isn't any more trustworthy than malloc(), and should be marked as @system. Given the state of this code I worry that the entire std.array needs to be reviewed for memory safety. The apparent necessity of: static U trustedCast(U, V)(V v) @trusted { return cast(U) v; } looks like the canary died trying to tell the programmer something was wrong, and got fed to the cat instead of fixed. I am not saying that std.array is broken as far as the users of it are concerned. I am saying that it is broken in that it sidesteps the language's attempts to check safety, and serves as a wrong example on how to use D's features to write memory safe code. (uninitializedArray() is definitely broken, however.) It reminds me of the early days of D2 where people were arguing that the compiler was too strict in its const checking. I believe we finally did reach consensus that it was doing the right thing, and I hope history will repeat itself :-) Yes, some strong words in this message. Makes me hope I didn't make a mistake - but surely if I did it'll get pointed out :-) en garde.

First step: https://github.com/D-Programming-Language/phobos/pull/2963

(In reply to Andrei Alexandrescu from comment #62) > I don't see how this is pushing your argument further. Make a technical > point and it'll be well appreciated. Big chunks of @trusted are maintenance disaster. @trusted completely disabled all compiler safety verification and because of that every time a single line changes in @trusted function it must be completely re-evaluated as a whole during the review to ensure that safety promise stays. Situation becomes worse because compiler is very conservative in what it considers reliably @safe and thus amount of necessary@trusted functions is very big. Combined, that makes using @trusted in a way you propose so impractical that resource-wise it is more effective to completely ignore @safe feature at all - it does not provide enough benefits for such investment. Does that sounds technical enough? > > @trusted functions longer than few lines are inherently unmaintainable > > unless implementation is supposed to be read-only (like with bindings). > > Saying "no, you don't really have the problem" doesn't make it go away. > > Especially when I have already tried approach you suggest and experienced > > how it fails in practice. > > I see the good code in std.file has gone bonkers. I hope _this_ is not your understanding of a "technical point" ;)

Thanks, Walter! This comment alone was more useful and meaningful than rest of this thread combined :) Let me use example from the very same std.file to oppose: S readText(S = string)(in char[] name) @safe if (isSomeString!S) { import std.utf : validate; static auto trustedCast(void[] buf) @trusted { return cast(S)buf; } auto result = trustedCast(read(name)); validate(result); return result; } There are two important bits here: 1) This is verified to be safe only if call to `validate` is safe. Making it all @trusted would allow to change `validate` to @system and silently break the promise. 2) Cast is restricted to string types. Pretty much only way it can go wrong is if function signature changes to allow other types - it is effectively @safe function that compiler fails to identify as one. 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. 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).

(In reply to Dicebot from comment #65) > (In reply to Andrei Alexandrescu from comment #62) > > I don't see how this is pushing your argument further. Make a technical > > point and it'll be well appreciated. > > Big chunks of @trusted are maintenance disaster. Agreed. > @trusted completely > disabled all compiler safety verification and because of that every time a > single line changes in @trusted function it must be completely re-evaluated > as a whole during the review to ensure that safety promise stays. I understand. To summarize: there is merit in marking a function as @safe even though it has a few non-safe portions because the compiler can at least verify those non-safe portions. So, the argument goes, we get a bit more interstitial verification. That's also the case with functions such as trustedOpen, trustedFstat, trustedRead, trustedRealloc, trustedPtrAdd (urgh!), trustedPtrSlicing. Any code added may use them and needs to be checked for safety. I hope this is understood as well. > Situation > becomes worse because compiler is very conservative in what it considers > reliably @safe and thus amount of necessary@trusted functions is very big. > Combined, that makes using @trusted in a way you propose so impractical that > resource-wise it is more effective to completely ignore @safe feature at all > - it does not provide enough benefits for such investment. > > Does that sounds technical enough? Yes, thanks. It is also not convincing, as it seems my arguments are not convincing to you. Again it is getting clear we are reaching irreducible positions on this so arguing the point is not productive. New arguments might change this, so feel free to bring any fresh perspective on the subject. I believe I have a good understanding of your current points. This must go one way or another; it can't go both ways. If you were responsible I trust you would do what you think is best over my unconvincing arguments, and let me decide how to handle your decision in good form. As things are Walter and I must do what we believe is best (starting with https://github.com/D-Programming-Language/phobos/pull/2963) and I trust you will handle our decision in good form. Thank you.

(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.

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.

Loosely related: https://github.com/D-Programming-Language/druntime/pull/1157. That and more like it is definitely good to do.

(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.

(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.

(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.

(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; }

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.

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.

Thanks Steve for the nice summary!

One more argument to the issue: Ugly paradigms we show for our library code readers. And yes, this does matter, as when a person starts with the language he may start from reading its library code (that's exactly what I did). And when you (a newbie) see all these wrappers around system API you starting to ask yourself: is it that "easy way to communicate with C that was stated as a language advantage?" P.S. I tried to start this discussion half a year ago [1] but looks like nobody noticed. I'm happy the problem is finally stated. [1] https://github.com/D-Programming-Language/phobos/pull/2465#issuecomment-53950146

This pull request: https://github.com/dlang/phobos/pull/4786 will help simplify things in std.file by making some of the machinations to work with both arrays and ranges unnecessary.

(In reply to Walter Bright from comment #79) > This pull request: > > https://github.com/dlang/phobos/pull/4786 has been pulled.

THIS ISSUE HAS BEEN MOVED TO GITHUB https://github.com/dlang/phobos/issues/10113 DO NOT COMMENT HERE ANYMORE, NOBODY WILL SEE IT, THIS ISSUE HAS BEEN MOVED TO GITHUB