Thread overview
Feedback Thread: DIP 1028--Make @safe the Default--Final Review
Mar 25
ag0aep6g
1 day ago
Dukc
March 25
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.
March 25
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".
March 25
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.
6 days ago
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



1 day ago
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).