www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - Feedback Thread: DIP 1035-- system Variables--Community Review Round 1

reply Mike Parker <aldacron gmail.com> writes:
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.
Jun 10
next sibling parent reply Paul Backus <snarwin gmail.com> writes:
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/
Jun 10
parent Dennis <dkorpel gmail.com> writes:
On Wednesday, 10 June 2020 at 11:45:39 UTC, Paul Backus wrote:
 s/member functions/member variables/
Good catch.
Jun 10
prev sibling next sibling parent reply Stanislav Blinov <stanislav.blinov gmail.com> writes:
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.
Jun 10
parent Dennis <dkorpel gmail.com> writes:
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.
Jun 10
prev sibling parent reply Timon Gehr <timon.gehr gmx.ch> writes:
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"
Jun 10
parent Dennis <dkorpel gmail.com> writes:
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.
Jun 10