Discussion Thread: DIP 1035-- at system Variables--Community Review Round 1

[H. S. Teoh]

On Wed, Jun 17, 2020 at 10:30:52AM -0400, Andrei Alexandrescu via Digitalmars-d wrote:
> On 6/17/20 9:30 AM, Dennis wrote:
[...]
> > I thought the whole premise of @safe was that code review is
> > inadequate for catching memory corruption bugs.
> 
> Modules that contain @trusted code need to be reviewed manually. We
> need to make clear in the documentation that it's not only the
> @trusted bits in the module; it's the entire module. (That is the case
> independently on the adoption of the DIP.)
[...]

This sounds good in theory, but it presents a significant practical
challenge: in a large codebase like, say, Phobos, to use a familiar
example, where a significant number of individuals contribute code,
reviewing an entire module is an onerous task, since one would have to
do this every time anything in the module is changed. (Without thorough
review, you cannot say with 100% assurance that a one-line change
somewhere won't have effects that percolate throughout the module and
interact in potentially unsafe ways with @trusted code.)

Given the relatively large sizes of your typical Phobos module,
reviewers are unlikely to be aware of every @trusted function in the
module and their potential interactions -- in fact, in modules like
std.algorithm.*, reviewers may not even be familiar with all of the
contents of that module, let alone have sufficient knowledge of all the
uses of @trusted therein and their potential interactions, and yet they
are put in the position of having to review a code change in the context
of the entire module.

This is a good idea in theory, but in practice presents significant
problems to overcome.  The incentives are all wrong: one is motivated to
just ignore the rest of the module (too large, too much time required)
and review only a subset of the code that ought to be reviewed (let's
just look at the function being changed, or, slightly more
optimistically, glance at a few @trusted bits and just hope for the
best).

It would seem, to me, that a far better design would be one in which the
parts of a module that need to be reviewed, when presented with some
changeset X to be reviewed, are easily searched for (e.g., search for
"@trusted", or whatever other construct we choose/invent for this
purpose) and evaluated with respect to X, and the rest of the code can
be left alone.  I.e., we want the machine to tell us where we need to
look to identify potentially unsafe code, rather than leave it up to
convention or impractical policies ("review the entire module").


T

-- 
In theory, software is implemented according to the design that has been carefully worked out beforehand. In practice, design documents are written after the fact to describe the sorry mess that has gone on before.