On Wednesday, 22 August 2018 at 09:23:26 UTC, Walter Bright wrote:

> dip1000 has been around for two years, and its predecessor dip25 several years now. Plenty of time for anyone to comment and/or propose something better.

Part of the problem is that the implementation keeps changing without keeping the documentation in sync.  For example you're implementing all of these inference rules without documenting them:

https://github.com/dlang/dmd/pull/8346
https://github.com/dlang/dmd/pull/8408

I asked you about, instead of inferring the attributes, allowing users to add such logic themselves:

void foo(T)(T x)
    if (__traits(isPointer, T)) { T = scope T; }
{ }

But, then the PR got rubber-stamped, and now here we are.

Mike

On Wednesday, 22 August 2018 at 09:05:13 UTC, Walter Bright wrote:
> On 8/21/2018 8:58 PM, Nicholas Wilson wrote:
>> On Tuesday, 21 August 2018 at 14:31:02 UTC, Atila Neves wrote:
>>> The problem is that the code we write doesn't deal directly with pointers - see the recent confusion in this forum over where `scope` on the left applies to the `this` pointer or the one returned by the member function.
>>>
>>> Kagamin just told me I needed to use `return` instead of `scope` to get things to work and I'm still not sure why.
>> 
>> The way I think about it is if you have a function that takes a pointer, any pointer, and either returns it or a pointer derived from it (dereferencing or indexing) that argument must be marked `return`. In your case it was a pointer derived from `this` so `return` must be applied to `this`.
>
>
> Another way to think about it is this:
>
>    S s;
>    return &s;
>
> We all know that is an error. The idea is to have a way to express that for:
>
>     S s;
>     return s.foo();
>
> and:
>
>     S s;
>     return foo(&s);
>
> so that the compiler knows that the return value of foo() is attached to the lifetime of s. Pretty much everything flows from that.

Would the guideline below be correct?

"Add scope to every non-template member function that isn't meant to escape this and add return to every non-template member function that returns all or part of `this` by pointer or ref if you want the compiler to check that nothing gets escaped in @safe code."

On Wednesday, 22 August 2018 at 02:18:15 UTC, Mike Franklin wrote:
> On Wednesday, 22 August 2018 at 01:07:28 UTC, Mike Franklin wrote:
>
>> But what bothers me the most...
>
> Something else that rubs me the wrong way is that DIP 1000 is currently in a status of `DRAFT`:  https://github.com/dlang/DIPs/blob/master/DIPs/README.md
>
> What the heck is going on here?  We're adding features to the compiler and modifying Phobos in production releases based on a `DRAFT` proposal?

No, it's behind a flag, so you can't really say that we're shipping it as "production ready release".
In fact I think we should have a hell of a lot more of such experimental flags.
This would allow us to be able to merge things quickly, and gain real-world feedback and testing on complicated matters instead of PRs stalling to death in the queue.

For reference, Rust has currently 148 opt-in experimental language features:

https://doc.rust-lang.org/unstable-book/index.html

On Wednesday, 22 August 2018 at 11:02:00 UTC, Seb wrote:

> No, it's behind a flag, so you can't really say that we're shipping it as "production ready release".

The changes to Phobos are not behind a flag.  We're making changes to Phobos in the release branch to accommodate a draft/experimental/choose-your-adjective feature.

Mike

On 8/22/18 5:23 AM, Walter Bright wrote:
> On 8/21/2018 6:07 PM, Mike Franklin wrote:
>> The proposed idea wants to make the first parameter, if it's `ref`, special.
> 
> This is because Phobos is written with functions of the form:
> 
>      void put(sink, parameters...)
> 
> which corresponds to:
> 
>      sink.put(parameters...)
> 
> The two forms are fairly interchangeable, made more so by the Uniform Function Call Syntax.


> 
>  > Why not the first `ref` parameter regardless of whether it's the absolute first in the list.  Why not the last `ref` parameter?  Why not all `ref` parameters?
> 
> Good question. If this fairly restricted form solves the problems, then there is no need for the more flexible form. Things can always be made more flexible in the future, but tightening things can be pretty disruptive. Hence, unless there is an obvious and fairly strong case case for the flexibility, then it should be avoided for now.

What about:

size_t put(sink, parameters...)

Does this qualify as the sink being the "return" type? Obviously the real return can't contain any references, so it trivially can be ruled out as the destination of any escaping parameters.

Or how about a member function that takes a ref parameter? Is `this` the "return" or is the ref parameter the "return"?

My problem with the idea is that it is going to seem flaky -- we are using convention to dictate what is actually the return parameter, vs. what semantically happens inside the function. It's going to confuse anyone trying to do it a different way. I've experienced this in the past with things like toHash, where if you didn't define it with the exact signature, it wouldn't actually be used.

I realize obviously, that `put` is already specified. But as I said in the bug report, we should think twice about defining rules based solely on how Phobos does things, and calling that the solution.

As for things being made "more flexible in the future" this basically translates to code breakage. For example, if you are depending on only the first parameter being considered the "return" value, and all of a sudden it changes to encompass all your parameters, your existing code may fail to compile, even if it's correctly safe and properly annotated.

> 
> I want to ensure Atila is successful with this. But that means Phobos has to compile with dip1000. So I need to make it work.
> 

I think it's a very worthy goal to make Phobos work, and a great proof of concept for dip1000's veracity.

However, one-off rules just to make it work with existing code go against that goal IMO. Rules that stand on their own I think will fare better than ones that are loopholes to allow existing code to compile.

-Steve

On 8/22/2018 3:52 AM, Atila Neves wrote:
> On Wednesday, 22 August 2018 at 09:05:13 UTC, Walter Bright wrote:
>> On 8/21/2018 8:58 PM, Nicholas Wilson wrote:
>>> On Tuesday, 21 August 2018 at 14:31:02 UTC, Atila Neves wrote:
>>>> The problem is that the code we write doesn't deal directly with pointers - see the recent confusion in this forum over where `scope` on the left applies to the `this` pointer or the one returned by the member function.
>>>>
>>>> Kagamin just told me I needed to use `return` instead of `scope` to get things to work and I'm still not sure why.
>>>
>>> The way I think about it is if you have a function that takes a pointer, any pointer, and either returns it or a pointer derived from it (dereferencing or indexing) that argument must be marked `return`. In your case it was a pointer derived from `this` so `return` must be applied to `this`.
>>
>>
>> Another way to think about it is this:
>>
>>    S s;
>>    return &s;
>>
>> We all know that is an error. The idea is to have a way to express that for:
>>
>>     S s;
>>     return s.foo();
>>
>> and:
>>
>>     S s;
>>     return foo(&s);
>>
>> so that the compiler knows that the return value of foo() is attached to the lifetime of s. Pretty much everything flows from that.
> 
> Would the guideline below be correct?
> 
> "Add scope to every non-template member function that isn't meant to escape this and add return to every non-template member function that returns all or part of `this` by pointer or ref if you want the compiler to check that nothing gets escaped in @safe code."

Being a template doesn't make any difference, except that it will helpfully infer these things.

Also, since 'this' is passed by 'ref' to struct member functions, it cannot escape anyway with dip1000:

  struct S {
    int x;
    @safe ref int foo() { return x; }
  }

  dmd test -dip1000
  test.d(4): Error: returning this.x escapes a reference to parameter this, perhaps annotate with return

`scope` is for pointers, `ref` does not need further annotation.

On 8/22/2018 6:50 AM, Steven Schveighoffer wrote:
> What about:
> 
> size_t put(sink, parameters...)
> 
> Does this qualify as the sink being the "return" type? Obviously the real return can't contain any references, so it trivially can be ruled out as the destination of any escaping parameters.

Your reasoning is correct, but currently it only applies with 'void' return types.


> Or how about a member function that takes a ref parameter? Is `this` the "return" or is the ref parameter the "return"?

`this` is the ref parameter. In particular, consider a constructor:

  struct S {
     int* p;
     this(return scope int* p) { this.p = p; }
  }

  int i;
  S s = S(&i);

This code appears in Phobos, and it is very reasonable to expect it to check as safe.


> My problem with the idea is that it is going to seem flaky -- we are using convention to dictate what is actually the return parameter, vs. what semantically happens inside the function. It's going to confuse anyone trying to do it a different way. I've experienced this in the past with things like toHash, where if you didn't define it with the exact signature, it wouldn't actually be used.
> 
> I realize obviously, that `put` is already specified. But as I said in the bug report, we should think twice about defining rules based solely on how Phobos does things, and calling that the solution.

Phobos doesn't do this by accident. It's how constructors work (see above) and how pipeline programming works.


> As for things being made "more flexible in the future" this basically translates to code breakage. For example, if you are depending on only the first parameter being considered the "return" value, and all of a sudden it changes to encompass all your parameters, your existing code may fail to compile, even if it's correctly safe and properly annotated.

It's a good point. But I don't see an obvious use case for considering all the ref parameters as being returns.

> > I want to ensure Atila is successful with this. But that means Phobos has to
>> compile with dip1000. So I need to make it work.
>>
> 
> I think it's a very worthy goal to make Phobos work, and a great proof of concept for dip1000's veracity.
> 
> However, one-off rules just to make it work with existing code go against that goal IMO. Rules that stand on their own I think will fare better than ones that are loopholes to allow existing code to compile.

I couldn't come up with a better idea than this, and this one works.

On Thursday, 23 August 2018 at 08:48:15 UTC, Walter Bright wrote:
> Being a template doesn't make any difference, except that it will helpfully infer these things.

Including the `return` attribute.

> Also, since 'this' is passed by 'ref' to struct member functions, it cannot escape anyway with dip1000:
>
>   struct S {
>     int x;
>     @safe ref int foo() { return x; }
>   }
>
>   dmd test -dip1000
>   test.d(4): Error: returning this.x escapes a reference to parameter this, perhaps annotate with return

If we add the `return` attribute, we can escape a pointer to `this`. And since attributes are often inferred, it might come as a surprise to the programmer.

However, as far as I understand DIP 1000, it should make sure that the returned pointer won't outlive the struct instance. And I guess that's why it's ok to infer the `return` attribute?

That depends on DMD not over-estimating the lifetimes of things, though. Unfortunately, it does just that. When in doubt, DMD goes with infinite (e.g., `malloc`). I think that's a problem.

But I've already demonstrated my ignorance regarding DIP 1000 in a long, not very fruitful discussion with Atila on Bugzilla [1], so maybe I just don't get it.


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

On Thursday, 23 August 2018 at 08:48:15 UTC, Walter Bright wrote:
> On 8/22/2018 3:52 AM, Atila Neves wrote:
>> On Wednesday, 22 August 2018 at 09:05:13 UTC, Walter Bright wrote:
>>> On 8/21/2018 8:58 PM, Nicholas Wilson wrote:
>>>> On Tuesday, 21 August 2018 at 14:31:02 UTC, Atila Neves wrote:
>>>>> The problem is that the code we write doesn't deal directly with pointers - see the recent confusion in this forum over where `scope` on the left applies to the `this` pointer or the one returned by the member function.
>>>>>
>>>>> Kagamin just told me I needed to use `return` instead of `scope` to get things to work and I'm still not sure why.
>>>>
>>>> The way I think about it is if you have a function that takes a pointer, any pointer, and either returns it or a pointer derived from it (dereferencing or indexing) that argument must be marked `return`. In your case it was a pointer derived from `this` so `return` must be applied to `this`.
>>>
>>>
>>> Another way to think about it is this:
>>>
>>>    S s;
>>>    return &s;
>>>
>>> We all know that is an error. The idea is to have a way to express that for:
>>>
>>>     S s;
>>>     return s.foo();
>>>
>>> and:
>>>
>>>     S s;
>>>     return foo(&s);
>>>
>>> so that the compiler knows that the return value of foo() is attached to the lifetime of s. Pretty much everything flows from that.
>> 
>> Would the guideline below be correct?
>> 
>> "Add scope to every non-template member function that isn't meant to escape this and add return to every non-template member function that returns all or part of `this` by pointer or ref if you want the compiler to check that nothing gets escaped in @safe code."
>
> Being a template doesn't make any difference, except that it will helpfully infer these things.

The reason I wrote "non-template" is precisely because attributes get inferred for templates and therefore it would be at best redundant to annotate.

What about the guideline being correct or not?

>
> Also, since 'this' is passed by 'ref' to struct member functions, it cannot escape anyway with dip1000:
>
>   struct S {
>     int x;
>     @safe ref int foo() { return x; }
>   }
>
>   dmd test -dip1000
>   test.d(4): Error: returning this.x escapes a reference to parameter this, perhaps annotate with return

Returning by ref is good, since it's a lot harder to escape it. Since variables can't be declared ref, I can't pass it to a ref global. However:

----------
struct S {
    int x;
    @safe int* foo() { return &x; }
}
----------

% dmd -o- -dip1000 foo.d
% echo $?
0

Oops:

----------
int* gPtr;
void main() {
    auto s = S(42);
    gPtr = s.foo;
}
----------

"Don't use pointers then!". Ok:

----------
int[] gSlice;
void main() {
    auto s = S([42]);
    gSlice = s.foo;
}

struct S {
    int[] x;
    @safe int[] foo() return { return x; }
}
----------

% dmd -o- -dip1000 bar.d
% echo $?
0

Oops. I can't say `int[] x;` since it doesn't apply to fields.

> `scope` is for pointers, `ref` does not need further annotation.

I always forget this, it's confusing. It doesn't help that slices have pointers, so I guess `scope` is for them too?

And in a struct, `this` is a `ref`, yet `scope` on a member function applies to `this`.

Atila

On 8/23/18 4:58 AM, Walter Bright wrote:
> On 8/22/2018 6:50 AM, Steven Schveighoffer wrote:
>> What about:
>>
>> size_t put(sink, parameters...)
>>
>> Does this qualify as the sink being the "return" type? Obviously the real return can't contain any references, so it trivially can be ruled out as the destination of any escaping parameters.
> 
> Your reasoning is correct, but currently it only applies with 'void' return types.
> 
> 
>> Or how about a member function that takes a ref parameter? Is `this` the "return" or is the ref parameter the "return"?
> 
> `this` is the ref parameter. In particular, consider a constructor:
> 
>    struct S {
>       int* p;
>       this(return scope int* p) { this.p = p; }
>    }
> 
>    int i;
>    S s = S(&i);
> 
> This code appears in Phobos, and it is very reasonable to expect it to check as safe.

What I mean to say is, we have a semantic today -- the return value is hooked to any `return` parameters, end of story. This is clear, concise, and easy to understand.

You are saying that in some cases, the return value is actually deposited in the `this` parameter. In cases where the actual return type is void, OK, I see that we can tack on that rule without issues.

Furthermore any member function (or UFCS function for that matter) REQUIRES the first parameter to be the aggregate. How do you make a member function that stuffs the return into a different parameter properly typecheck? What rule do we tack on then? It's going to be confusing to anyone who writes their API thinking about how it's call syntax reads, not how the compiler wants to do flow analysis.

Not to mention, the keyword is `return`, not `returnorfirstparam`. It's still going to be confusing no matter how you look at it.

>> My problem with the idea is that it is going to seem flaky -- we are using convention to dictate what is actually the return parameter, vs. what semantically happens inside the function. It's going to confuse anyone trying to do it a different way. I've experienced this in the past with things like toHash, where if you didn't define it with the exact signature, it wouldn't actually be used.
>>
>> I realize obviously, that `put` is already specified. But as I said in the bug report, we should think twice about defining rules based solely on how Phobos does things, and calling that the solution.
> 
> Phobos doesn't do this by accident. It's how constructors work (see above) and how pipeline programming works.

Constructors I agree are reasonable to consider `this` to be the return value. On that point, I would say we should definitely go ahead with making that rule, and I think it will lead to no confusion whatsoever.

pipeline programming depends on returning something other than `void`, so I don't see how this applies.

It's more when you are setting members via properties where this comes into play. We need it -- we need this ability to tell the compiler "this parameter connects to this other parameter". I just don't know if the proposed rules are a) good enough for the general case, and b) don't cause more confusion than they are worth.

>> As for things being made "more flexible in the future" this basically translates to code breakage. For example, if you are depending on only the first parameter being considered the "return" value, and all of a sudden it changes to encompass all your parameters, your existing code may fail to compile, even if it's correctly safe and properly annotated.
> 
> It's a good point. But I don't see an obvious use case for considering all the ref parameters as being returns.

You would have to consider the shortest liftetime and assume everything goes there. It would restrict your legitimate calls. Only mitigating factor may be if you take the ones you aren't going to modify as const or inout.

>> > I want to ensure Atila is successful with this. But that means 
>> Phobos has to
>>> compile with dip1000. So I need to make it work.
>>>
>>
>> I think it's a very worthy goal to make Phobos work, and a great proof of concept for dip1000's veracity.
>>
>> However, one-off rules just to make it work with existing code go against that goal IMO. Rules that stand on their own I think will fare better than ones that are loopholes to allow existing code to compile.
> 
> I couldn't come up with a better idea than this, and this one works.
> 

I want to stress that it may be a valid solution, but we should strive to prove the solutions are the best possible rather than just use duct-tape methodology.

It should even be considered that perhaps there are better solutions even than the approach dip1000 has taken.

I also want to point out that the attitude of 'we could just fix it, but nobody will pull my request' is unhelpful. We want to make sure we have the best solution possible, don't take criticism as meaningless petty bickering. People are genuinely trying to make sure D is improved. Hostility towards reviews or debate doesn't foster that.

-Steve