On 17.06.20 16:27, Andrei Alexandrescu wrote:
> Not even close. The crux of the matter is that forgetting to add @system to that variable makes @safe code do unsafe things with no diagnostic for the compiler. That's a problem with the safety system, regardless of the adoption of this DIP. We can't say "@safe D code is safe, except of course if you forget to insert @system on key variables, in which case it won't be with no warning."

If you forget @system on a safety-critical variable, then an @trusted function that relies on it does not have a safe interface and is invalid.

We're saying "@safe D code is safe, except when you make a mistake in @trusted code". Relying on a non-@system variable is such a mistake.

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.

On Wednesday, 17 June 2020 at 14:27:17 UTC, Andrei Alexandrescu wrote:
> On 6/17/20 9:19 AM, Stanislav Blinov wrote:
>> On Wednesday, 17 June 2020 at 12:41:57 UTC, Andrei Alexandrescu wrote:
>>> On 6/17/20 2:14 AM, Timon Gehr wrote:
>>>>
>>>>> Embellishments are immediate (ctors, allowing void init, opAssign, etc). To the extent this type can be pried open in @safe code, these are definitely bugs in the existing language.
>>>>
>>>> Not all of them, though the fact that it has a default constructor `this(T value){ this.value = value; }` is a bug.
>>>
>>> Ehm. The structure given was a minimal sketch, not an attempt at a complete implementation. An implementation could choose to disable the constructor or define it as @system etc.
>> 
>> ...or the language would do all that work for you, under this DIP; no new user types required, no ctors/dtors/sets/gets, merely a @system in front of a variable. That something *can* be a library solution does not mean that it *should* be one (*cough* emplace *cough* move *cough*). The DIP *is* a solution to many of the "bugs" with @safe.
>> 
>> Writing @safe code in a module should be no different to writing @safe code outside that module.
>> 
>> @system int x;
>> 
>> Done.
>
> Not even close.

Ahem. As opposed to your SysVar!T? Very close. Very, very, very close.

> The crux of the matter is that forgetting to add @system to that variable makes @safe code do unsafe things with no diagnostic for the compiler. That's a problem with the safety system, regardless of the adoption of this DIP. We can't say "@safe D code is safe, except of course if you forget to insert @system on key variables, in which case it won't be with no warning."

The same holds for SysVar!int, only with added burdens both on the programmer and on the compiler. Plus, if you are considering the forgetfulness rhetoric, one might as well forget to put a @safe on a function, potentially with the same consequences.
int is, inherently, a safe type, therefore programmer's forgetfulness in this case cannot be a consideration for the compiler: that's what review is for.

> The main merit of this DIP is to bring attention to the problem and collect a list of issues to look at.

...while providing a generalized solution to a whole bunch of them.

> Coming from the other side: assume all bugs in @safe mentioned in the DIP are fixed (as they should anyway) - the question is what would be the usefulness of it then. I think a DIP listing the bugs and then motivating its necessity without assuming they won't be fixed would be stronger.

It will enable the programmer to express unsafe types and unsafe values when the compiler would not infer them as such, *without requiring the programmer to concoct arbitrary wrappers*, such as the SysVar!T.

On 6/17/20 11:24 AM, H. S. Teoh wrote:
> 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.)

This will remain a problem. Fundamentally a module containing gnarly things like tagged unions (e.g. std.variant) cannot be modified naively, even if the modified code is @safe. It's nice to have a mechanism to help with that, but it's opt-in so the problem remains.

On 6/17/20 10:59 AM, ag0aep6g wrote:
> On 17.06.20 16:27, Andrei Alexandrescu wrote:
>> Not even close. The crux of the matter is that forgetting to add @system to that variable makes @safe code do unsafe things with no diagnostic for the compiler. That's a problem with the safety system, regardless of the adoption of this DIP. We can't say "@safe D code is safe, except of course if you forget to insert @system on key variables, in which case it won't be with no warning."
> 
> If you forget @system on a safety-critical variable, then an @trusted function that relies on it does not have a safe interface and is invalid.
> 
> We're saying "@safe D code is safe, except when you make a mistake in @trusted code".

This has been the case before.

On Wednesday, 17 June 2020 at 14:27:17 UTC, Andrei Alexandrescu wrote:
> The crux of the matter is that forgetting to add @system to that variable makes @safe code do unsafe things with no diagnostic for the compiler. That's a problem with the safety system, regardless of the adoption of this DIP. We can't say "@safe D code is safe, except of course if you forget to insert @system on key variables, in which case it won't be with no warning.

Forgetting a @system annotation does not allow you to break @safe with DIP 1035. Let's take this example:
```
int* x = cast(int*) 0x0F48;
void main() @safe {
    *x = 3;
}
```
This compiles today. With DIP 1035, x would be _inferred_ @system. Marking it @safe would be an error, marking it @system would be redundant. If the initializer for x were `null`, it would be inferred @safe, but that's okay. Variable `x` won't break a @safe main without bad @trusted or exploiting @safe bugs.

> Coming from the other side: assume all bugs in @safe mentioned in the DIP are fixed (as they should anyway) - the question is what would be the usefulness of it then.

This DIP *is* a fix for those bugs, for which there is no obvious solution. The idea is to have a consistent design, instead of having a list of small enhancements and hole plugs that are half-baked. Notice the trend here:
- https://issues.dlang.org/show_bug.cgi?id=18554 (does not solve union/array casts, undone by 15371, spawns your issue)
- https://issues.dlang.org/show_bug.cgi?id=19646 (incomplete solution, spawns 20347)
- https://issues.dlang.org/show_bug.cgi?id=19968 (incomplete solution, spawns 20148)

So let's say we fix those second round bugs like you suggest, which I will call 'bug fix':

```
import std.bigint;
BigInt x = BigInt(10);

void main() @safe {
    x *= 2;
    // now: compiles, no problem
    // DIP 1035: compiles iff BigInt constructor is @safe, which it is
    // 'bug fix': error: x cannot be assumed to have a safe value
}
```

```
import std.experimental.checkedint;
void foo(int[] x) @safe {
    auto data = cast(Checked!int[]) x;
    // now: compiles, no problem
    // DIP 1035: compiles, no problem
    // 'bug fix': error: Checked!int has private members that are implicitly set
}
```

Ideally, programmers now:
- add @trusted blocks, albeit redundant
- review their entire modules

Pessimistically, programmers:
- just leave things @system
- add @trusted on too large scopes
- make members needlessly public
- make their JSON serializer work on private fields with a custom UDA that signals trusted access, of which some people think it's bad @trusted and other's won't, furthering the discussion

On Wednesday, 17 June 2020 at 15:54:13 UTC, Andrei Alexandrescu wrote:
> On 6/17/20 10:59 AM, ag0aep6g wrote:
>> On 17.06.20 16:27, Andrei Alexandrescu wrote:
>>> Not even close. The crux of the matter is that forgetting to add @system to that variable makes @safe code do unsafe things with no diagnostic for the compiler. That's a problem with the safety system, regardless of the adoption of this DIP. We can't say "@safe D code is safe, except of course if you forget to insert @system on key variables, in which case it won't be with no warning."
>> 
>> If you forget @system on a safety-critical variable, then an @trusted function that relies on it does not have a safe interface and is invalid.
>> 
>> We're saying "@safe D code is safe, except when you make a mistake in @trusted code".
>
> This has been the case before.

Exactly. You insinuated that the meaning of @safe would change with the DIP. It doesn't.

If you forget @system on a variable, you're no worse off than now. But if you remember to add it, you can write proper @trusted code. You practically can't do that at the moment.

On 6/17/20 11:58 AM, Dennis wrote:
> Forgetting a @system annotation does not allow you to break @safe with DIP 1035. Let's take this example:
> ```
> int* x = cast(int*) 0x0F48;
> void main() @safe {
>      *x = 3;
> }
> ```
> This compiles today. With DIP 1035, x would be _inferred_ @system. Marking it @safe would be an error, marking it @system would be redundant.

Cool. So the unsafe part is embedded in the history of x, and its @system attribute carries that to @safe code using the variable. That's akin to https://en.wikipedia.org/wiki/Taint_checking, perhaps the DIP could mention that.

(Note that this is also disallowing a number of correct programs:

int* x = cast(int*) 0x0F48;
void main() @safe {
    x = new int;
    *x = 3;
})

Leading with this example would do good for the paper. Looking through the current narrative, it's difficult to distinguish fixable trouble with @safe from the value added by the proposed feature.

Going through the code samples:

* Good illustratory example of status quo: void main() @safe { int* a = void; ... }

* Sumtype example: move to existing issues that need a fix for __traits(getMember). Replace with a compelling example such as the one above.

* The example with @safe vs. @safe: is not very relevant because fixing it invalidates it.

* Probably getPtr() is a good opener that could go as far back as the rationale. Fundamentally: variables don't carry information about their being initialized in safe code. This is very much akin to tainted.

* The bool story seems like a problem with bool more than anything. It seems excessive to mark something as common and useful as bool as a @system type. The right solution is to fix bool.

Generally confining the issues with @safe that need solving anyway to their own section (or an appendix) and presenting a value proposition independently of that would make the proposal stronger.

On Wednesday, 17 June 2020 at 15:58:01 UTC, Dennis wrote:
> [snip]
> This DIP *is* a fix for those bugs, for which there is no obvious solution. The idea is to have a consistent design, instead of having a list of small enhancements and hole plugs that are half-baked. Notice the trend here:
> - https://issues.dlang.org/show_bug.cgi?id=18554 (does not solve union/array casts, undone by 15371, spawns your issue)
> - https://issues.dlang.org/show_bug.cgi?id=19646 (incomplete solution, spawns 20347)
> - https://issues.dlang.org/show_bug.cgi?id=19968 (incomplete solution, spawns 20148)
>
> So let's say we fix those second round bugs like you suggest, which I will call 'bug fix':
>[snip]

I think it can't hurt to emphasize in the DIP that even if issue 20347 is fixed, then it would only mean that the global variable would still be @system. For instance, the DIP example for getPtr could be changed to below and still not generate an error. Fixing 20347 would just mean that you cannot make the initialization of x @safe.

auto getPtr() @system {return cast(int*) 0x7FFE_E800_0000;}

@system int* x = getPtr();

void main() @safe {
    int y = *x; // = 3; // memory corruption
}

On 6/17/20 12:39 PM, jmh530 wrote:
> On Wednesday, 17 June 2020 at 15:58:01 UTC, Dennis wrote:
>> [snip]
>> This DIP *is* a fix for those bugs, for which there is no obvious solution. The idea is to have a consistent design, instead of having a list of small enhancements and hole plugs that are half-baked. Notice the trend here:
>> - https://issues.dlang.org/show_bug.cgi?id=18554 (does not solve union/array casts, undone by 15371, spawns your issue)
>> - https://issues.dlang.org/show_bug.cgi?id=19646 (incomplete solution, spawns 20347)
>> - https://issues.dlang.org/show_bug.cgi?id=19968 (incomplete solution, spawns 20148)
>>
>> So let's say we fix those second round bugs like you suggest, which I will call 'bug fix':
>> [snip]
> 
> I think it can't hurt to emphasize in the DIP that even if issue 20347 is fixed, then it would only mean that the global variable would still be @system. For instance, the DIP example for getPtr could be changed to below and still not generate an error. Fixing 20347 would just mean that you cannot make the initialization of x @safe.
> 
> auto getPtr() @system {return cast(int*) 0x7FFE_E800_0000;}
> 
> @system int* x = getPtr();
> 
> void main() @safe {
>      int y = *x; // = 3; // memory corruption
> }

Simply and powerfully put, a piece of data with indirections that originated in @system code could force @safe code to do unsafe things. It took me a while to figure this may be the main argument for the feature. A comparison with tainted would be very helpful.