[Issue 1983] Delegates violate const

[d-bugmail at puremagic.com]

https://issues.dlang.org/show_bug.cgi?id=1983

--- Comment #25 from timon.gehr at gmx.ch ---
(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");
}

--