DIP 1028---Make @safe the Default---Community Review Round 1

[Joseph Rushton Wakeling]

On Thursday, 2 January 2020 at 09:47:48 UTC, Mike Parker wrote:
> This is the feedback thread for the first round of Community 
> Review for DIP 1028, "Make @safe the Default":
>
> https://github.com/dlang/DIPs/blob/1b705f8d4faa095d6d9e3a1b81d6cfa6d688554b/DIPs/DIP1028.md

First, @Walter, thanks for writing this, and @Mike, thanks for 
taking care of the review process :-)

Since I'm co-author of some key bits of the current reviewer 
guidelines, I'll try to follow the questions there fairly 
precisely and see if it makes for some nice structure in my 
feedback.


(1) Is the proposal acceptable as a language change in principle?

Clearly yes: @safe-by-default fits within an observable trend of 
languages getting more and more concerned with provable memory 
safety, and with a related trend of requiring the user to write 
provably safe code.  Rust in particular has significantly raised 
the bar in this space, and proven that such constraints are often 
welcomed by developers.

The should we/shouldn't we questions around this are likely to 
come down to practicalities.

However, this does seem like a fairly major philosophical shift 
in the language design.  Up until now D has (possibly only 
implicitly) followed what amounts to a "permissive by default" 
design philosophy where (apart from really obvious stupidities) 
the default setting is that one can do what one likes, and 
stronger design constraints (including memory safety but also 
e.g. constness and immutability, purity, nogc, nothrow, final 
methods, ...) are readily available but all opt-in.

The existence of a parallel DIP for nothrow-by-default seems to 
confirm this push for a more restrictive-by-default language.  
The two DIPs should be considered on their own merits, so I won't 
try to couple them, beyond noticing the design trend they both 
contribute to.  However, the impact of that trend should be 
considered in terms of the impact on the D development 
experience.  Some thoughts on pros and cons will be covered below.

As a separate issue, it's probably a good idea to take discussion 
of keyword choices off the table for the purposes of this DIP.  
@safe, @trusted and @system have


(2) Is the proposal workable in practice?

-- "Is the proposed feature specified in sufficient detail?" --

There is sufficient spec to make the change happen, per se.  What 
is missing is a sufficiently detailed overview of the practical 
problems that will arise from the migration, and explicit 
proposals for how to deal with them.  This brings us to ...

-- "Are edge cases, flaws, and risks identified and addressed?" --

The identified breaking changes and risks are discussed only at a 
high level.  The proposed solutions (explicit annotation of all 
non-templated functions) are correct per se but don't really 
capture the complexity of the likely reality.  Some examples:

   * @system-by-default means that if a @safe attribute is 
overlooked,
     that bug can be fixed without breaking change: by contrast, 
with
     @safe-by-default, an overlooked @system attribute can't be 
fixed
     without breaking change to the API concerned.

     (Yes, I can hear you now saying that if the function isn't 
@safe
     then the compiler will object and force the user to add 
@system,
     so this oversight won't happen.  But this is about intent: the
     function might be safe in practice, as initially implemented,
     but with no intention to preserve that guarantee.  One 
advantage
     of opt-in constraints is that generally one can be sure that 
the
     developer means them to be there.)

   * As noted already in others' feedback, the use of `@system:` 
to tag
     multiple functions has an asymmetric impact compared to 
`@safe:`.
     In the latter case one likely _wants_ that attribute to apply 
to
     all subsequent functions, including templated ones.  However, 
we
     probably do not want catch-all `@system:` to override the 
inferred
     safety of templated function instantiations.  This edge case 
should
     be discussed, with suggestions for how to address it.

   * The DIP contains no advice or impact assessment for the case 
of
     3rd-party libraries that are no longer actively maintained, 
and
     the consequent risks for obsoleting a large amount of existing
     D code.  Ideally the DIP should contain a robust estimate of 
the
     numbers of projects this might impact, and some discussion of 
the
     pros and cons of that impact, and mitigation strategies.

     The migration plan should include clear steps for how to 
regularly
     remeasure the expected impact on 3rd-party library usability 
over
     time (e.g. as more and more libraries are adapted to support 
the
     new feature).  There should also be explicit criteria for 
deciding
     on what level of impact is (un)acceptable in order to 
transition
     from `--preview=safedefault` to the feature being on by 
default.

   * Issues related to taking the address of local variables 
(mentioned
     several times in this discussion thread) should be discussed, 
with
     reference to other DIPs that address that concern.  It should 
be
     made clear whether finalization of those other features is 
needed
     (or strongly desirable) to finalize @safe-by-default.

   * The impact of @safe-by-default on `extern` APIs should be 
discussed.
     We have no reasonable grounds to assume these functions are 
safe by
     default: the DIP should address how to deal with this (which 
should
     ideally not rely on developer virtue).

   * The impact of @safe-by-default on the ability to write 
`@nogc` code
     should be covered by the DIP, including appropriate 
references to
     other relevant DIPs.

   * The advice in the current draft of the DIP to "annotate 
functions
     that aren't safe with `@trusted` or `@system` should include 
clear
     guidance as to _when_ to use `@trusted` and when to use 
`@system`.
     We don't want to "fix" migration problems by blindly slapping
     `@trusted` onto code that hasn't been properly validated.

     We have already had cases of people trying to do that just to 
get
     stuff to compile with `@safe` 
<https://github.com/msoucy/dproto/pull/79>
     so we should try to avoid the risk of spreading that kind of 
code
     around.  (Rust has to deal with similar concerns, of too many 
devs
     just adding `unsafe` blocks willy-nilly to get the compiler 
off
     their backs, and I've seen similarly problematic uses of 
@trusted
     in even some quite prominent D libraries.)

   * It seems likely that @safe-by-default will increase the 
number of
     occasions that developers have to use @trusted.  The DIP 
should
     try to make some estimate of the amount of impact, and should
     address whether it is necessary (or at least very desirable) 
to
     add support for @trusted code blocks as well as functions (cf.
     what Rust allows with `unsafe`, and feedback by Manu and 
others
     on the problems of needing to define local lambdas to apply 
the
     @trusted attribute to).

These last few examples touch on another missing risk: there is 
no assessment of the expected impact on developer experience.  
Arguably a very nice productivity feature of D is the ability to 
hack readily and only worry about introducing strict constraints 
when one actively wants them.  This is part of what makes D so 
readily usable for everything from small casual scripts to 
large-scale libraries and applications.

Those of us who tend to apply `@safe` willingly and regularly may 
underestimate the impact on users who prefer fast iteration over 
strictly enforced constraints.  I'm particularly concerned that 
it may get too many developers into the habit of unthinkingly 
slapping down `@trusted` on code that doesn't deserve it, rather 
as some Rust developers just `unsafe` lots of things without 
really thinking it through.

It's easy to dismiss those people as architects of their own 
pain, but the problem is how such users can spread bad habits by 
example.  We should perhaps not underestimate the importance of 
consent in submitting to constraints ... :-)

OTOH the positive flipside of imposing constraints by default is 
that it means that the combination of different constraints gets 
much better battle tested: there is a much lower barrier to 
discovering (and hopefully fixing) tricky edge cases.

-- "Are there any platform or architecture pitfalls to be aware 
of?" --

None that I can think of.

-- "Is there an implementation that proves the proposed feature 
works in practice?" --

The basic implementation is likely trivial.  Questions of proof 
need to apply more to the migration path (see below).

-- "Does the DIP consider prior work from other languages?" --

Yes, but the consideration is merely of their existence.  It 
would be good to have a more detailed comparison, discussing how 
they achieve those outcomes, and what the resulting constraints 
and developer experiences are.

-- "If the proposed feature is a breaking change, is there a 
well-defined migration path?" --

This is the crux of the matter.  There are many small finnicky 
impacts that this change will have.

The basic migration path of using `--preview=safedefault` and 
ironing out kinks is sound.  However, what needs to be 
established (which is currently missing) is a clear statement of 
the criteria that will be used to determine when (and if!) it is 
appropriate to transition from the `--preview` feature to having 
@safe-by-default ... by default.

In short, acceptance of moving to the `--preview=safedefault` 
stage should NOT be taken as acceptance of the @safe-by-default 
transition in its entirety.  The DIP should define the definite 
blockers to that transition, and should outline a robust review 
process for the decision to finalize (or abandon) the change.

The core code migration step (adding `@system` to non-templated 
functions without an existing `@safe`, `@trusted` or `@system` 
attribute) ought to be possible to automate: the DIP might 
mandate the creation of such a tool as a requirement before the 
transition can be finalized.


(3) Summary

The proposed feature is a significant breaking change.  If we are 
lucky, the practical impact may be much smaller than one might 
anticipate, but that needs to be robustly established before 
approval can be given to the DIP (or at least, to transitioning 
away from `--preview=safedefault`).

The DIP needs to provide much more detail on the anticipated 
impacts, the migration paths, and the risks for the existing and 
future D ecosystem (with particular attention to how many 
existing codebases may be obsoleted).  It should also clarify if 
any other DIPs or experimental features need to be finalized 
before this DIP can be.

The anticipated impact on both developer and maintainer 
experience should be carefully outlined, with clearly written 
mitigation strategies for the worst pain points.

In short: since nothing stops anyone who cares from having a 
`@safe` codebase right now through the existing opt-in features, 
show us in detail how the pain of transitioning to opt-out is 
really worth it ;-)