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

RazvanN <razvan.nitu1305@gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |razvan.nitu1305@gmail.com

--- Comment #19 from RazvanN <razvan.nitu1305@gmail.com> ---
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?

--

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

Stanislav Blinov <stanislav.blinov@gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |stanislav.blinov@gmail.com
           See Also|                            |https://issues.dlang.org/sh
                   |                            |ow_bug.cgi?id=16095

--

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

--- Comment #20 from Stanislav Blinov <stanislav.blinov@gmail.com> ---
It was: https://issues.dlang.org/show_bug.cgi?id=16095

--

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

--- Comment #21 from anonymous4 <dfj1esp02@sneakemail.com> ---
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

--

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

Simen Kjaeraas <simen.kjaras@gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |simen.kjaras@gmail.com

--- Comment #22 from Simen Kjaeraas <simen.kjaras@gmail.com> ---
(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.

--

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

--- Comment #23 from anonymous4 <dfj1esp02@sneakemail.com> ---
f(()const{}); works, but doesn't enforce const for closure.

--

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

Bolpat <qs.il.paperinik@gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |qs.il.paperinik@gmail.com

--- Comment #24 from Bolpat <qs.il.paperinik@gmail.com> ---
(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.

--

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

--- Comment #25 from timon.gehr@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");
}

--

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

--- Comment #26 from Bolpat <qs.il.paperinik@gmail.com> ---
(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.

--

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

--- Comment #27 from timon.gehr@gmx.ch ---
(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.

--