www.digitalmars.com         C & C++   DMDScript  

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

reply Mike Parker <aldacron gmail.com> writes:
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.
Jun 10
next sibling parent Mike Parker <aldacron gmail.com> writes:
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
Jun 10
prev sibling next sibling parent reply WebFreak001 <d.forum webfreak.org> writes:
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.
Jun 10
next sibling parent Dennis <dkorpel gmail.com> writes:
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.
Jun 10
prev sibling parent reply Stanislav Blinov <stanislav.blinov gmail.com> writes:
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.
Jun 10
parent reply Dennis <dkorpel gmail.com> writes:
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.
Jun 10
next sibling parent reply Stanislav Blinov <stanislav.blinov gmail.com> writes:
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.
Jun 10
parent Dennis <dkorpel gmail.com> writes:
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.
Jun 10
prev sibling parent reply Stanislav Blinov <stanislav.blinov gmail.com> writes:
On Wednesday, 10 June 2020 at 14:20:37 UTC, Dennis wrote:
 On Wednesday, 10 June 2020 at 12:36:13 UTC, Stanislav Blinov 
 wrote:
 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.
Memory corruption is undefined behavior, but undefined behavior is not memory corruption. The former is a subset of the latter, they're not equivalent, and so should not be used interchangeably. BUT, so far as those comments in examples are concerned, I do stand corrected regardless (should've consulted a dictionary beforehand), since "corruption" can refer both to corrupting and being corrupt(ed). Apologies.
Jun 10
parent reply Timon Gehr <timon.gehr gmx.ch> writes:
On 10.06.20 17:56, Stanislav Blinov wrote:
 undefined behavior is not memory corruption.
It is legal for an implementation to cause memory corruption whenever it encounters undefined behavior.
Jun 10
parent reply Stanislav Blinov <stanislav.blinov gmail.com> writes:
On Wednesday, 10 June 2020 at 17:02:57 UTC, Timon Gehr wrote:
 On 10.06.20 17:56, Stanislav Blinov wrote:
 undefined behavior is not memory corruption.
It is legal for an implementation to cause memory corruption whenever it encounters undefined behavior.
Just as it is legal to not. Therefore, one is not equivalent to (interchangeable with) the other.
Jun 10
parent Dennis <dkorpel gmail.com> writes:
On Wednesday, 10 June 2020 at 17:11:02 UTC, Stanislav Blinov 
wrote:
 Just as it is legal to not. Therefore, one is not equivalent to 
 (interchangeable with) the other.
Please, let's not discuss at length the nuances in the definition/semantics of the terms "memory corruption" and "undefined behavior", unless there is an important distinction relevant for the DIP.
Jun 10
prev sibling next sibling parent reply Stanislav Blinov <stanislav.blinov gmail.com> writes:
 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.
Jun 10
parent Timon Gehr <timon.gehr gmx.ch> writes:
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); }
Jun 10
prev sibling next sibling parent reply Joseph Rushton Wakeling <joseph.wakeling webdrake.net> writes:
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 don't have any specific feedback yet on the contents of the DIP or their correctness, but I do want to commend the author on how systematic it is in covering the different issues or concerns, and providing concrete examples to illustrate them. This is a standard that all DIPs should aspire to :-)
Jun 14
parent Stanislav Blinov <stanislav.blinov gmail.com> writes:
On Sunday, 14 June 2020 at 10:46:17 UTC, Joseph Rushton Wakeling 
wrote:

 I don't have any specific feedback yet on the contents of the 
 DIP or their correctness, but I do want to commend the author 
 on how systematic it is in covering the different issues or 
 concerns, and providing concrete examples to illustrate them.

 This is a standard that all DIPs should aspire to :-)
Indeed. Problem well defined, solution well presented.
Jun 14
prev sibling next sibling parent reply Dukc <ajieskola gmail.com> writes:
Bump (we don't want to forget a DIP discussion now do we?)
Jun 16
parent Dominikus Dittes Scherkl <dominikus scherkl.de> writes:
On Tuesday, 16 June 2020 at 10:44:46 UTC, Dukc wrote:
 Bump (we don't want to forget a DIP discussion now do we?)
Seems the feature is not controversal - nobody complains, so it seems everybody is pleased with it. Let's move it forward!
Jun 16
prev sibling parent reply Andrei Alexandrescu <SeeWebsiteForEmail erdani.org> writes:
Synopsis: this DIP should be best replaced with a series of bug reports 
related to  safe. We can't define new features whenever the ones we have 
have bugs in them. (Note that accepting the DIP would still allow all of 
its egregious examples to compile.)

A few notes:

* The background section should kickstart the narrative, but the general 
remarks it makes leave the reader wanting. For example: "This can only 
work if the language in which such types are defined has the needed 
encapsulation capabilities." to which the obvious question is, "what's 
wrong with a struct that puts that thing in a private member and brokers 
all access to it?"

* The second example uses:

__traits(getMember, sum, "tag") = 1; // ! ruin integrity of tagged union

The obvious conclusion here is: using __traits(getMember) breaks 
encapsulation, therefore it should not be allowed in  safe code. That is 
a bug in  safe that needs fixing (regardless of the introduction of 
another feature).

The explanation that follows fails to convince:

* "the tag having private visibility only encapsulates at the module 
level, while  trusted is applied at the function level, so it can still 
be violated by functions in the same module" -> the D protection system 
is fundamentally module level. Construing that in an objection for just 
this one feature creates the odd precedent that all other encapsulation 
primitives should be changed or complemented accordingly. That's well 
outside the scope of this one feature.

* "even outside the module, __traits(getMember, ) bypasses private" -> 
that's an obvious bug that needs to be fixed.

* <<The tagged union is just one example; there are many other 
situations where data should only be modified by code that "knows what 
it's doing">> - they call that private.

* "As long as the reference count can be meddled from  safe code, 
freeing the memory cannot be  trusted, making  safe reference counting 
unsupported in D." -> That means there are bugs in  safe, not a need for 
yet another feature with its own liabilities.

* The section title "Existing holes in  safe" contains its own 
conclusion. The obvious answer is, great work on documenting them, they 
need to be fixed!

* And indeed the example with getPtr() illustrates an obvious bug. Safe 
code has no business calling into  system code. It's worth noting, 
again, that even if this DIP is approved, that code would continue being 
problematic.

* The paragraph about bool's problems even has a reference to an 
existing issue. So how does it motivate the introduction of a new feature?

* The quoted Rust discussion is the best argument in the DIP. I don't 
know Rust enough to figure whether they have a solution similar to our 
 safe/ trusted/ system troika, i.e. I'm not sure they'd have a 
workaround just as satisfying as ours. The DIP should discuss that to 
establish applicability. (Also, that Rust proposal is quite controversial.)

Speaking of workarounds, aside from fixing the bugs in  safe, I think 
someone in need for a system variable can use a little module with:

struct SysVar(T) {
     private T value;
     ref T get()  system { return value; }
     void set(ref T rhs)  system { value = rhs; }
}

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.

* The "Alternatives" section should be moved before the bulk of the 
proposal. The narrative should go: here's the problem we have, here are 
the solution within the existing language, here are the problems with 
them, here is the new feature that solves them better.

* "First of all, disallowing the bypassing of private in  safe code is 
not sufficient for ensuring struct invariants. As mentioned in the 
quote, sometimes invariants need to hold onto types that are not unsafe, 
such as int. When there are no pointer members, then private fields can 
still be indirectly written to using overlap in a union, 
void-initialization, or array casting." -> all of these are unsafe 
operations. What am I missing?

* "Finally, it would mean that circumventing visibility constraints 
using __traits(getMember, ...) must become  system or deprecated 
entirely, similarly to .tupleof. This would break all ( safe) code that 

things considered, making private work with  trusted appears to be a 
bigger hassle than introducing checks for  system variables and fields." 
-> I don't understand this argument. Yes you should be able to bypass, 
just not in  safe code. Yes fixing that would break code, as it should. 
Again it's a problem with the feature that won't get solved by adding 
another one. I think the best value that can be derived from this DIP 
might be the necessity of __traits(getPublicMember, ...).

To conclude, it seems this DIP has great value at documenting, arguing, 
and illustrating the failings of  safe, but should at best conclude that 
those bugs need fixing.
Jun 16
parent reply Timon Gehr <timon.gehr gmx.ch> writes:
On 17.06.20 03:12, Andrei Alexandrescu wrote:
 * The background section should kickstart the narrative, but the general 
 remarks it makes leave the reader wanting. For example: "This can only 
 work if the language in which such types are defined has the needed 
 encapsulation capabilities."
It's a bad formulation. This is not about encapsulation.
 to which the obvious question is, "what's 
 wrong with a struct that puts that thing in a private member and brokers 
 all access to it?"
 ...
It does not. The module would. Also, you could add ` safe` code to it that would then in turn break memory safety. If you diff looks like this: - system int x; + int x; That is fishy. If you diff looks like this: + safe void foo(){ x=3; } That's not supposed to be fishy. Furthermore, your suggestion does not solve the issue with unsafe initializers.
 * The second example uses:
 
 __traits(getMember, sum, "tag") = 1; // ! ruin integrity of tagged union
 
 The obvious conclusion here is: using __traits(getMember) breaks 
 encapsulation, therefore it should not be allowed in  safe code. ...
safe means memory safe, not encapsulated. See reflection in Java. For variables that satisfy invariants, the compiler cannot always figure out whether accesses are permissible without some annotations.
 The explanation that follows fails to convince:
 
 * "the tag having private visibility only encapsulates at the module 
 level, while  trusted is applied at the function level, so it can still 
 be violated by functions in the same module" -> the D protection system 
 is fundamentally module level.
The documentation says trusted code has to be audited, not all code in modules that contain a bit of trusted code or no trusted but both system and safe code.
 Construing that in an objection for just 
 this one feature creates the odd precedent that all other encapsulation 
 primitives should be changed or complemented accordingly. That's well 
 outside the scope of this one feature.
 
safe means memory safe, not encapsulated. private variables are variables that have to respect some invariant. system variables are variables that have unsafe values.
 
 * "even outside the module, __traits(getMember, ) bypasses private" -> that's
an obvious bug that needs to be fixed.
It's not obvious that it is a bug (it would need to be defined to be a bug) nor is it obvious that making bypassing of `private` unsafe obviates the need for system variables.
 
 * <<The tagged union is just one example; there are many other situations
where data should only be modified by code that "knows what it's doing">> -
they call that private.
So your suggestion is safe code cannot access any private members? safe code is code written by someone who potentially does not know what they are doing.
 
 * And indeed the example with getPtr() illustrates an obvious bug. Safe code
has no business calling into  system code.
Under current language rules, it's not safe code. That's the problem. Variable initializers have no safety annotations.
 It's worth noting, again, that even if this DIP is approved, that code would
continue being problematic.
No, that's only true for your supposedly equivalent workaround, the DIP solves that problem. It's proposed change (3). However, I think accessing `extern` variables would also need special consideration.
 
 * The paragraph about bool's problems even has a reference to an existing
issue.
The issue is there is undefined behavior in safe code. Fixing it requires design work as there are multiple ways to fix it.
 So how does it motivate the introduction of a new feature?
The DIP introduces a couple concepts related to system variables and defines them.
 
 * The quoted Rust discussion is the best argument in the DIP.
Not really. I don't think you will find additional arguments there.
 I don't know Rust enough to figure whether they have a solution similar to our
 safe/ trusted/ system troika, i.e. I'm not sure they'd have a workaround just
as satisfying as ours.
The Rust workaround would be to create some sort of cell struct with a private member and unsafe accessors and use that.
 The DIP should discuss that to establish applicability. (Also, that Rust
proposal is quite controversial.)
It's similar to some extent but not fully. Rust has unsafe functions and unannotated functions, the equivalent of a trusted function is any function that has at least one `unsafe` block in it. Additionally, functions that can access certain state that unsafe functions can access have to be treated as trusted, but the language does not make it easy to track this. That's why there is a desire for unsafe fields in Rust; to eliminate that last category. This is also why we want it in D, as we can't trust safe code.
 
 Speaking of workarounds, aside from fixing the bugs in  safe, I think someone
in need for a system variable can use a little module with:
 
 struct SysVar(T) {
     private T value;
     ref T get()  system { return value; }
     void set(ref T rhs)  system { value = rhs; }
 }
 ...
You don't need the setter and calling it with an rvalue calls postblit and destructor more times than necessary.
 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. Maybe you can make it work the way you envision, but what is to stop someone from coming along and adding some more safe code to that module? You won't even find the problem by grepping for trusted and nobody wrote any unsafe code.
 
 
 * "First of all, disallowing the bypassing of private in  safe code is not
sufficient for ensuring struct invariants. As mentioned in the quote, sometimes
invariants need to hold onto types that are not unsafe, such as int. When there
are no pointer members, then private fields can still be indirectly written to
using overlap in a union, void-initialization, or array casting." -> all of
these are unsafe operations. What am I missing? 
Simply that those are not inherently unsafe operations. --- union U{ int a; private int b; } void main() safe{ U u; u.a=3; // change b } --- void main() safe{ int x=void; } --- struct S{ private int x; } void main() safe{ S s; (cast(int[])cast(int[1])s)[]=2; } --- Walter has expressed a desire to keep it that way, particularly adamantly for `void` initialization.
Jun 16
next sibling parent Andrei Alexandrescu <SeeWebsiteForEmail erdani.org> writes:
On 6/17/20 2:14 AM, Timon Gehr wrote:
 The documentation says  trusted code has to be audited, not all code in 
 modules that contain a bit of  trusted code or no  trusted but both 
  system and  safe code.
That seems to be a problem with the documentation. The unit of review is the module. A module with system or trusted code needs to be reviewed as a unit.
Jun 17
prev sibling next sibling parent Andrei Alexandrescu <SeeWebsiteForEmail erdani.org> writes:
On 6/17/20 2:14 AM, Timon Gehr wrote:
 * "even outside the module, __traits(getMember, ) bypasses private" -> 
 that's an obvious bug that needs to be fixed.
It's not obvious that it is a bug (it would need to be defined to be a bug)
https://issues.dlang.org/show_bug.cgi?id=20941
 nor is it obvious that making bypassing of `private` unsafe 
 obviates the need for  system variables.
It's an objection to that particular argument.
Jun 17
prev sibling next sibling parent reply Andrei Alexandrescu <SeeWebsiteForEmail erdani.org> writes:
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.
 Maybe you can make it 
 work the way you envision, but what is to stop someone from coming along 
 and adding some more  safe code to that module?
A code review.
Jun 17
next sibling parent reply Stanislav Blinov <stanislav.blinov gmail.com> writes:
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. Private or no private, writing to it is not safe no matter where you do it. No SysVar!insert_all_your_types_here all over the binary; no associated runtime costs (i.e. copies for and in get/set); no associated implementation costs (at least in current language, until such time that moves do get realized).
Jun 17
parent reply Andrei Alexandrescu <SeeWebsiteForEmail erdani.com> writes:
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. 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 main merit of this DIP is to bring attention to the problem and collect a list of issues to look at. 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.
Jun 17
next sibling parent jmh530 <john.michael.hall gmail.com> writes:
On Wednesday, 17 June 2020 at 14:27:17 UTC, Andrei Alexandrescu 
wrote:
 [snip]

 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."
That is a fair point. One potential resolution would be to allow for safe/ trusted/ system (with system the default) variable initialization and prevent taking the address of a system variable in a safe function.
Jun 17
prev sibling next sibling parent reply ag0aep6g <anonymous example.com> writes:
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.
Jun 17
parent reply Andrei Alexandrescu <SeeWebsiteForEmail erdani.com> writes:
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.
Jun 17
parent ag0aep6g <anonymous example.com> writes:
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.
Jun 17
prev sibling next sibling parent Stanislav Blinov <stanislav.blinov gmail.com> writes:
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.
Jun 17
prev sibling parent reply Dennis <dkorpel gmail.com> writes:
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
Jun 17
next sibling parent reply Andrei Alexandrescu <SeeWebsiteForEmail erdani.com> writes:
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.
Jun 17
parent Timon Gehr <timon.gehr gmx.ch> writes:
On 17.06.20 18:30, Andrei Alexandrescu wrote:
 
 * 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.
That's what the DIP proposes. It does not make `bool` a system type, it just explicitly makes it a type with _trap values_. I.e., safe code cannot write arbitrary bit patterns into `bool` memory because some of them do not correspond to either `true` or `false`.
Jun 17
prev sibling parent reply jmh530 <john.michael.hall gmail.com> writes:
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 }
Jun 17
parent Andrei Alexandrescu <SeeWebsiteForEmail erdani.com> writes:
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.
Jun 17
prev sibling parent reply Dennis <dkorpel gmail.com> writes:
On Wednesday, 17 June 2020 at 12:41:57 UTC, Andrei Alexandrescu 
wrote:
 Maybe you can make it work the way you envision, but what is 
 to stop someone from coming along and adding some more  safe 
 code to that module?
A code review.
I thought the whole premise of safe was that code review is inadequate for catching memory corruption bugs.
Jun 17
parent reply Andrei Alexandrescu <SeeWebsiteForEmail erdani.com> writes:
On 6/17/20 9:30 AM, Dennis wrote:
 On Wednesday, 17 June 2020 at 12:41:57 UTC, Andrei Alexandrescu wrote:
 Maybe you can make it work the way you envision, but what is to stop 
 someone from coming along and adding some more  safe code to that 
 module?
A code review.
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.) Modules that have only safe code (no trusted, no system) should provide safety guarantees. The DIP improves on that in that it points to a number of issues with safe that need fixing.
Jun 17
parent reply "H. S. Teoh" <hsteoh quickfur.ath.cx> writes:
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.
Jun 17
parent Andrei Alexandrescu <SeeWebsiteForEmail erdani.com> writes:
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.
Jun 17
prev sibling parent reply jmh530 <john.michael.hall gmail.com> writes:
On Wednesday, 17 June 2020 at 06:14:25 UTC, Timon Gehr wrote:
 On 17.06.20 03:12, Andrei Alexandrescu wrote:
 [snip]
 
 * And indeed the example with getPtr() illustrates an obvious 
 bug. Safe code has no business calling into  system code.
Under current language rules, it's not safe code. That's the problem. Variable initializers have no safety annotations.
It seems to me as if this is the most important point that has been made so far. There are two ways to interpret what Andrei is saying: 1) There is a bug with safe that should be fixed. However, Timon notes that this is not part of the definition of safe and the whole point of the DIP. 2) There is a bug in the program. In this case, he has argued elsewhere that safe-ty reviews should happen on a module basis. That means the review would need to check variable initialization to be sure they are not calling system code. This is because the compiler is not checking them for you. However, the whole point of this DIP is so that the compiler would do those checks for you. Regardless, it means that safe is not currently safe.
Jun 17
parent reply Andrei Alexandrescu <SeeWebsiteForEmail erdani.com> writes:
On 6/17/20 10:30 AM, jmh530 wrote:
 Regardless, it means that  safe is not currently safe.
Affirmative. Clearly this is the problem. Opting into system by means of an variable annotation that is NOT FLAGGED IF MISSING is not helping. Making system opt-in is completely backwards.
Jun 17
parent reply Timon Gehr <timon.gehr gmx.ch> writes:
On 17.06.20 16:37, Andrei Alexandrescu wrote:
 On 6/17/20 10:30 AM, jmh530 wrote:
 Regardless, it means that  safe is not currently safe.
Affirmative. Clearly this is the problem. Opting into system by means of an variable annotation that is NOT FLAGGED IF MISSING is not helping. Making system opt-in is completely backwards.
No, it is not. It is a tool to write correct and efficient trusted code. trusted code has to ensure memory safety regardless of how safe code behaves. The type system and safe checks allow trusted code to make assumptions about safe code. Introducing specific features that restrict what safe code can do is precisely the right way to go about it and it has been the way trusted works from the start. The goal of any modular analysis is to be able to at least assign blame to any problem that ends up cropping up anyway, this is true in particular for D's safe/ system/ trusted and the blame always lies in trusted functions. Without system variables, trusted functions always have to check that their invariants hold. This can range from expensive to completely intractable, in particular if the invariants are related to the state of memory allocators. You are either misunderstanding trusted or arguing in favor of completely redesigning D's safety system and thereby destroying what makes trusted great. In that case we can just as well switch to the inferior Rust `unsafe` model with unclear assumptions and guarantees and get rid of trusted entirely.
Jun 17
parent Andrei Alexandrescu <SeeWebsiteForEmail erdani.com> writes:
On 6/17/20 12:47 PM, Timon Gehr wrote:
 On 17.06.20 16:37, Andrei Alexandrescu wrote:
 On 6/17/20 10:30 AM, jmh530 wrote:
 Regardless, it means that  safe is not currently safe.
Affirmative. Clearly this is the problem. Opting into system by means of an variable annotation that is NOT FLAGGED IF MISSING is not helping. Making system opt-in is completely backwards.
No, it is not.
I was misunderstanding.
Jun 17