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.

On 25.03.20 08:03, Mike Parker wrote:
> You can find DIP 1028 here:
> 
> https://github.com/dlang/DIPs/blob/5afe088809bed47e45e14c9a90d7e78910ac4054/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".

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.

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

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

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.

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?

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.

See my reply to Jonathan M Davis.

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.