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

anonymous4 <dfj1esp02@sneakemail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Hardware|x86_64                      |All
                 OS|Linux                       |All

--- Comment #10 from anonymous4 <dfj1esp02@sneakemail.com> ---
You can use a wrapper to mark data as unsafe:

@safe:

const(int)* gInts;

void main() {
    auto s = MyStruct(10);
    gInts = s.ints; //error, can't call @system unwrap
}

struct MyStruct
{
    import core.stdc.stdlib;
    Unsafe!(int*) ints;
    this(int size) @trusted { ints = cast(int*) malloc(size); }
    ~this() { () @trusted { free(ints); }(); }
    scope ptr(this This)() { return ints; }
}

struct Unsafe(T)
{
    private T data;
    @system:
    @disable this(this);
    this(T val){ data=val; }
    void opAssign(T val){ data=val; }
    T unwrap(){ return data; }
    alias unwrap this;
}

--

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

--- Comment #11 from Atila Neves <atila.neves@gmail.com> ---
> @safe applies to functions/methods, not variables/fields. You can't forbid @safe code from accessing a visible variable.

Yes. But the code I presented is in one file for simplicity reasons. In real life MyStruct would be library code. The bug isn't about accessing variables. I understand what you mean about @safe messing up pointers, but I want to prevent client code from doing that, not my own implementation!

> You're still just copying an `int*` around, which isn't unsafe.

Of course it is, that's basically the whole point of Rust and DIP1000.

> Without `scope` on the variable and without a destructor, there is no indication that `s.ints` has a non-infinite lifetime.

>From DIP1000:

"For all global and static variables, lifetime is infinite."
"For values allocated on the garbage collected heap, lifetime is infinite
whilst reachability is dependent on the references in the program bound to
those values."

Algebra of lifetimes lists "*e", "new", "e[i]", "ArrayLiteral" and "ArrayLiteral[constant]" as the only expressions with infinite lifetime.

And again:

"A variable is inferred to be scope if it is initialized with a value that has a non-∞ lifetime."


Therefore, since `auto s = MyStruct(10)` doesn't match any of the above conditions for an infinite lifetime, `auto` or `scope` should be the same thing.

> I'm not sure if I understand that correctly, but this compiles just fine:

I'm arguing it shouldn't.

--

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

--- Comment #12 from ag0aep6g <ag0aep6g@gmail.com> ---
(In reply to Atila Neves from comment #11)
> > You're still just copying an `int*` around, which isn't unsafe.
> 
> Of course it is, that's basically the whole point of Rust and DIP1000.

You're right. Of course DIP 1000 is about restricting how pointers can be copied around. I was completely wrong here. Didn't really think in DIP 1000 terms. Sorry for the noise.


[...]
> From DIP1000:
> 
> "For all global and static variables, lifetime is infinite."
> "For values allocated on the garbage collected heap, lifetime is infinite
> whilst reachability is dependent on the references in the program bound to
> those values."
> 
> Algebra of lifetimes lists "*e", "new", "e[i]", "ArrayLiteral" and "ArrayLiteral[constant]" as the only expressions with infinite lifetime.
> 
> And again:
> 
> "A variable is inferred to be scope if it is initialized with a value that has a non-∞ lifetime."
> 
> 
> Therefore, since `auto s = MyStruct(10)` doesn't match any of the above conditions for an infinite lifetime, `auto` or `scope` should be the same thing.

DIP 1000 on `null`: "lifetime(null) is infinite". `null` is an instance of a
global, I guess.

On function calls, it says that the lifetime of the result is defined in the "section dedicated to discussing functions". Unfortunately, that section doesn't mention "lifetime" even once, and I find it rather hard to figure out what the lifetime of a function call is supposed to be when the function body isn't available for analysis.

So if we leave the pointer as `null`, it has infinite lifetime. And it's okay to copy it around. This is what happens in all the posted snippets that don't have a `malloc` call.

For `malloc(...)` it's not clear to me what the lifetime is supposed to be. Apparently, DMD goes with infinite. The consequence is that the `free` call always comes as a surprise to the compiler. I.e., calling `free` always breaks the @trusted promise, and @safe becomes unreliable. The best we can do with this is using an @trusted `free` anyway and containing the fallout.

What if `malloc(...)` had a zero lifetime instead? Then any access to a field initialized from it would have to be @trusted, because its lifetime would always be considered over. Since @safe code wouldn't be able to even look at the field, @safe might stay reliable. I'm not sure if this could work out.

I suppose the goal is to somehow get access to the field with proper lifetime (i.e. tied to the struct instance, if the destructor `free`s). With the current implementation we might do it like this:

----
--- main.d
import the_ugly;
void* global;
void main() @safe
{
    auto s = S(1);
    int* local = s.p;
    static assert(!__traits(compiles, global = s.p));
}
--- the_ugly.d
struct S
{
    private int* p_;
    int* p() return @safe
    {
        return p_;
    }
    this(int n) @trusted
    {
        import core.stdc.stdlib: malloc;
        p_ = cast(int*) malloc(int.sizeof);
    }
    ~this() @trusted
    {
        import core.stdc.stdlib: free;
        free(p_);
    }
}
----

The problem is of course that the whole module the_ugly must be verified manually. @safe and attribute inference cannot be relied upon. That's brittle.

Could a method like `p` be written if DMD chose a zero lifetime for `malloc(...)` instead of an infinite one? If so, that might be nice. `p` would probably have to be @trusted, but @safe and attribute inference could be relied upon for other methods and `p_` wouldn't even have to be private.


> > I'm not sure if I understand that correctly, but this compiles just fine:
> 
> I'm arguing it shouldn't.

You said it "doesn't compile". Or more precisely: You described some code that "doesn't compile" and I couldn't reproduce it from the description. If the code you thought of is different from the snippet I pieced together, maybe show it if it's interesting.

--

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

--- Comment #13 from Atila Neves <atila.neves@gmail.com> ---
> Apparently, DMD goes with infinite

Only if it's a template this function. As mentioned before, writing out the explicit instantitations for mutable, const and immutable doesn't compile.

That's the bug. Hence the issue title being "DIP1000 defeated if auto used instead of scope in variable declaration with template this member function" instead of "DIP1000 defeated if auto used instead of scope in variable declaration".

dmd is already doing the right thing with `scope ptr() { return ints; }`,
`scope ptr() const { return ints; }` and `scope ptr() immutable { return ints;
}` even if it's `auto s = MyStruct(10)` instead of `scope s = MyStruct(10)`.

It's only if `ptr` is a template this function this bug manifests. There's also a bug with `inout`, but that's another issue:

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

I didn't bother to respond to the rest of your analysis. The crux of the problem here is the interaction of a template function with DIP1000 and inferred lifetimes, and how it differs from the non-template functions.

--

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

Mike Franklin <slavo5150@yahoo.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |slavo5150@yahoo.com

--- Comment #14 from Mike Franklin <slavo5150@yahoo.com> ---
(In reply to Atila Neves from comment #13)

> It's only if `ptr` is a template this function this bug manifests

I suspect that's due to the undocumented inference rules add by https://github.com/dlang/dmd/pull/8408

Note that in DIP25 it states:  "Annotation are deduced for templates and lambdas, but must be explicit for all other declarations".

--

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

--- Comment #15 from ag0aep6g <ag0aep6g@gmail.com> ---
(In reply to Atila Neves from comment #13)
> > Apparently, DMD goes with infinite
> 
> Only if it's a template this function.

No. My statement was about the lifetime of `malloc(...)`, which is called in the constructor. The constructor doesn't have a template this parameter.

Your code has a template this parameter on the method `ptr`. But before `ptr` comes into play, the lifetime of the field has already been set to infinite.

If we initialize the field to something with restricted lifetime, then leaking `s.ptr` gets rejected as it should be:

----
const(int)* gInts;
void main() @safe
{
    int x;
    auto s = MyStruct(&x);
    gInts = s.ptr; /* Error: scope variable s assigned to gInts with longer
lifetime */
}
struct MyStruct
{
    int* ints;
    scope ptr(this This)() { return ints; }
}
----

The problem is that `ints` starts out with infinite lifetime when it's initialized with a `malloc` call. `ptr` just passes the wrong lifetime on.

Also, if anything, it's the inference of the `return` attribute that's the problem. The template this parameter merely triggers that. Empty template parentheses or an `auto` return type have the same effect.


> As mentioned before, writing out the
> explicit instantitations for mutable, const and immutable doesn't compile.
[...]
> dmd is already doing the right thing with `scope ptr() { return ints; }`,
> `scope ptr() const { return ints; }` and `scope ptr() immutable { return
> ints; }` even if it's `auto s = MyStruct(10)` instead of `scope s =
> MyStruct(10)`.

As mentioned before, I can't reproduce this. Please post complete code.

With those definitions for `ptr` I get this code:

----
@safe:

const(int)* gInts;

void main() {
    auto s = MyStruct(10);
    gInts = s.ptr;
}

struct MyStruct
{
    import core.stdc.stdlib;
    int* ints;
    this(int size) @trusted { ints = cast(int*) malloc(size); }
    ~this() { () @trusted { free(ints); }(); }

    scope ptr() { return ints; }
    scope ptr() const { return ints; }
    scope ptr() immutable { return ints; }
}
----

And that compiles just fine. Because the `return` attribute is still inferred.


> It's only if `ptr` is a template this function this bug manifests. There's also a bug with `inout`, but that's another issue:
> 
> https://issues.dlang.org/show_bug.cgi?id=17927

Yes, that's another issue. The method there doesn't have the `return` attribute.

Maybe your point is that the `return` attribute shouldn't be inferred? Then `ptr` wouldn't compile, but you could still make essentially the same mistake by accessing `s.ints` directly.


> I didn't bother to respond to the rest of your analysis. The crux of the problem here is the interaction of a template function with DIP1000 and inferred lifetimes, and how it differs from the non-template functions.

My point is that changing the lifetime of `malloc(...)` could maybe fix this issue and prevent the very similar mistake of copying `s.ints`, while still allowing the `return` attribute to be inferred.

--

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

--- Comment #16 from Mike Franklin <slavo5150@yahoo.com> ---
This thread is hard to follow.  What's the cut-and-paste test case that demonstrates the problem?

--

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

--- Comment #17 from Atila Neves <atila.neves@gmail.com> ---
After reading it again, I think the summary is (copied from earlier) this:

Function template => inferred attributes => scope. But fine, this compiles and shouldn't:

----------
@safe:

const(int)* gInts;

void main() {
    auto s = MyStruct();
    gInts = s.ptr;
}

struct MyStruct {
    int* ints;
    scope ptr(this This)() { return ints; }
}
----------

--

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

--- Comment #18 from Mike Franklin <slavo5150@yahoo.com> ---
`scope ptr(this This)() { return ints; }` is ambiguous.

Did you mean `scope int* ptr(this This)() { return ints; }` or `int* ptr(this
This)() scope { return ints; }`

The former is supposed to apply `scope` to the return value while the latter
should `scope` to the implicit `ref this` parameter.  See
https://github.com/dlang/DIPs/blob/master/DIPs/other/DIP1000.md#scope-function-returns

>From your explanation, I assume you're expecting `scope` to be inferred for the
implicit `ref this` parameter, correct?

--

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

--- Comment #19 from Atila Neves <atila.neves@gmail.com> ---
I can't reproduce the issue anymore. It helps that I now understand DIP1000 a lot better than I did then. This seems to work fine:


--------------------
int* gInts;

void main() @safe {
    scope s = MyStruct();
    gInts = s.ptr;
}

struct MyStruct {
    int* ints;
    auto ptr(this This)() @safe return scope { return ints; }
}
--------------------

It lets you escape the pointer however if you replace `scope` with `auto` when declaring `s`. In any case that'd be a different bug.

--