This is the feedback thread for the Final Review of DIP 1033, "Implicit Conversion of Expressions to Delegates".

===================================
**THIS IS NOT A DISCUSSION THREAD**

Posts in this thread must adhere to the feedback thread rules outlined in the Reviewer Guidelines (and listed at the bottom of this post).

https://github.com/dlang/DIPs/blob/master/docs/guidelines-reviewers.md

That document also provides guidelines on contributing feedback to a DIP review. Please read it before posting here. If you would like to discuss this DIP, please do so in the discussion thread:

https://forum.dlang.org/post/ysdmdeemnrfwqemkjrlr@forum.dlang.org

==================================

You can find DIP 1033 here:

https://github.com/dlang/DIPs/blob/8e56fc593ece5c74f18b8eb68c3f9dcedf2396a7/DIPs/DIP1033.md

The review period will end at 11:59 PM ET on December 4, or when I make a post declaring it complete. Feedback posted to this thread after that point may be ignored.

At the end of the Final Review, the DIP will be forwarded to the language maintainers for Formal Assessment.

==================================
Posts in this thread that do not adhere to the following rules will be deleted at the DIP manager's discretion:

* All posts must be a direct reply to the DIP manager's initial post, with only two exceptions:

    - Any commenter may reply to their own posts to retract feedback contained in the original post

    - The DIP author may (and is encouraged to) reply to any feedback solely to acknowledge the feedback with agreement or disagreement (preferably with supporting reasons in the latter case)

* Feedback must be actionable, i.e., there must be some action the DIP author can choose to take in response to the feedback, such as changing details, adding new information, or even retracting the proposal.

* Feedback related to the merits of the proposal rather than to the contents of the DIP (e.g., "I'm against this DIP.") is allowed in Community Review (not Final Review), but must be backed by supporting arguments (e.g., "I'm against this DIP because..."). The supporting arguments must be reasonable. Obviously frivolous arguments waste everyone's time.

* Feedback should be clear and concise, preferably listed as bullet points (those who take the time to do an in-depth review and provide feedback in the form of answers to the questions in the reviewer guidelines will receive much gratitude). Information which is irrelevant to the DIP or is not provided in service of clarifying the feedback is unwelcome.

There's a few problems and some things I don't really agree with. I'll start with the most obvious problem regarding the example with absolutePath.

> pure @safe string absolutePath(string path, string delegate() base = getcwd());

This becomes (per the DIP):

pure @safe string absolutePath(string path, string delegate() base = () @trusted { return getcwd() });

Which would still give an error, though the delegate encapsulating getcwd() is auto inferred, the actually parameter isn't auto inferred according to existing rules.

The parameter `base` is both impure and @unsafe. Unless there is some auto inference happening here this would fail to compile. It wouldn't be used the same way, the implementation would have to change to assume `base` is pure and @safe.

If there is auto inference of parameter types, the DIP needs to outline the details for this. Again, it would still be impure even if it auto inferred to be @trusted from `getcwd`. So again, the implementation would have to change to assume `base` is pure.

In either case, the code is broken and/or is giving the illusion that the problem at hand is simpler than it actually is by masking implementation details.



The DIP also doesn't go over one of the common discussions regarding lazy that is brought up from time to time; in regards to readability. This would be a good case to implement it. For example in C# at the call site the keyword `ref` has to be added when passing an argument to a ref parameter. Similarly you would just use a lambda `() =>` in C#, there is no special case to implicitly convert to a delegate.


> int delegate() dg = () { return 3; };
> int delegate() dg = () => 3;
> int delegate() dg = { return 3; };
> int delegate() dg = 3;

When you have 4 ways to express the exact same thing, there is an obvious design problem. You shouldn't need 4 different ways to express the same thing.

The existing Lambda syntax already has a lot of confusion, it isn't uncommon for individuals to mistakenly do the following:

    auto dg = x => { return x * x; };

Which doesn't do what it appears to do.

This would introduce syntax that can be similarly misunderstood. AS follows, the similar syntax can be used today. This code may not be common but it illustrates the commonality in the syntax, as calling a function doesn't require the `()` parenthesis.

    https://run.dlang.io/is/Nu6gct

    alias FooDelegate = int delegate();
    struct A {
        FooDelegate a() { return null; }
    }

    void main()
    {
        A a;
        int delegate() t = a.a;
    }


In summation, this is just replacing something that already was and is repeating past mistake*s*. Ultimately this is just syntax sugar to avoid 4 characters `() =>`. The justification is flimsy (along with errors in the DIP) and the benefit isn't outweighed by the hit to readability and the potential for that improvement by requiring to just define a delegate.

I really hope this DIP is at least delayed, as once it is added it won't be removed. If the implicit conversion does end up being needed for some reason, a better justification could be formulated with that reason and then be additively introduced at a later date.

On 20.11.20 08:30, Mike Parker wrote:
> https://github.com/dlang/DIPs/blob/8e56fc593ece5c74f18b8eb68c3f9dcedf2396a7/DIPs/DIP1033.md 

In the rationale:

> string delegate() base = getcwd()

Later, there's this on function pointers:

> Implicit conversion of expressions to function lambdas is not done. There doesn't seem much point to it, as there will be no arguments to the function lambda, meaning the expression can only consist of globals.

But as the rationale shows, the expression can also consist of a function call like `getcwd()`. The DIP fails to show how

    string delegate() base = getcwd()

is desirable, but

    string function() base = getcwd()

isn't.

TL;DR: Fix attributes for functions taking delegates as parameters first. Otherwise, this DIP is useless with respect to the direction D is intended to go: using more attributes (default, inferred or explicit ones).

The DIP says: "Although this DIP renders lazy redundant and unnecessary, it does not propose actually removing lazy. That will be deferred for a future DIP." That indicates that replacing `lazy T` by T delegate() is fine in any case. It's not! When attributes are present, behavior cannot even easily be replicated with delegates.

Those two functions should be practically identical, right?

    T foo(T)(lazy T value) /*pure*/ { return value(); }
    T goo(T)(T delegate() value) /*pure?*/ { return value(); }

If T isn't something really funny that might be impure when moved, foo will be inferred `pure` always. Noticed the question mark I put behind goo's pure? Well, goo will be inferred `pure` never because the compiler sees its body calling the impure delegate.
So from the point of attributes, `lazy` and delegates are vastly different. Attribute inference is near useless for templates that take delegate parameters. Attributes have a hard time being applied to regular functions taking delegate parameters.
The programmer is stuck between a rock and a hard place:
A) Put the desired attribute(s) on the delegate
B) Overload on pure and impure delegates.
C) Ignore attributes (which is what most would do, accidentally).

Issues:
A) Makes the function not available for closures that aren't complying to the attribute.
B) For if the function can be inferred pure, @safe, nothrow and/or @nogc, make up to 16 overloads, all with the same body. No one wants that, not even with mixins.
C) But then: why have attributes in the first place

I saw myself giving up D because I thought attributes are one of the best things D has to offer, but failing it so badly is sad. Completely idiomatic @safe code must drop @safe because **one** library function just works best using a delegate (or function pointer).

The DIP introduces a questionable implicit conversion of T to T delegate() and questionable distinctions between function pointers and delegates on usage, but misses the point of attributes completely. Solving the attribute issue could make up the drawbacks of the implicit conversion.

That's for the problem.

---

But I have an idea for a solution.

Assuming a delegate parameter is called in the body of the function it's the parameter of, which is---in my opinion---a very reasonable assumption, it should be the caller who's deciding what behavior of a function is fine and which isn't. `lazy` works like that.

    S pureFactory() pure;
    S impureFactory();

    void goo(S delegate() value) pure { return value(); }

should compile. Even without the body visible. Here's the reason:

    void pureContext() pure
    {
        goo(&pureFactory); // compiles   (1)
        goo(&impureFactory); // error    (2)
    }

(1) Plugging in a pure delegate in an otherwise pure function will result in a pure operation, so the context (= the attributes of the caller) is satisfied.
(2) Plugging in an impure delegate in an otherwise pure function will result in a possibly impure operation, so the context is not satisfied.

    void impureContext()
    {
        goo(&pureFactory); // compiles   (3)
        goo(&impureFactory); // compiles (4)
    }

(3) See (1). Pure operations are allowed in an impure context.
(4) Plugging in an impure delegate in an otherwise pure function will result in a possibly impure operation, but the context does not care.

Type checking goo isn't complicated. The compiler just needs to pretend that the delegate parameter type is `S delegate() pure` (i.e. the original `S delegate()` with goo's `pure` attribute). If that is fine, goo is pure relative to its parameter.

The caller can decide on the callee's signature, the parameter's (delegate) type, and its own attributes whether a call will be valid. This approach does not need complicated analysis by the compiler. The question "Am I plugging in a pure delegate argument to a (relatively) pure (i.e. pure annotated) function?" is answered by signatures and types alone.

It's similar to weakly pure functions. Allowing weakly pure functions made more functions strictly pure. The same way, relatively pure functions make more functions non-relatively pure.

I have not thought in depth about nested delegate parameter types, i.e. functions like

    void f(int delegate(int delegate()) higherOrderParam) pure { ... }

but solving it for the first layer will solve most applications of delegates. It seems easy, but no one so far has affirmed me the following reasoning is correct in general:

    int g() pure;
    int h();

In the body of f,

    higherOrderParam(&g);   // compiles
    higherOrderParam(&h);   // error

since higherOrderParam might call its parameter, but f knows what it's plugging in. So I assume that delegate types need never be annotated pure/whatever since the context will always be able to discern if the thing it's concerned about will be pure/whatever or not.

The only drawbacks of my solution that I know of are
* pure/whatever functions called with impure delegates that aren't called become assumed impure (breaking change in theory)
* pure/whatever functions aren't guaranteed to act pure/whatever unless all argumetns to delegate parameters are pure/whatever. This is a non-issue for all functions without delegate parameters and the correct thing for almost all functions with delegate parameters.
* Template functions could be surprisingly inferred pure/whatever when template parameters are delegate or function pointer types. Surprising here means to the programmer; almost guaranteed to be a pleasant surprise.

The breaking of the change can be seen to be rather weak since plugging in an impure delegate in a pure function will still be in a greater context where after all, that delegate will be called. Delegates are to be called after all.

With that change, attributes on delegate parameter mean: This function cannot work properly if that delegate isn't pure/whatever.

On 11/20/2020 4:40 PM, MD-39 wrote:
> There's a few problems and some things I don't really agree with. I'll start with the most obvious problem regarding the example with absolutePath.
> 
>> pure @safe string absolutePath(string path, string delegate() base = getcwd());
> 
> This becomes (per the DIP):
> 
> pure @safe string absolutePath(string path, string delegate() base = () @trusted { return getcwd() });
> 
> Which would still give an error, though the delegate encapsulating getcwd() is auto inferred, the actually parameter isn't auto inferred according to existing rules.
> 
> The parameter `base` is both impure and @unsafe. Unless there is some auto inference happening here this would fail to compile. It wouldn't be used the same way, the implementation would have to change to assume `base` is pure and @safe.
> 
> If there is auto inference of parameter types, the DIP needs to outline the details for this. Again, it would still be impure even if it auto inferred to be @trusted from `getcwd`. So again, the implementation would have to change to assume `base` is pure.
> 
> In either case, the code is broken and/or is giving the illusion that the problem at hand is simpler than it actually is by masking implementation details.

It won't compile, and that seems correct to me.


> The DIP also doesn't go over one of the common discussions regarding lazy that is brought up from time to time; in regards to readability. This would be a good case to implement it. For example in C# at the call site the keyword `ref` has to be added when passing an argument to a ref parameter. Similarly you would just use a lambda `() =>` in C#, there is no special case to implicitly convert to a delegate.

The C# style makes refactoring code harder. Instead of modifying the callee, you have to additionally modify all of the callers.



>> int delegate() dg = () { return 3; };
>> int delegate() dg = () => 3;
>> int delegate() dg = { return 3; };
>> int delegate() dg = 3;
> 
> When you have 4 ways to express the exact same thing, there is an obvious design problem. You shouldn't need 4 different ways to express the same thing.

This comes from the desire to dispense with unnecessary syntax.


> The existing Lambda syntax already has a lot of confusion, it isn't uncommon for individuals to mistakenly do the following:
> 
>      auto dg = x => { return x * x; };
> 
> Which doesn't do what it appears to do.
> 
> This would introduce syntax that can be similarly misunderstood. AS follows, the similar syntax can be used today. This code may not be common but it illustrates the commonality in the syntax, as calling a function doesn't require the `()` parenthesis.
> 
>      https://run.dlang.io/is/Nu6gct
> 
>      alias FooDelegate = int delegate();
>      struct A {
>          FooDelegate a() { return null; }
>      }
> 
>      void main()
>      {
>          A a;
>          int delegate() t = a.a;
>      }
> 
> 
> In summation, this is just replacing something that already was and is repeating past mistake*s*. Ultimately this is just syntax sugar to avoid 4 characters `() =>`. The justification is flimsy (along with errors in the DIP)

Please be explicit about errors in the DIP, as just saying there are errors in the DIP is not helpful.

> and the benefit isn't outweighed by the hit to readability and the potential for that improvement by requiring to just define a delegate.

It doesn't introduce new syntax, and the point of lazy is to enable lazy evaluation of arguments without having to use delegate syntax. All this does is replace the 'lazy' with the delegate syntax. D already uses this for variadic lazy arguments, and nothing has come up bad about it.

On Fri, Nov 20, 2020 at 5:35 PM Mike Parker via Digitalmars-d < digitalmars-d@puremagic.com> wrote:

> [...]
>

I'd just like to make sure a reference to my concern about hidden implicit allocations that I made in the comments thread is present here.

I would strongly appeal to amend the DIP to only apply to `scope`
delegates, such that they may (should? must?) stack allocate. Non-scope
delegates require typical `() =>` syntax, which is important because it is
possible to SEE the allocation.
Potential for invisible closure allocations at every argument to every
function call in D is terrifying to me.

Otherwise, I like this DIP.

On Sunday, 22 November 2020 at 08:46:16 UTC, Walter Bright wrote:
> On 11/20/2020 4:40 PM, MD-39 wrote:
>> There's a few problems and some things I don't really agree with. I'll start with the most obvious problem regarding the example with absolutePath.
>> 
>>> pure @safe string absolutePath(string path, string delegate() base = getcwd());
>> 
>> This becomes (per the DIP):
>> 
>> pure @safe string absolutePath(string path, string delegate() base = () @trusted { return getcwd() });
>> 
>> Which would still give an error, though the delegate encapsulating getcwd() is auto inferred, the actually parameter isn't auto inferred according to existing rules.
>> 
>> The parameter `base` is both impure and @unsafe. Unless there is some auto inference happening here this would fail to compile. It wouldn't be used the same way, the implementation would have to change to assume `base` is pure and @safe.
>> 
>> If there is auto inference of parameter types, the DIP needs to outline the details for this. Again, it would still be impure even if it auto inferred to be @trusted from `getcwd`. So again, the implementation would have to change to assume `base` is pure.
>> 
>> In either case, the code is broken and/or is giving the illusion that the problem at hand is simpler than it actually is by masking implementation details.
>
> It won't compile, and that seems correct to me.

Yah I can clarify that misunderstanding for you.

This is actually the error referred to below. The DIP gives this signature as the equivalent to "pure @safe string absolutePath(string path, lazy string base = getcwd());". But as you stated the equivalent is understandably not going to compile. An actual equivalent to the signature absolutePath() should be given that is intended to actually compile and work.

>
>> The DIP also doesn't go over one of the common discussions regarding lazy that is brought up from time to time; in regards to readability. This would be a good case to implement it. For example in C# at the call site the keyword `ref` has to be added when passing an argument to a ref parameter. Similarly you would just use a lambda `() =>` in C#, there is no special case to implicitly convert to a delegate.
>
> The C# style makes refactoring code harder. Instead of modifying the callee, you have to additionally modify all of the callers.

I can clarify this as well.

Making it harder to refactor code also decreases the number of potential bugs. Consider the following code:

    int i = 0;
    foo( ++i );
    assert( i == 1 ); // change foo() to use delegate

If you were to change foo() to take a delegate you would now be changing the behavior everywhere foo is used. By requiring `() =>` you would force the user to add the checks and make it visibly more readable. This was a common complaint of `lazy`, that you can't tell whether code at the call site is being executed there or not. Now sure the example is practical, but unless the parameter is a constant it will change what the code does.

Also consider implicit conversion with D's structs. The argument could be made that it makes refactoring easier if implicit conversions were allowed. Implicit conversions are only allowed with assignments for this reason. Implicit conversions have negative unpredictable side effects; as is the case with feature.


>>> int delegate() dg = () { return 3; };
>>> int delegate() dg = () => 3;
>>> int delegate() dg = { return 3; };
>>> int delegate() dg = 3;
>> 
>> When you have 4 ways to express the exact same thing, there is an obvious design problem. You shouldn't need 4 different ways to express the same thing.
>
> This comes from the desire to dispense with unnecessary syntax.

That is an admirable goal, but what it ends up creating is complexity.

>> The existing Lambda syntax already has a lot of confusion, it isn't uncommon for individuals to mistakenly do the following:
>> 
>>      auto dg = x => { return x * x; };
>> 
>> Which doesn't do what it appears to do.
>> 
>> This would introduce syntax that can be similarly misunderstood. AS follows, the similar syntax can be used today. This code may not be common but it illustrates the commonality in the syntax, as calling a function doesn't require the `()` parenthesis.
>> 
>>      https://run.dlang.io/is/Nu6gct
>> 
>>      alias FooDelegate = int delegate();
>>      struct A {
>>          FooDelegate a() { return null; }
>>      }
>> 
>>      void main()
>>      {
>>          A a;
>>          int delegate() t = a.a;
>>      }
>> 
>> 
>> In summation, this is just replacing something that already was and is repeating past mistake*s*. Ultimately this is just syntax sugar to avoid 4 characters `() =>`. The justification is flimsy (along with errors in the DIP)
>
> Please be explicit about errors in the DIP, as just saying there are errors in the DIP is not helpful.

Of course, see the first paragraph for clarification.

>> and the benefit isn't outweighed by the hit to readability and the potential for that improvement by requiring to just define a delegate.
>
> It doesn't introduce new syntax, and the point of lazy is to enable lazy evaluation of arguments without having to use delegate syntax. All this does is replace the 'lazy' with the delegate syntax. D already uses this for variadic lazy arguments, and nothing has come up bad about it.

It does replace lazy but it isn't 100% equivalent. The argument can be made that lazy doesn't need to be replaced and that's the argument I'm making. A wait-and-see approach would be preferred here. As once this is implemented it can't be removed nor corrected.

I've never seen a variadic lazy being used, I tried searching phobos and found no use of it. I'm not even sure under what circumstance one would even want to use that. There are bugs in the variadic lazy implementation as well (see the discussion thread). Just because nothing has come up doesn't mean there isn't anything wrong with the implementation, it can just mean that no one is using it. From my experience it isn't used very often (at all?).

Hope that clarifies some of the points I was making.

On Tuesday, 24 November 2020 at 23:45:24 UTC, MD-39 wrote:

> Yah I can clarify that misunderstanding for you.
>

I'm usually very strict about posts violating the Feedback Thread rules, but I'm not going to delete this one. Please, unless the DIP author explicitly asks you to clarify something, take this sort of the post to the Discussion Thread (where you can note that you're replying to the DIP author's response in this thread and paste a link to it). This thread is specifically for raising issues with the DIP, not for discussing them. If you're not writing something that goes in the review summary, then it doesn't belong here.

And please don't reply to this post.

Thanks!

On Friday, 20 November 2020 at 07:30:05 UTC, Mike Parker wrote:

> The review period will end at 11:59 PM ET on December 4, or when I make a post declaring it complete.

This review period is over. Thanks to everyone who participated.

On Sunday, 6 December 2020 at 10:56:52 UTC, Mike Parker wrote:
> On Friday, 20 November 2020 at 07:30:05 UTC, Mike Parker wrote:
>
>> The review period will end at 11:59 PM ET on December 4, or when I make a post declaring it complete.
>
> This review period is over. Thanks to everyone who participated.

Note to all: the DIP author will likely respond to feedback that he did not yet respond to. If he does so, please do not reply to his posts in this thread. Although the review is over, please take any such replies to the discussion thread.

Thank you.