Bug 1983 – Delegates violate const

There appears to be a large hole in the const system, demonstrated by the following code. import std.stdio; class A { private int x_ = 1; void x(int n) { x_ = n; } int x() const { return x_; } } void main() { const(A) a = new A; // The following correctly won't compile // a.x = 2; // a.x(2); // **** BUT THIS WILL **** (&a.x)(2); writefln(a.x); // writes 2 } The problem is that the expression (&a.x) has type void delegate(int x) wheras it /should/ have type void delegate(int x) const Unfortunately, there is no way to declare a const delegate (by which I mean, a delegate whose context pointer is typed const). I see this as a big problem. Without the ability to specify the constancy of delegate contexts, it will be impossible to declare pure delegates. (If we ever get parallel foreach, we're going to need pure delegates).

IMHO the problem here isn't the type of (&a.x) but the fact that it's allowed to compile at all. Creating a delegate to a non-const member function of a const object should be a compile-time error.

> ------- Comment #1 from [email protected] 2008-04-10 05:55 ------- > IMHO the problem here isn't the type of (&a.x) but the fact that it's allowed > to compile at all. Same thing. You just stated the problem differently. a.x = 2; won't compile because a.x is typed const. Wheras (&a.x)(2); will compile because (&a.x) is not typed const. The fact that it compiles is a /symptom/. We can treat the symptom, but I'd rather treat the disease. To treat the disease, the type system must allow types such as ReturnType delegate(Params...) const to exist. Likewise ReturnType delegate(Params...) invariant and eventually, we're even going to need ReturnType delegate(Params...) invariant pure Janice

Janice Caron wrote: >> ------- Comment #1 from [email protected] 2008-04-10 05:55 ------- >> IMHO the problem here isn't the type of (&a.x) but the fact that it's allowed >> to compile at all. > > Same thing. You just stated the problem differently. > > a.x = 2; > > won't compile because a.x is typed const. Wheras > > (&a.x)(2); > > will compile because (&a.x) is not typed const. The fact that it > compiles is a /symptom/. I disagree. As long as (&a.x) for mutable a and const x (and similar cases with invariant, etc.) doesn't compile, there's no need for 'delegate() const'. See below. > We can treat the symptom, but I'd rather > treat the disease. To treat the disease, the type system must allow > types such as > > ReturnType delegate(Params...) const > > to exist. Likewise > > ReturnType delegate(Params...) invariant > > and eventually, we're even going to need > > ReturnType delegate(Params...) invariant pure I can see the need for the pure variant, but not any of the others. I see no reason to distinguish between a delegate to a const method and one to a normal method, as long as you can't create a delegate to a method using an object of inappropriate constness. Who (except again in case of 'pure') cares if the object pointed to by the void* .ptr changes if the method is called? Obviously whoever created the delegate must have had mutable access to it[1] (since that should IMHO be the only way to create such a delegate), so they have every right to create a delegate that mutates it. In other words: since you don't have a useful explicit reference to the object, who cares if it's const or not? [1] Or invoked undefined behavior by somehow casting away const, in which case it doesn't matter *what* happens.

Bug 3656 seems to be another example of this.

Posted patches: https://github.com/D-Programming-Language/dmd/pull/70

(In reply to comment #3) > Janice Caron wrote: > >> ------- Comment #1 from [email protected] 2008-04-10 05:55 ------- > >> IMHO the problem here isn't the type of (&a.x) but the fact that it's allowed > >> to compile at all. > > > > Same thing. You just stated the problem differently. > > > > a.x = 2; > > > > won't compile because a.x is typed const. Wheras > > > > (&a.x)(2); > > > > will compile because (&a.x) is not typed const. The fact that it > > compiles is a /symptom/. > > I disagree. As long as (&a.x) for mutable a and const x (and similar > cases with invariant, etc.) doesn't compile, there's no need for > 'delegate() const'. See below. > > > We can treat the symptom, but I'd rather > > treat the disease. To treat the disease, the type system must allow > > types such as > > > > ReturnType delegate(Params...) const > > > > to exist. Likewise > > > > ReturnType delegate(Params...) invariant > > > > and eventually, we're even going to need > > > > ReturnType delegate(Params...) invariant pure > > I can see the need for the pure variant, but not any of the others. I > see no reason to distinguish between a delegate to a const method and > one to a normal method, as long as you can't create a delegate to a > method using an object of inappropriate constness. > Who (except again in case of 'pure') cares if the object pointed to by > the void* .ptr changes if the method is called? Obviously whoever > created the delegate must have had mutable access to it[1] (since that > should IMHO be the only way to create such a delegate), so they have > every right to create a delegate that mutates it. > > In other words: since you don't have a useful explicit reference to the > object, who cares if it's const or not? > > > [1] Or invoked undefined behavior by somehow casting away const, in > which case it doesn't matter *what* happens. const delegates are extremely important to parallelism and concurrency, i.e. the routines in std.parallelism. This is because calling a const delegate from multiple threads is _safe_, given guarantees that mutating members/delegates won't be called until after synchronization barriers. (i.e. using const delegates provides race-safety to parallel foreach, reduce, etc.) (Well, except for the corner case of accessing a global shared variable)

*** Issue 3656 has been marked as a duplicate of this issue. ***

Revised pull request: http://d.puremagic.com/issues/show_bug.cgi?id=1983

(In reply to comment #9) > Revised pull request: > http://d.puremagic.com/issues/show_bug.cgi?id=1983 https://github.com/D-Programming-Language/dmd/pull/2130

Commit pushed to master at https://github.com/D-Programming-Language/phobos https://github.com/D-Programming-Language/phobos/commit/977947e7329d5b5c10242a0efb58c85697283e98 Merge pull request #1331 from 9rnsr/fix1983 Supplemental change for Issue 1983 - Delegates violate const

*** Issue 8781 has been marked as a duplicate of this issue. ***

*** Issue 8654 has been marked as a duplicate of this issue. ***

*** Issue 9462 has been marked as a duplicate of this issue. ***

*** Issue 9149 has been marked as a duplicate of this issue. ***

Possible fix from issue 9149: disallow access to delegates in const objects.

*** Issue 16056 has been marked as a duplicate of this issue. ***

issue 16058 - looks like there is some progress on this issue?

Compiling the code in the original bug post yields: issue.d(19): Error: mutable method issue.A.x is not callable using a const a It seems like this was fixed. Maybe close this?

It was: https://issues.dlang.org/show_bug.cgi?id=16095

Nice progress. New test case: --- struct A { void delegate() a; int b; void f(){ a=&g; } void g(){ b++; } int h() const { a(); return b; } } void f(ref const A a) { const int b1=a.h, b2=a.h; assert(b1==b2,"changed"); } unittest { A a; a.f(); f(a); } --- Though issue 16058 allows one to design API that enforces correctness: void f(void delegate() const a); unittest { A a; f(&a.g); } Error: cannot pass argument &a.g of type void delegate() to parameter void delegate() const a

(In reply to anonymous4 from comment #21) > void f(void delegate() const a); > unittest > { > A a; > f(&a.g); > } Sadly, this also disallows a large set of valid use cases, the simplest of which is f((){}). If (){} yielded a void delegate() const, this would not be a problem.

f(()const{}); works, but doesn't enforce const for closure.

(In reply to anonymous4 from comment #21) > Nice progress. New test case: > --- > struct A > { > void delegate() a; > int b; > void f(){ a=&g; } > void g(){ b++; } > int h() const { a(); return b; } > } > > void f(ref const A a) > { > const int b1=a.h, b2=a.h; > assert(b1==b2,"changed"); > } > > unittest > { > A a; > a.f(); > f(a); > } > --- I fail to see how this is a problem. Const correctness isn't as strong as you think it is. You're conflating const and immutable in a non-obvious way. First, consider that the unittest's variable `a` is mutable, so (this is important!) the free function `f` cannot expect its parameter `a` not to change when doing anything. One usually thinks that aliasing is necessary for that (and even here, aliasing occurs, its just rather hidden), but because `f` only takes one `const` parameter and is factually pure (annotate all functions and the unittest `pure` if you like, it compiles, I tried). What you do by calling `a.f()` in the unittest, is creating a mutable reference to `a` in the context pointer of `A.a` (why TF did you have to use names multiple times??). In the const method `h`, the delegate `a` is const which means you cannot assign it like in `A.f`, but calling it is okay. This way, using its mutable context pointer, it mutates the underlying object. While it looks like a `const` method mutates the object, this isn't a const violation. One could argue that the context pointer in a delegate is part of it. Viewing a delegate as a pair (void function(ref Context, ...) fp, Context* ptr) where Context is a suitable struct holding the contents of the context, ptr is transitively part of the struct and in a const method, must be const. If this were an issue of const only, but not immutable, this bug report would be invalid. HOWEVER, this can be rephrased in a completely pure/immutable style. Immutable means much more than const. It is much easier to spot violations of it, since any change is one. I made a version of the quoted code that clearly violates immutable: struct A { void delegate() pure dg; int value; this(int value) pure { this.value = value; this.dg = &this.mut; } void mut() pure { ++value; } int kaboom() pure const { dg(); return value; } } void f(ref const A x) pure { immutable int v1 = x.kaboom; immutable int v2 = x.kaboom; assert(v1 == v2, "changed"); // fails } void main() pure { immutable A a = immutable(A)(0); f(a); // usually fails, but might pass due to optimization: assert(a.value == 0, "changed"); } It's even hard to pin-point which exact part of the code should be an error.

(In reply to Bolpat from comment #24) > (In reply to anonymous4 from comment #21) > > Nice progress. New test case: > > --- > > struct A > > { > > void delegate() a; > > int b; > > void f(){ a=&g; } > > void g(){ b++; } > > int h() const { a(); return b; } > > } > > > > void f(ref const A a) > > { > > const int b1=a.h, b2=a.h; > > assert(b1==b2,"changed"); > > } > > > > unittest > > { > > A a; > > a.f(); > > f(a); > > } > > --- > > I fail to see how this is a problem. Const correctness isn't as strong as > you think it is. There's not such thing as "const correctness" in D, that's a C++-ism. D const and C++ const are not the same thing. > You're conflating const and immutable in a non-obvious way. > First, consider that the unittest's variable `a` is mutable, so (this is > important!) the free function `f` cannot expect its parameter `a` not to > change when doing anything. It can expect const pure methods called with const arguments not to change anything, and the caller can expect that nothing changes because it passes exclusively const parameters. There's absolutely no expectation that there can be mutable aliasing hidden anywhere, because const is supposed to be transitive. > One usually thinks that aliasing is necessary > for that (and even here, aliasing occurs, its just rather hidden), but > because `f` only takes one `const` parameter and is factually pure (annotate > all functions and the unittest `pure` if you like, it compiles, I tried). > What you do by calling `a.f()` in the unittest, is creating a mutable > reference to `a` in the context pointer of `A.a` (why TF did you have to use > names multiple times??). In the const method `h`, the delegate `a` is const > which means you cannot assign it like in `A.f`, but calling it is okay. No, calling it should not be okay. That's the bug. You can't call a mutable method on a `const` context. > This > way, using its mutable context pointer, it mutates the underlying object. > While it looks like a `const` method mutates the object, this isn't a const > violation. > ... It does not just look like it, of course it is a const violation. The context pointer of a `const(void delegate()pure)` has no business being mutable. > One could argue that the context pointer in a delegate is part of it. > Viewing a delegate as a pair (void function(ref Context, ...) fp, Context* > ptr) where Context is a suitable struct holding the contents of the context, > ptr is transitively part of the struct and in a const method, must be const. > ... Sure, that's precisely how it works and you can't call that function with a const context as it requires a mutable context. > If this were an issue of const only, but not immutable, this bug report > would be invalid. > ... Absolutely not, this entire argument is basically "even though it is obviously a const violation, it actually is not, because there is a mutable reference to the same data somewhere else in your program". That other reference is not reachable through the arguments of the pure function. D's type system is supposed to enforce strong aliasing guarantees. > HOWEVER, this can be rephrased in a completely pure/immutable style. > Immutable means much more than const. It is much easier to spot violations > of it, since any change is one. > ... I agree that this is a better example, but it demonstrates precisely the same issue. `const` references are not supposed to be able to change the data, this is exactly why immutable can implicitly convert to const. Note that mutable data that was created in a strongly pure context can implicitly convert to immutable, maybe that can make it more obvious that the const violation is actually a problem. But also note that in D, `const`-qualified references being used to change anything is UB even if no immutable data is involved at all: the optimizer may assume it does not happen. > I made a version of the quoted code that clearly violates immutable: > > struct A > { > void delegate() pure dg; > int value; > > this(int value) pure > { > this.value = value; > this.dg = &this.mut; > } > > void mut() pure { ++value; } > > int kaboom() pure const > { > dg(); > return value; > } > } > > void f(ref const A x) pure > { > immutable int v1 = x.kaboom; > immutable int v2 = x.kaboom; > assert(v1 == v2, "changed"); // fails > } > > void main() pure > { > immutable A a = immutable(A)(0); > f(a); > // usually fails, but might pass due to optimization: > assert(a.value == 0, "changed"); > } > > It's even hard to pin-point which exact part of the code should be an error. The problem is that you shouldn't be able to call "dg" in "kaboom", as its type is `const(void delegate()pure)`. It would have to be at least something like `const(void delegate()pure const)`. Finally, maybe Walter thinks the code snippet has UB anyway as it creates internal references into a struct. The code does not compile with `@safe`, but I think any demonstration of type system unsoundness should. Here's the example adapted so it is accepted as @safe: @safe: class A{ void delegate()pure dg; int value; this(int value)pure{ this.value=value; this.dg=&this.mut; } void mut()pure{ ++value; } int kaboom()pure const{ dg(); // this should not compile, we are calling a mutable delegate with a const context return value; } } void f(ref const A x)pure{ immutable int v1=x.kaboom; immutable int v2=x.kaboom; assert(v1==v2, "changed"); // usually fails, but might pass due to optimization } void main()pure{ immutable a=new A(0); // pure constructor, so this is okay f(a); // usually fails, but might pass due to optimization: assert(a.value==0,"changed"); }

(In reply to timon.gehr from comment #25) > > It's even hard to pin-point which exact part of the code should be an error. > > The problem is that you shouldn't be able to call "dg" in "kaboom", as its > type is `const(void delegate()pure)`. It would have to be at least something > like `const(void delegate()pure const)`. My position is that distinguishing delegates for mutability of the context is a bad idea. Thinking about it for a while, I come to the conclusion the problem is elsewhere, namely uniqueness deduction is broken: > this(int value) pure > { > this.value = value; > this.dg = &this.mut; > } The mere fact that the struct has a delegate field means a reference to it *can* exist in it. Therefore, the result of the constructor cannot be assumed to be unique. The implicit cast to immutable is invalid. IMO, the way the type system currently works, this is the issue. > Finally, maybe Walter thinks the code snippet has UB anyway as it creates > internal references into a struct. As you showed, it can be mitigated using a class instead of a struct. I haven't tried. > void main()pure{ > immutable a=new A(0); // pure constructor, so this is okay "pure constructor, so this is okay": That's where the bad stuff begins. Uniqueness isn't just slapping pure on stuff. A pure delegate need not return a unique result as it can use its context to get its result from. That's basically what happens here. Requiring const or immutable annotations on delegates for the context is a breaking change and hell is it breaking.

(In reply to Bolpat from comment #26) > (In reply to timon.gehr from comment #25) > > > It's even hard to pin-point which exact part of the code should be an error. > > > > The problem is that you shouldn't be able to call "dg" in "kaboom", as its > > type is `const(void delegate()pure)`. It would have to be at least something > > like `const(void delegate()pure const)`. > > My position is that distinguishing delegates for mutability of the context > is a bad idea. No, it's just the obvious way it has to be type checked, because the delegate context is covariant, but the delegate argument is contravariant. struct DG{ void function(Ctx* ctx) dgptr; Ctx* ctxptr; } DG a; a.dgptr(a.ctxptr); // ok const(DG) b; b.dgptr(b.ctxptr); // error Delegates are not above documented type system guarantees. The analogy above is not perfect, because the delegate type `B delegate(A)q` is actually an existential type `∃C. q(C)×(A×q(C)→B)` with an evaluation map `ev[A,B,q]: (∃C. q(C)×const(A×q(C)→B))×A→B`, but this aspect remains the same: `const(∃C. q(C)×(A×q(C)→B)) = ∃C. const(q(C))×const(A×q(C)→B)`. You can't instantiate the evaluation map with any arguments `[A,B,q]` that makes it accept arguments of type `(∃C. const(q(C))×const(A×q(C)→B))×A` unless `q` includes `const` (or `immutable`). However, we still have `B delegate(A)q ⊆ B delegate(A)` because: `B delegate(A)q = ∃C. q(C)×(A×q(C)→B) ⊆ ∃C'. C'×(A×C'→B) = B delegate(A)`, where I have set `C'=q(C)`. Delegate contexts are not magic, they are just an opaque pointer to data. Where exactly do you disagree? > Thinking about it for a while, I come to the conclusion the > problem is elsewhere, namely uniqueness deduction is broken: > > > this(int value) pure > > { > > this.value = value; > > this.dg = &this.mut; > > } > > The mere fact that the struct has a delegate field means a reference to it > *can* exist in it. Therefore, the result of the constructor cannot be > assumed to be unique. Sure, there can be an internal reference. That's by design. However, everything that is not immutable is newly allocated memory and the only way to reach it is through the constructor's result. > The implicit cast to immutable is invalid. No, it's perfectly fine due to the transitivity of immutable. Even if that implicit cast was not allowed, the type system would be broken, it would just be less obvious. > IMO, the way the type system currently works, this is the issue. > ... The type checking of delegates is unsound, because it allows implicit unsafe type coercion in the wrong direction, from supertype to subtype. Pure functions can't change `const` arguments. Implicit hiding of mutation capabilities within `const` structures was never part of the design, and delegate contexts are not exempt from sound type checking. > ... > > > void main()pure{ > > immutable a=new A(0); // pure constructor, so this is okay > > "pure constructor, so this is okay": That's where the bad stuff begins. > Uniqueness isn't just slapping pure on stuff. A pure delegate need not > return a unique result as it can use its context to get its result from. > That's basically what happens here. > > Requiring const or immutable annotations on delegates for the context is a > breaking change and hell is it breaking. Just for `const` or `immutable` delegates. The annotations are not required to call a mutable delegate, you can still remove context qualifiers from mutable delegates as much as you wish, because the context is opaque. This will still work just fine: immutable int x=3; void delegate()immutable dg = ()immutable => x; void delegate() dg2 = dg; Not sure what your issue is. Is there really a lot of code out there that cares about annotating data const/immutable, yet relies on buggy delegate type checking to bypass the annotations? Those projects should not annotate in the first place.

(In reply to timon.gehr from comment #27) It took me some time to think about and come to something like conclusion. Some parts may look like we [Timon Gehr and me] agree, but I'm not entirely sure we are. My mental model of a delegate is struct DG(R, Args...) { alias Context = void*; alias FP = R function(Context, Args); FP fp; Context context; this(FP fp, Context context) { this.fp = fp; this.context = context; } R opCall(Args args) { return fp(context, args); } } too. However, I make myself aware that this is an implementation detail and in principle, other implementations are possible where the context pointer is not actually part of the delegate object but externalized to a global associative array. One would be this: struct DG(R, Args...) { alias Context = void*; alias FP = R function(Context, Args); FP fp; static Context[FP] contexts; this(FP fp, Context context) { this.fp = fp; contexts[fp] = context; } ~this() { // contexts.remove(fp); } R opCall(Args args) { return fp(contexts[fp], args); } As someone in the forums in the discussion of `const` and `immutable` pointed out, stuff that is conceptually part of an object need not literally be part of that object. If `DG` is a delegate type, having `const(DG)` mean the context must not be mutated by a call to that delegate is a perception that comes from seeing a delegate as a functionPtr-context pair. Thanks to your [Timon Gehr's] explanations (the formal stuff wasn't so easy to comprehend, to be honest), I now do not see annotations for how the delegate can do things with its context as a problem now. I see immutable contexts as an absolute win: A pure delegate makes almost no practically useful guarantees, but a pure immutable one does. Now I feel the need to invent terminology: A const/immutable/... [annotated] delegate has those restrictions on how its context pointer is affected by it. A const/immutable/... (type|qualified) delegate cannot be assigned. I will use "type" or "qualified" always, but "annotated" only for emphasis. The annotations are of the same sort as `pure` and `nothrow` and describe the behavior of the function pointer with respect to the value. Type qualifiers describe the variable. As an example, an `immutable` annotated delegate is alias DGa = void delegate(int) immutable and a ´const` qualified delegate is alias DGb = const(void delegate(int)) And those annotations have nothing to do with each other. They mean orthogonal things. A DGa object can be reassigned and calling a DGb object can mutate stuff through its context. Following the example class, I don't think a `const` method should be barred from calling a mutable annotated delegate on the same reasoning it isn't barred from calling mutable methods of other classes. Most likely, the delegate's context has nothing to do with the class. (We made it to prove a point.) Barring a `const` method of a class to only call a `const` annotated member delegate is a major restriction. In the example, we had a `pure` constructor that assigns the address of a mutable method to a mutable delegate member. This is valid, obviously, it's an exact type match. Using the second model of handling contexts with an external AA, it proves that a reference is leaked. That leaked reference invalidates uniqueness. That means the implicit cast to immutable is invalid, too. However, the result is unique when the delegate were annotated `immutable` (or `const`?). Then, the assignment of a MUTABLE member function wouldn't be valid. If we tried to replace the pure constructor by two (or three) ones for explicit mutable and immutable (and const) construction, it becomes more obvious that if an aggregate type contains a mutable annotated (i.e. not `const` or `immutable` or `inout` annotated) delegate, even the result of a `pure` constructor cannot be assumed to yield unique values. I don't think that an `immutable` (or `const`) delegate type variable should extend the qualification down to its context as it would be a breaking change. It would have to if your model is the function-ptr--context pair, but I think this model is misleading. The qualification of the delegate object should only affect assignment of the function ptr and the context, but not references reachable through that context. If it were, a delegate with a context that binds non-`immutable` values cannot be assigned to an `immutable` qualified delegate type variable. The annotation of a delegate should only affect the signature of the function pointer: The context parameter is mutable, const or immutable depending on the annotation. It tells what the function pointer does, not what the context is.

alias Context = void*; ................^^^^^ This is where the problem begins and why I think many fail to recognize the type system holes. `void*` is just an implementation detail. It really shouldn't be part of anyone's mental model **as far the type system is concerned**. Most importantly, the front-end shouldn't even know that the context pointer is represented as void*. Instead, it should follow the type system rules, as if the context pointer is correctly specified. For example: With delegates to struct and class non-static member functions it should be fairly obvious what is the right type: --- struct S { int x; char[] cx; ref char foo(); } S s; auto dg = &s.foo; // Context = S*; immutable S s; auto dg2 = &s.foo; // Context = immutable(S)*; --- With nested functions is where most people get confused: --- auto test() { int x; char[] cx; ref char nested() // Context = (struct { int x; char[] cx; })*; { /* ... */ } return &nested; } ---

(In reply to Bolpat from comment #28) > However, I make myself aware that this is an implementation detail and in principle, other implementations are possible where the context pointer is not actually part of the delegate object but externalized to a global associative array. Sorry, but this falls flat on its face because a `pure` function cannot access a mutable global associative array. The context pointer is indeed part of the delegate.

(In reply to timon.gehr from comment #30) > (In reply to Bolpat from comment #28) > > However, I make myself aware that this is an implementation detail and in > > principle, other implementations are possible where the context pointer > > is not actually part of the delegate object but externalized to a global > > associative array. > > Sorry, but this falls flat on its face because a `pure` function cannot > access a mutable global associative array. The context pointer is indeed > part of the delegate. My AA externalization code was to convey an idea, not to propose an implementation. It was to help people visualize how contexts could be not part of the delegate object. I was well aware about that limitation.

*** Issue 11043 has been marked as a duplicate of this issue. ***

Changing writefln to writeln, let's try the opening example today: --- test.d(19): Error: mutable method `test.A.x` is not callable using a `const` `a` --- where test.d(19) is the line: (&a.x)(2); So the bug has been fixed at some point.

This has not been fixed, even though the original example no longer compiles.

Separate issues should get their own bugzilla entries. The issue that started this thread is fixed.