This is the discussion thread for the first round of Community Review of DIP 1035, "@system Variables":

https://github.com/dlang/DIPs/blob/148c78e6e7eeb5609715cb31a45c0aa5c8ebdce7/DIPs/DIP1035.md

The review period will end at 11:59 PM ET on June 24, or when I make a post declaring it complete. Discussion in this thread may continue beyond that point.

Here in the discussion thread, you are free to discuss anything and everything related to the DIP. Express your support or opposition, debate alternatives, argue the merits, etc.

However, if you have any specific feedback on how to improve the proposal itself, then please post it in the feedback thread. The feedback thread will be the source for the review summary I write at the end of this review round. I will post a link to that thread immediately following this post. Just be sure to read and understand the Reviewer Guidelines before posting there:

https://github.com/dlang/DIPs/blob/master/docs/guidelines-reviewers.md

And my blog post on the difference between the Discussion and Feedback threads:

https://dlang.org/blog/2020/01/26/dip-reviews-discussion-vs-feedback/

Please stay on topic here. I will delete posts that are completely off-topic.

On Wednesday, 10 June 2020 at 08:38:31 UTC, Mike Parker wrote:

> However, if you have any specific feedback on how to improve the proposal itself, then please post it in the feedback thread. The feedback thread will be the source for the review summary I write at the end of this review round. I will post a link to that thread immediately following this post.

The Feedback Thread is here:

https://forum.dlang.org/post/teoiwvqqpfqcyfnduvhc@forum.dlang.org

On Wednesday, 10 June 2020 at 08:38:31 UTC, Mike Parker wrote:
> This is the discussion thread for the first round of Community Review of DIP 1035, "@system Variables":
>
> https://github.com/dlang/DIPs/blob/148c78e6e7eeb5609715cb31a45c0aa5c8ebdce7/DIPs/DIP1035.md
>
> [...]

I really like the idea of having safety on variables instead of trying to protect it with private as it makes @safe much harder to accidentally abuse in a single file or with traits. I really like the DIP but I don't think the changes to __traits(getFunctionAttributes) might be worth any breakage and the inferring of safety on initial assignment "when the compiler knows that a function is safe" is kind of going against the current attribute inferring implementation in D.

First I propose a solution to this problem:

struct Wrapper(T) {
    @system T t;
    this(T t) @trusted {
        this.t = t; // Oops! Calls a `@system` copy constructor
    }
}

I don't see a solution proposed to this pitfall in the DIP but wouldn't this be solvable from a library function? Something like

ref T trustedAssign(T)(const return ref T value) @trusted { return *cast(T*)&value; }

struct Wrapper(T) {
    @system T t;
    this(T t) @safe {
        this.t.trustedAssign = t; // "cast away @system"
    }
}

Well considering dip1000 and other factors the function might not be this trivial but the general idea of having a function to access @system as @trusted/@safe would work, no?


> __traits(getFunctionAttributes) may be called on variables and fields

regarding this: I would like to keep getFunctionAttributes away just in case someone __traits(compiles) it to check for a function. (sounds like a reasonable enough thing someone would do I think)


> bool getValidBool()   pure @system {return true;}
> bool getInvalidBool() pure @system {return *(cast(bool*) &ub);}
> 
> bool b0 = getValidBool();   // despite unsafe type with @system initialization expression, inferred as @safe
> bool b1 = getInvalidBool(); // inferred as system

and

> Variables and fields without annotation are @safe unless their initial value is not @safe
> [...]
> An exception to the above rules is made on unsafe types when the compiler knows the resulting value is safe.

I think this doesn't fit into how D currently infers attributes. Instead the @system should probably be part of the return type in some way. Otherwise this is making it not possible to use a D library using a .di file the same as using a .d file (which currently works fine) - For template functions this is fine like all other attributes are inferred but for normal functions I think this is some kind of inferring we don't want to start. Maybe a possible solution would be having the safety as some kind of attribute on the return type, but I'm not too sure on that.

On Wednesday, 10 June 2020 at 10:00:40 UTC, WebFreak001 wrote:
> I don't see a solution proposed to this pitfall in the DIP but wouldn't this be solvable from a library function? Something like

The currently proposed solution is to not mark the constructor @trusted but @safe.

> ref T trustedAssign(T)(const return ref T value) @trusted { return *cast(T*)&value; }

Such a function is easily abused.
```
void main() @safe {
    immutable x = 3;
    x.trustedAssign = 7;
}
```

> Well considering dip1000 and other factors the function might not be this trivial but the general idea of having a function to access @system as @trusted/@safe would work, no?

There is control flow checking in the constructor that does not go across function boundaries, so I think it would require an intrinsic or language change.

> regarding this: I would like to keep getFunctionAttributes away just in case someone __traits(compiles) it to check for a function. (sounds like a reasonable enough thing someone would do I think)

That's fair. I'll change that.

> I think this doesn't fit into how D currently infers attributes. Instead the @system should probably be part of the return type in some way.

I don't want this code to break because `x` becomes @system here:
```
bool foo() {return true;} // @system function by default
bool x = foo();
```

To me it's similar to value range propagation.
```
short x0 = cast(short) 500; // allowed, cast int to short
short x1 = 500; // also allowed, compiler knows value is in range
```

> Otherwise this is making it not possible to use a D library using a .di file the same as using a .d file (which currently works fine)

It's not possible to initialize variables from extern functions anyway:

```
bool foo();
bool x = foo();
```

Error: foo cannot be interpreted at compile time, because it has no available source code

> Maybe a possible solution would be having the safety as some kind of attribute on the return type, but I'm not too sure on that.

The return value of a @system function is deemed @system in the general case, but if a @system function returns a bool `false` or `true` at compile time I see no reason to still pretend it may be an invalid bool.

On Wednesday, 10 June 2020 at 10:00:40 UTC, WebFreak001 wrote:

> struct Wrapper(T) {
>     @system T t;
>     this(T t) @trusted {
>         this.t = t; // Oops! Calls a `@system` copy constructor
>     }
> }
>
> I don't see a solution proposed to this pitfall in the DIP but wouldn't this be solvable from a library function? Something like
>
> ref T trustedAssign(T)(const return ref T value) @trusted { return *cast(T*)&value; }
>
> struct Wrapper(T) {
>     @system T t;
>     this(T t) @safe {
>         this.t.trustedAssign = t; // "cast away @system"
>     }
> }

That one is just an incorrect example in the DIP. It uncoditionally trusts a (possibly @system) destructor of T. And yes, absolutely, it should not use the copy constructor at all. It should use a core.lifetime : moveEmplace, a call to which it *can* put in a @trusted delegate if, and only if, T itself is @safe (because moveEmplace would write into the argument).
And later down the line, if/when Walters move ctors come into play, `this.t = t`; would just [hopefully *have to*, if Walter listens] move, thus removing destructor call, and leaving in inference from the move ctor.

From the feedback thread:

On Wednesday, 10 June 2020 at 13:56:11 UTC, Timon Gehr wrote:
> Memory safety cannot depend on the correctness of a `@safe` constructor.

> struct VmInstruction {
>     @system Opcode opcode; // this need not be private, just a valid enum member
>     this(Opcode opcode) @safe {
>         this.opcode = opcode; // forgot to check
>     }
> }
> ...
> void main() @safe {
>     auto code = [VmInstruction(cast(Opcode)20)];
>     execute(code);
> }

Good observation. It *almost* feels like there's a case lurking for disallowing enum casts in @safe code.

On 10.06.20 16:15, Stanislav Blinov wrote:
>  From the feedback thread:
> 
> On Wednesday, 10 June 2020 at 13:56:11 UTC, Timon Gehr wrote:
>> Memory safety cannot depend on the correctness of a `@safe` constructor.
> 
>> struct VmInstruction {
>>     @system Opcode opcode; // this need not be private, just a valid enum member
>>     this(Opcode opcode) @safe {
>>         this.opcode = opcode; // forgot to check
>>     }
>> }
>> ...
>> void main() @safe {
>>     auto code = [VmInstruction(cast(Opcode)20)];
>>     execute(code);
>> }
> 
> Good observation. It *almost* feels like there's a case lurking for disallowing enum casts in @safe code.

enum Opcode{
    decrement, increment, print,
}
...
void main() @safe {
    auto code = [VmInstruction(Opcode.increment|Opcode.print)];
    execute(code);
}

On Wednesday, 10 June 2020 at 12:59:16 UTC, Stanislav Blinov wrote:
> That one is just an incorrect example in the DIP. It uncoditionally trusts a (possibly @system) destructor of T.

That's the point of the example. It shows the problems with @trusted constructors and argues for allowing initialization of @system variables in @safe code. Note that this was a very late addition, so it did not get draft review and could use some improvements.

On Wednesday, 10 June 2020 at 15:13:57 UTC, Dennis wrote:
> On Wednesday, 10 June 2020 at 12:59:16 UTC, Stanislav Blinov wrote:
>> That one is just an incorrect example in the DIP. It uncoditionally trusts a (possibly @system) destructor of T.
>
> That's the point of the example. It shows the problems with @trusted constructors and argues for allowing initialization of @system variables in @safe code. Note that this was a very late addition, so it did not get draft review and could use some improvements.

Yes it could, as the example emphasizes the copy ctor but makes no mention of a dtor :) I.e. the ctor in the example cannot possibly be explicitly marked @trusted unless @safe-ty of T's dtor is ascertained first.

On Wednesday, 10 June 2020 at 15:33:14 UTC, Stanislav Blinov wrote:
> Yes it could, as the example emphasizes the copy ctor but makes no mention of a dtor :) I.e. the ctor in the example cannot possibly be explicitly marked @trusted unless @safe-ty of T's dtor is ascertained first.

It's an example of bad @trusted. The copy constructor is an example of why it's bad, it's not the only reason. I'll mention the destructor for completeness though.