Another update on safer by default

A few weeks ago, we discussed evolving safe in OpenD. I looked at inference, but that doesn't give any message by default when you break it. I looked at completely swapping the default, and was quickly skeptical if it was even doable, much less worth it. That post ended with a note on a balancing act.

Since then, I've try to strike that balance, and I think I have succeeded in making a reasonable first step: "Safer by default": https://github.com/opendlang/opend/pull/65 (Just watch mostly the druntime and phobos edits in that PR, as those are where most the work went. A few other arsd updates got pulled in there too, but some of them also required with the update. See that diff in isolation here: https://github.com/adamdruppe/arsd/commit/4e3bf88447fd52868a2486d6e6cb23f67f218bd4 )

The basic idea here is:

• Inference is left the same right now. (I will likely still expand it, but not yet.) Templates, auto returning functions, and any others that currently infer @safety are unchanged.
• Functions marked with @system, @trusted, or @safe are unchanged.
• All other functions are opted into to deprecation warnings as if they were @safe, with several exceptions:
  • Calling other functions is allowed; there are no warnings for calling other system or unannotated functions. This lets you work incrementally without propagating through the whole call tree at once.
  • Taking the address of a local is permitted. This is because a lot of functions do this to pass them to C functions and such, so it was a *very* common thing that felt impractical to deal with it. It certainly can lead to legitimate safety bugs, but the main ones are caught regardless:

    int* foo() {
    	int b;
    	return &b;
    }

    $ opend-dmd sbd
    sbd.d(3): Error: returning & b escapes a reference to local variable `b

    So disabling this check for non-@safe functions reduced the cost of migration without sacrificing too big of a benefit.

    The associated dip1000 checks with this are also silenced by default.

  • Catching Error and other Throwables are permitted. This might be bad form, but I think it is just silly to prohibit it.
  • asm is permitted without additional warning. Sure, asm can do anything, but it is enough of a hassle that it is fair to say any programmer writing some inline asm doesn't need the compiler telling them what they already know; I see the benefit of safe(r) by default as catching mistakes. Something that is already obvious and inconvenient enough to be clearly intentional benefits the least from the added checks.
  • Pointer slicing is permitted. You are slicing the pointer to get to a safer interface! After you slice it, you have a safer interface, since bounds checking is now enabled. Of course, you can slice it incorrectly and violate memory safety similarly as any other pointer arithmetic, but in the spirit of incremental improvements, safer by default, it is allowed to help you step in the right direction.
  • Unsafe casts are permitted... for now, at least. The main use of "unsafe" casts that came up in the test set were related to malloc or casting to immutable, and thus again a step in the right direction in practice, and since it is an explicit cast, this is also, generally speaking, intentional enough that this check is more redundant nagging than helpful.

    That said, maybe in the next iteration, I'll revisit this. Perhaps it could allow casts from void* (and to it, for class references), but not the others, in the default relaxed mode. This way, the more dangerous casts you might accidentally step into still get another layer of warning before you proceed. We'll see.

    I do have to be careful though. While I'm doing incremental updates here, OpenD users would probably not have that same luxury, so I must keep count of the total cost/benefit, and not succumb to recency bias. So... again, we'll see.

  • Using the .ptr property of arrays is permitted. This is another one that I'm a bit mixed on - it bypasses bounds checking, and must be done carefully, but it is also intentional enough - you must explicitly use .ptr - that again, I feel the compiler warning again is more cost than benefit.

  All the other safe checks are enabled as deprecation warnings. You can silence them in one of three ways:

  • Mark it explicitly @system. This silences the warnings without changing the interface; it was implicitly @system before anyway, so nothing has really changed when you do this. The compiler is just drawing your attention to it.

    This is the quickest and safest thing to do if you just want to silence the warning and move on; it has no side effects compared to the old default. (At least when used on the individual function; using a whole block that affects other functions too might do other things. I recommend you always mark functions system/trusted/safe individually.) If you do this, you might consider periodically auditing explicit @system annotations to see if any of them can be reasonably upgraded to the following other options.

  • Mark it explicitly @trusted. This also, per existing old D rules, silences any warnings, but it *does* change the interface - a @trusted function can be called from other explicitly @safe functions. You should only do this if you're sure the implementation is correct and the interface cannot be misused, so don't do it blindly, but if you are sure, it might be a good idea.
  • Change the implementation to use other techniques. Once the compiler draws you attention to a function, you might realize it was just poorly written in the first place and you want to fix it instead! I found two bugs in simpledisplay thanks to these warnings and simply fixed them.

  I'll admit this only enables about half the safe checks in the compiler... but half is better than none, right?

It took some time to annotate druntime to build without warnings after this change, but I was able to get my pong game - and all the arsd modules it used (which is a lot of code, but notable does not include the C ports like arsd.jpeg, which will need additional work, though C ports are one place where @system: at the top might actually be an acceptable solution) - building with this safer-by-default regime in only a couple dozen edits, two of which were fixing actual bugs I had missed before. druntime being painful is expected - it is natural that implementations of garbage collection, threads and fibers, etc., that are found in there would include a lot of pointer manipulation and other system code.

Two real bugs fixed at the cost of twentyish trivial edits I think is a decent return on investment!

I suspect arsd is a kind of middle ground here. It isn't implementing a GC like druntime, but it is doing a lot of interfacing with the operating systems. The fact that druntime was doable in a few hours and arsd's primary modules took mere minutes and surfaced actual improvements to the code and interface tell me that most user code will likely have even lower cost for their benefit with this.

I am likely to merge the PR code to enable this and port the libraries to use it into opend master soon. Not all included libraries are updated - not even all of arsd at this point - but enough is that we can do real work as-is. I just want to run a few more tests, compiling more of my applications with it, before calling it good enough to move on.