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 2020
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 2020
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 2020
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 2020
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 2020
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 2020
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 2020
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 2020
parent Walter Bright <newshound2 digitalmars.com> writes:
See my reply to Jonathan M Davis.
Apr 11 2020
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 2020
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 2020