www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - Feedback Thread: DIP 1028--Make safe the Default--Final Review

reply Mike Parker <aldacron gmail.com> writes:
This is the feedback thread for the Final Review of DIP 1028, 
"Make  safe the Default".

===================================
**THIS IS NOT A DISCUSSION THREAD**

Posts in this thread must adhere to the feedback thread rules 
outlined in the Reviewer Guidelines (and listed at the bottom of 
this post).

https://github.com/dlang/DIPs/blob/master/docs/guidelines-reviewers.md

That document also provides guidelines on contributing feedback 
to a DIP review. Please read it before posting here. If you would 
like to discuss this DIP, please do so in the discussion thread:

https://forum.dlang.org/post/jelbtgegkwcjhzwzesig forum.dlang.org

==================================

You can find DIP 1028 here:

https://github.com/dlang/DIPs/blob/5afe088809bed47e45e14c9a90d7e78910ac4054/DIPs/DIP1028.md

The review period will end at 11:59 PM ET on April 8, or when I 
make a post declaring it complete. Feedback posted to this thread 
after that point may be ignored.

At the end of the Final Review, the DIP will be forwarded to the 
language maintainers for Formal Assessment.

==================================
Posts in this thread that do not adhere to the following rules 
will be deleted at the DIP author's discretion:

* All posts must be a direct reply to the DIP manager's initial 
post, with only two exceptions:

     - Any commenter may reply to their own posts to retract 
feedback contained in the original post

     - The DIP author may (and is encouraged to) reply to any 
feedback solely to acknowledge the feedback with agreement or 
disagreement (preferably with supporting reasons in the latter 
case)

* Feedback must be actionable, i.e., there must be some action 
the DIP author can choose to take in response to the feedback, 
such as changing details, adding new information, or even 
retracting the proposal.

* Feedback related to the merits of the proposal rather than to 
the contents of the DIP (e.g., "I'm against this DIP.") is 
allowed in Community Review (not Final Review), but must be 
backed by supporting arguments (e.g., "I'm against this DIP 
because..."). The supporting arguments must be reasonable. 
Obviously frivolous arguments waste everyone's time.

* Feedback should be clear and concise, preferably listed as 
bullet points (those who take the time to do an in-depth review 
and provide feedback in the form of answers to the questions in 
this document will receive much gratitude). Information 
irrelevant to the DIP or is not provided in service of clarifying 
the feedback is unwelcome.
Mar 25
next sibling parent reply ag0aep6g <anonymous example.com> writes:
On 25.03.20 08:03, Mike Parker wrote:
 You can find DIP 1028 here:
 
 https://github.com/dlang/DIPs/blob/5afe088809bed47e45e14c9a90d7e78910ac
054/DIPs/DIP1028.md 
The DIP says about `extern` functions:
 An unmarked extern (D) function will now be considered  safe.
It does not say clearly what happens with other `extern` functions. For unmarked `extern (C)` and `extern (C++)` functions, the DIP should either say that they will still be considered system, or it should say that they become errors. The DIP should also mention `extern (Windows)` and `extern (Objective-C)`, or just have a catch-all sentence about "other `extern` functions".
Mar 25
parent Walter Bright <newshound2 digitalmars.com> writes:
On 3/25/2020 12:29 AM, ag0aep6g wrote:
 The DIP says about `extern` functions:
 
 An unmarked extern (D) function will now be considered  safe.
It does not say clearly what happens with other `extern` functions. For unmarked `extern (C)` and `extern (C++)` functions, the DIP should either say that they will still be considered system, or it should say that they become errors. The DIP should also mention `extern (Windows)` and `extern (Objective-C)`, or just have a catch-all sentence about "other `extern` functions".
Since safe will now be the default, all extern functions will be safe. It shouldn't be necessary to enumerate what declarations are covered by this. It would only be necessary to enumerate declarations which would not be.
Mar 25
prev sibling next sibling parent reply Jonathan M Davis <newsgroup.d jmdavisprog.com> writes:
On Wednesday, March 25, 2020 1:03:16 AM MDT Mike Parker via Digitalmars-d 
wrote:
 This is the feedback thread for the Final Review of DIP 1028,
 "Make  safe the Default".
The line "Interfaces to extern C and C++ functions should always be explicitly marked" is pretty vague from the perspective of a spec. Does it mean that the compiler will require that the programmer mark extern(C) functions as system, trusted, or safe and that extern(C) functions without any of those attributes are illegal? Or does it have a default if not provided, and it's just that it's best practice for the programmer to explicitly specify it? And if it has a default, what is the default? Based on the paragraph above stating that "An unmarked extern (D) function will now be considered safe," I would guess that that means that any functions which aren't extern(D) (be they extern(C), extern(C++), extern(ObjC) or whatever) would be treated as system if not marked otherwise (just like happens now), but I think that the DIP should be explicit about it. There's also the question of declaration vs definition for functions which aren't extern(D). It makes no sense for a function declaration which is not extern(D) to be treated as safe by default, because the compiler can't guarantee its safety. However, I don't think that it would be a problem for non-extern(D) function definitions, because then the compiler _can_ check their safety. Either way, the DIP should be explicit about what happens with declarations and definitions which are not extern(D) - regardless of whether declarations and definitions are treated differently. I'd also argue that safe should not be allowed on function declarations which are not extern(D), because the compiler can't guarantee their safety (meaning that if an explicit safety attribute is supplied, it must either be system or trusted), and allowing safe on non-extern(D) declarations makes it much harder for programmers to grep for trusted code that could be the source of safety problems. In practice, right now, I'm unaware of anyone marking extern(C) declarations as safe, but if someone did, it could be very hard to track down the problem if it were hiding a memory safety bug, and given the ease of marking all declarations and definitions in a module with safe via safe: or safe {}, it wouldn't surprise me if some code bases are accidentally marking extern(C) declarations with safe right now and thus hiding memory safety issues. However, allowing safe on non-extern(D) function _definitions_ should be fine, since then the compiler actually is verifying the code. Regardless, much as I think that the DIP _should_ make it illegal to mark non-extern(D) declarations as safe, that would arguably be an improvement over what the DIP currently does rather than being required for the DIP to actually make sense or be acceptable. - Jonathan M Davis
Mar 25
parent Walter Bright <newshound2 digitalmars.com> writes:
On 3/25/2020 9:57 PM, Jonathan M Davis wrote:
 The line "Interfaces to extern C and C++ functions should always be
 explicitly marked" is pretty vague from the perspective of a spec.
The DIP isn't entirely a spec. When it says "should" it's a recommendation for best practice.
 Based on the paragraph above stating that "An unmarked extern (D) function
 will now be considered  safe," I would guess that that means that any
 functions which aren't extern(D) (be they extern(C), extern(C++),
 extern(ObjC) or whatever) would be treated as  system if not marked
 otherwise (just like happens now), but I think that the DIP should be
 explicit about it.
It's pretty clear that declarations would default to safe. All of them. The discussion feedback indicated that this point wasn't confusing.
 There's also the question of declaration vs definition for functions which
 aren't extern(D). It makes no sense for a function declaration which is not
 extern(D) to be treated as  safe by default, because the compiler can't
 guarantee its  safety. However, I don't think that it would be a problem for
 non-extern(D) function definitions, because then the compiler _can_ check
 their  safety. Either way, the DIP should be explicit about what happens
 with declarations and definitions which are not extern(D) - regardless of
 whether declarations and definitions are treated differently.
 
 I'd also argue that  safe should not be allowed on function declarations
 which are not extern(D), because the compiler can't guarantee their  safety
 (meaning that if an explicit  safety attribute is supplied, it must either
 be  system or  trusted), and allowing  safe on non-extern(D) declarations
 makes it much harder for programmers to grep for  trusted code that could be
 the source of  safety problems. In practice, right now, I'm unaware of
 anyone marking extern(C) declarations as  safe, but if someone did, it could
 be very hard to track down the problem if it were hiding a memory safety
 bug, and given the ease of marking all declarations and definitions in a
 module with  safe via  safe: or  safe {}, it wouldn't surprise me if some
 code bases are accidentally marking extern(C) declarations with  safe right
 now and thus hiding memory safety issues. However, allowing  safe on
 non-extern(D) function _definitions_ should be fine, since then the compiler
 actually is verifying the code. Regardless, much as I think that the DIP
 _should_ make it illegal to mark non-extern(D) declarations as  safe, that
 would arguably be an improvement over what the DIP currently does rather
 than being required for the DIP to actually make sense or be acceptable.
On the other hand, 1. it's a special case inconsistency, which has its own costs and confusion. 2. the compiler cannot verify any extern declarations as being safe, even D ones. It's always going to be up to the user to annotate them correctly. 3. the extern(C) specifies an ABI, it doesn't say anything about how the function is implemented, or even which language it is implemented in. A pretty big chunk of the dmd implementation (80-90%?) is extern(C++) 4. it's trivial to mark a block of C function declarations with system and trivial to audit it. I've done it already to a bunch of druntime headers 5. D's separate compilation model relies on extern declarations where source is not available and safety cannot be machine checked. It's inherent 6. We're just talking about the default. The whole point of safe being the default is that it is far and away the most common case, even for C functions. 7. A function having different attributes depending on whether or not a body is present is surprising behavior 8. annotating "extern(C) void free(void*);" as safe doesn't make it safe, either, again relying on the user 9. what do we do with "nothrow" by default? Say this doesn't apply to extern(C++) functions? Is anyone going to remember all these special cases?
Apr 11
prev sibling next sibling parent reply Dukc <ajieskola gmail.com> writes:
On Wednesday, 25 March 2020 at 07:03:16 UTC, Mike Parker wrote:
 [snip]
There is still this issue, that was brought up at previous review also: Consider large amounts of legacy code modules that mix a lot of templated and non-templated functions. Majority of the non-templated functions are not ` safe`, but a many templates rely on inferring safety based on their arguments. The code is already tested to be reliable, and refactoring it to be safe would not be worth it. This DIP, as it currently stands, would force the maintainers to add ` system` function by function to the codebase, which can be somewhat large effort. If the maintainers could just add ` system:` to top of each module, the migration effort would be greatly reduced. However, they can't because they rely on the templates in the modules inferring ` safe` based on the arguments. I recommend that the DIP proposes to avoid this problem somehow. One possibility is proposing an alternative to ` system:` that does not apply to templates. Another is proposing that by default, regular functions would have their safety inferred the same way it's inferred for templated functions (thanks to Vladimir Panteleev for this idea -wouldn't have occured to me).
Mar 31
parent Walter Bright <newshound2 digitalmars.com> writes:
On 3/31/2020 4:08 PM, Dukc wrote:
 On Wednesday, 25 March 2020 at 07:03:16 UTC, Mike Parker wrote:
 [snip]
There is still this issue, that was brought up at previous review also: Consider large amounts of legacy code modules that mix a lot of templated and non-templated functions. Majority of the non-templated functions are not ` safe`, but a many templates rely on inferring safety based on their arguments. The code is already tested to be reliable, and refactoring it to be safe would not be worth it. This DIP, as it currently stands, would force the maintainers to add ` system` function by function to the codebase, which can be somewhat large effort. If the maintainers could just add ` system:` to top of each module, the migration effort would be greatly reduced. However, they can't because they rely on the templates in the modules inferring ` safe` based on the arguments. I recommend that the DIP proposes to avoid this problem somehow. One possibility is proposing an alternative to ` system:` that does not apply to templates. Another is proposing that by default, regular functions would have their safety inferred the same way it's inferred for templated functions (thanks to Vladimir Panteleev for this idea -wouldn't have occured to me).
The DIP says: "Functions such as template functions, nested functions, and lambdas that are not annotated currently have their safe / system attribute inferred. This behavior will not change." which takes care of this.
Apr 11
prev sibling next sibling parent reply John Colvin <john.loughran.colvin gmail.com> writes:
On Wednesday, 25 March 2020 at 07:03:16 UTC, Mike Parker wrote:
 This is the feedback thread for the Final Review of DIP 1028, 
 "Make  safe the Default".
[snip]
 https://github.com/dlang/DIPs/blob/5afe088809bed47e45e14c9a90d7e78910ac4054/DIPs/DIP1028.md
I think one detail of the DIP should be amended: The section making non-D-mangled - e.g. extern(C) - functions safe by default. Points against: 1) existing extern(C) declarations - including bindings in libraries in transitive dependencies etc. - will likely be silently wrong in a way that breaks the guarantees of safe. Without careful review and updates, all that code can't be used if safe is a requirement, or nasty memory safety bugs will occur that would otherwise not have happened (because safe without this DIP would have caught them).* 2) It now means that one now has to search for extern(C) as well as trusted to find code that might be breaking safe. Linter rules to support correct usage of the language will be created to ensure all extern(C) declarations have explicit attributes, to avoid the inevitable mistakes that will occur otherwise. This is pretty sad, because the compiler can do a better job. 3) It can be quite hard to ascertain whether it's reasonable to mark a C function as trusted or not. The path of least resistance shouldn't be towards assuming safe without consideration. In summary, this invites new bugs and creates new work to avoid them. Suggestions to improve the DIP: A) non-D-mangled functions should be system by default, or B) the recommendation in the DIP "Interfaces to extern C and C++ functions should always be explicitly marked." should be enforced by the compiler. * yes, I know there are many cases where it will be totally fine, because no new safe (inferred or explicit) code is being written calling those functions (or any functions relying on attribute inference that do call those functions), or because existing complicated meta-programming won't spit out new calls in unexpected places and accidentally be inferred as safe. But those cases aren't all the important cases.
Apr 06
parent Walter Bright <newshound2 digitalmars.com> writes:
See my reply to Jonathan M Davis.
Apr 11
prev sibling next sibling parent jeckel <jeckel12381236 gmail.com> writes:
The extern(C) and friends should be  system by default. It is a 
special case, the intention is to make the unsafe code harder to 
use. Just like with ()  trusted { ... }().

While all extern  declaration can't be verified as being safe, 
this is especially true for extern(C) and extern(C++). As the 
mangling does not have whether it was  safe or not, unlike 
extern(D). Yes you can still use an extern(D) declaration that is 
unsafe, but that is exactly why this is an unsafe feature that 
shouldn't be allowed in the first place. For a language that 
prioritizes safety.

Take Rust for example. It has no header files, there are no 
"declarations", other than C. Cargos are a first class citizen, 
and is much safer as a result not relying on user error with 
declarations. Declarations to C are unsafe by default. Rust 
doesn't have a "safe"/"trusted" keyword, so they cannot be 
implied to be "safe". They are *always* unsafe.

Pointing to extern(D) and saying that's unsafe as well, so we 
should make extern(C) unsafe too. That's a very bad rationale. We 
should fix extern(D) as well to actually be  safe, like with what 
Rust has done. We shouldn't introduce more  unsafe code because 
something in D is already unsafe.

The declaration itself is what is unsafe. That's why Rust has 
done away with them with Cargo, other than C declarations which 
default to unsafe. This is where a separe compilation model is 
inherently  unsafe. That doesn't mean because one thing that is 
already unsafe, should continue to be unsafe.

I can guarantee you there's more extern(C)  unsafe code out there 
than there is extern(C)  safe code. So it makes sense to make 
extern(C)  unsafe by default, as that is most definitely the 
common case. If that's the kind of rationale you are trying to 
impose onto extern(C) of why it should be  safe by default. It 
large majority of extern(C) code is most definitely not  safe. 
Otherwise citation required.

It's not surprising if a declaration has different attributes 
than it's definition. If it was, it'll be extremely easy to 
figure out. If you use an extern(C) declaration in  safe code, 
you'll get an error saying it isn't safe and needs to be 
explicitly annotated. There's no gotcha, no surprise down the 
line when a bug shows itself. In comparison if there's an unsafe 
extern(C) function being used somewhere because extern(C) 
declarations can be defined *everywhere* including within the 
body of a function.

Even though currently ` safe:` "flows through", it doesn't for 
extern(C) declarations.

      safe:

     void test() {
         extern(C) void foo();
         foo(); // error calling  system function

         extern(C) void foo2() {
             // this is  safe
         }
         foo2(); // ok foo2 is  safe
     }

https://godbolt.org/z/ppsXMG

Even Walter of old knew better than to make extern(C) 
declarations implicitly  safe.

The case with nothrow is pretty simple. C doesn't have exceptions 
so it can always be nothrow. C++ has a nothrow equivalent and is 
included as part of the mangling, so that can be used for C++. So 
nothrow can simply be nothrow by default. It isn't that big of an 
issue as there isn't a case of calling a nothrow function and it 
actually being able to throw an exception. Even if there was, if 
an exception is thrown in a C++ nothrow function it simply 
terminates the application. Yes extern declarations are still 
terrible. It's something that was inherited from C and is 
something we should strive to remove, as Rust has done. Possibly 
in a future DIP.
Apr 11
prev sibling parent Mike Parker <aldacron gmail.com> writes:
On Wednesday, 25 March 2020 at 07:03:16 UTC, Mike Parker wrote:

 The review period will end at 11:59 PM ET on April 8, or when I 
 make a post declaring it complete. Feedback posted to this 
 thread after that point may be ignored.
 Posts in this thread that do not adhere to the following rules 
 will be deleted at the DIP author's discretion:

 * All posts must be a direct reply to the DIP manager's initial 
 post, with only two exceptions:

     - Any commenter may reply to their own posts to retract 
 feedback contained in the original post

     - The DIP author may (and is encouraged to) reply to any 
 feedback solely to acknowledge the feedback with agreement or 
 disagreement (preferably with supporting reasons in the latter 
 case)
This review round ended on April 8. Even so, the rules for the Feedback Thread are still in place. If you'd like to discuss any of the DIP author's comments, please post in the Discussion Thread: https://forum.dlang.org/post/ovllntpiebixbtrbiuxj forum.dlang.org
Apr 11