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

===================================
**THIS IS NOT A DISCUSSION THREAD**

Posts in this thread must adhere to the feedback thread rules outlined in the Reviewer Guidelines (and listed at the bottom of this post).

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

That document also provides guidelines on contributing feedback to a DIP review. Please read it before posting here. If you would like to discuss this DIP, please do so in the discussion thread:

https://forum.dlang.org/post/tgtrbqrjetdveznzxokh@forum.dlang.org
==================================

You can find DIP 1035 here:

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. Feedback posted to this thread after that point may be ignored.

At the end of this review round, the DIP will be moved into the Post-Community Round 1 state. Significant revisions resulting from this review round may cause the DIP manager to require another round of Community Review, otherwise the DIP will be queued for the Final Review.

==================================
Posts in this thread that do not adhere to the following rules will be deleted at the DIP author's discretion:

* All posts must be a direct reply to the DIP manager's initial post, with only two exceptions:

    - Any commenter may reply to their own posts to retract feedback contained in the original post

    - The DIP author may (and is encouraged to) reply to any feedback solely to acknowledge the feedback with agreement or disagreement (preferably with supporting reasons in the latter case)

* Feedback must be actionable, i.e., there must be some action the DIP author can choose to take in response to the feedback, such as changing details, adding new information, or even retracting the proposal.

* Feedback related to the merits of the proposal rather than to the contents of the DIP (e.g., "I'm against this DIP.") is allowed in Community Review (not Final Review), but must be backed by supporting arguments (e.g., "I'm against this DIP because..."). The supporting arguments must be reasonable. Obviously frivolous arguments waste everyone's time.

* Feedback should be clear and concise, preferably listed as bullet points (those who take the time to do an in-depth review and provide feedback in the form of answers to the questions in this document will receive much gratitude). Information irrelevant to the DIP or is not provided in service of clarifying the feedback is unwelcome.

On Wednesday, 10 June 2020 at 08:39:48 UTC, Mike Parker wrote:
> This is the feedback thread for the first round of Community Review of DIP 1035, "@system Variables".

Typo in the "Using private" section under "Alternatives":

> private only acts on the module level, so a @trusted member function cannot assume anything about member functions just because they are private.

s/member functions/member variables/

On Wednesday, 10 June 2020 at 08:39:48 UTC, Mike Parker wrote:

In proposed-changes:

> struct T {
>     @system int y;
>     @system int z = 3; // allowed
>     this(int y, int z) @safe {
>         this.y = y; // allowed, this is initialization
>         this.y = y; // second time disallowed, this is assignment to a `@system` variable
>         this.z = z; // disallowed, this is assignment
>     }
> }

The last one (`this.z = z`) is not an assignment, it is initialization, and should be allowed; that is, unless you want to change the constructor spec for this case.

Further down:

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

...is a bad example for two reasons. First, appropriate implementation of Wrapper won't be calling a copy constructor here, it will move. Second, T may have a @system destructor which would be called when constructor returns.

---

> bool becomes an unsafe type

Nice one.

---

In Examples:

> void main() @safe {
>     Var v = 1.0;
>     v.type = Var.Type.Str; // not allowed after this DIP
>     writeln(v.getString()); // memory corruption: the union messed up the string
> }

The last line is not memory corruption, it's undefined behavior.

> void main() @safe {
>     auto stack = getStack!string();
>     stack._top += 1; // not allowed after this DIP
>     writeln(stack.pop); // memory corruption: garbage string is printed
> }

Ditto.

---

A custom allocator.

> void main() @safe {
>     string[] strArr = customAlloc!string(3);
>     heapIndex = 0; // mess up allocator integrity! not allowed after this DIP.
>     int[] intArr = customAlloc!int(3);
>     intArr[] = -1; // overwrites string array
>     writeln(strArr[0]); // memory corruption: constructed pointer
> }

The `intArr[] = -1;` is memory corruption. Last line, as before, is just UB.

---

A custom slice type

...is self-contradicting. "...one does not want 4 or 8 bytes to store the length when 2 suffices... ...SmallSlice!T is no larger than a regular T[]---it still fits in two CPU registers."

> struct SmallSlice(T) if (__traits(isPOD, T)) {
>     @system private T* ptr = null;
>     @system private ushort _length = 0;
>     ubyte[size_t.sizeof-2] extraSpace; // 2 on 32-bit, 6 on 64-bit

How, then, is it "small", if SmallSlice!T.sizeof == (T[]).sizeof ? That you only use two out of (4 or 8) bytes doesn't make it "small" :)

---

A zero-terminated string

> void main() @safe {
>     Cstring c = cStringLiteral!"hello";
>     c.ptr = new char('D'); // not allowed after this DIP
>     writeln(c.length); // memory corruption: strlen likely goes past the single 'D' character
> }

As before, that last line is not memory corruption, it's UB. Corruption happens on the "not allowed after this DIP" line.

On 10.06.20 10:39, Mike Parker wrote:
> This is the feedback thread for the first round of Community Review of DIP 1035, "@system Variables".
> ...

I think this is a very well-written DIP that addresses an important problem.

However, I don't think the examples should be using `assert` to validate input data. At the very least, those asserts should be in `in` contracts, but even then, I am not sure if the semantics of `assert` supports your use case. In particular, does `-release` mean "disable memory safety checks" like `-noboundscheck` does?

(Besides that, probably `assert` should not be used at all, at least outside of contracts, if you care about memory safety:

https://dlang.org/spec/contracts.html

"Undefined Behavior: The subsequent execution of the program after an assert contract is false.")


Also, making initialization of `@system` variables `@safe` is not sound. `@system` variables are variables that need to satisfy additional invariants. The constructor has to establish those invariants. Memory safety cannot depend on the correctness of a `@safe` constructor.

Consider the following slightly adapted example from the DIP:

enum Opcode : ubyte {
    decrement, increment, print,
}

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
    }
}

int gCounter;
void decrementImpl() {gCounter++;};
void incrementImpl() {gCounter--;};
void printImpl() {import std; writeln(gCounter);};

immutable void function()[3] jumpTable = [
   &decrementImpl, &incrementImpl, &printImpl,
];

void execute(VmInstruction[] code) @trusted {
    foreach(instruction; code) {
        // indexing using .ptr to avoid bounds checks
        jumpTable.ptr[instruction.opcode]();
    }
}

void main() @safe {
    auto code = [VmInstruction(cast(Opcode)20)];
    execute(code);
}



Minor:
- "Ownsership and borrowing in D"
- "static initializtion"

On Wednesday, 10 June 2020 at 12:36:13 UTC, Stanislav Blinov wrote:
> The last one (`this.z = z`) is not an assignment, it is initialization, and should be allowed; that is, unless you want to change the constructor spec for this case.

Yes. I tried out some examples with immutable to figure what dmd counts as initialization and what as assignment, but I think I made a mistake there.

> Further down:
>
>> struct Wrapper(T) {
>>     @system T t;
>>     this(T t) @trusted {
>>         this.t = t; // Oops! Calls a `@system` copy constructor
>>     }
>> }
>
> ...is a bad example for two reasons. First, appropriate implementation of Wrapper won't be calling a copy constructor here, it will move. Second, T may have a @system destructor which would be called when constructor returns.

Maybe the name Wrapper is misleading since it gives some semantic expectations. I'm considering simply calling it 'S'.
The fact that a @system constructor gets implicitly trusted as well only supports the example, which is trying to show the problems with @trusted constructors.

> As before, that last line is not memory corruption, it's UB.

Each example has two consecutive moments:
A) Memory gets in a state that violates the programmer's intended invariants.
B) Because of that, immutable memory is mutated, or memory outside a pointer's allocated block is accessed.

If I understand correctly, you claim that A) should be called memory corruption and B) is undefined behavior, while I call B) memory corruption. In the D specification, corrupting memory is undefined behavior, and undefined behavior can manifest in memory corruption, so when talking about @safe the terms are sometimes used interchangeably. Also, as far as the D language is concerned, there's nothing wrong with e.g. setting the pointer to Cstring to something not null terminated, only the resulting out of bounds memory accesses are.

On Wednesday, 10 June 2020 at 11:45:39 UTC, Paul Backus wrote:
> s/member functions/member variables/

Good catch.

On Wednesday, 10 June 2020 at 13:56:11 UTC, Timon Gehr wrote:
> However, I don't think the examples should be using `assert` to validate input data. At the very least, those asserts should be in `in` contracts, but even then, I am not sure if the semantics of `assert` supports your use case. In particular, does `-release` mean "disable memory safety checks" like `-noboundscheck` does?

Personally I often find myself writing `if (!x) {assert(0);}` instead of `assert(x);` for exactly that reason: I want to assert something that's necessary for @safe.
I'm not sure if that's the idiomatic way to do it though. I'll change the examples to use that unless you have a better suggestion.

> Memory safety cannot depend on the correctness of a `@safe` constructor.

> Consider the following slightly adapted example from the DIP:

You could say that the problem is that `execute` is wrongly `@trusted` since it assumed something about `opcode` that is not enforced by VmInstruction. I get what you're saying though, and it comes back to "@trusted assumptions about @safe code", which unfortunately did not come to concensus.

https://forum.dlang.org/thread/rahiuh$2t8h$1@digitalmars.com?page=1

I'll come back to this later in the discussion thread, but the short version is:
Ideally all important trusted parts of the program have a @trusted annotation nearby, but I'm afraid it is infeasible. I was unable to find a satisfying design, though I'm open to suggestions.

> Minor:
> - "Ownsership and borrowing in D"
> - "static initializtion"

Thanks.