Fixup for #12528 - Add changelog, improve message, fix tests

[Geod24]

Most deprecations should be explained to the users.
Deprecations that trigger on valid user code absolutely must be explained to the user,
so this adds a changelog entry, instead of relying on the one-liner that comes with
the bugzilla entry.

Additionally, the message wasn't enlighting: Since it triggers on possibly valid code
(the user might have intended to write a delegate returning a delegate, or might have not),
the message should state why the syntax is disallowed, not merely what the deprecated syntax is doing.

Lastly, out of the three tests affected by PR12528, two of them used the syntax correctly:
the test was actually checking that there was a correct match with a function type.
Hence, instead of mindlessly adding the deprecation message to the test,
kicking the can down the road to the person that will actually turn this into an error,
the deprecation should have fixed those error messages in the first place.

CC @ibuclaw : Not too happy with the wording of the changelog, I take suggestions.

[dlang-bot]

Thanks for your pull request, @Geod24!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + dmd#12592"

[RazvanN7]

I would start the paragraph by explaining that the mentioned syntax does not have the expected effect. For example:

"[...] would often use (args) => { /* body */ } for delegate / function literal.

However, in D, this syntax results in a delegate that returns a delegate without any other side effects. This may trigger hard-to-debug bugs, therefore it is now deprecated.

Since the D delegate syntax uses either a simple arrow (examples), or braces (examples), if a delegate returning a delegate is indeed the intended usage, use (args) { return () => /* body */; } or (args) => () { /* body */ } instead."

How does that sound?

[Geod24]

I actually cut the last sentence, hope it's more readable now.

[RazvanN7]

Thank you for this!

[adamdruppe]

This is just wrong and it doesn't tell the user anything.

[adamdruppe]

The message I wrote is similar to a bug report: explain what you saw as well as what you expected to see. The idea is the user wrote that expecting to see a mutli line delegate. So the message specifically says that is not what happened and briefly describes what actually happened instead.

Then the next line explains to them how to fix it for either intent.

This is most likely going to be seen by new users and meeting them where they are will hopefully cut down on the support requests (though I've also seen many times when people copy paste perfectly clear messages, they just never bothered actually reading it.....)

[Geod24]

The idea is the user wrote that expecting to see a mutli line delegate

And that's where the message is lacking. It should not only cater to those making a mistake, but also to those actually using it correctly. As a matter of fact, since we're introducing a deprecation, it's quite likely that it will be seen by seasoned users first, before it is seen by newcomers...

The deprecation message should tell the users why what (s)he is doing is not possible, not state what the user (may have) intended to do. To draw a parallel with error messages, consider this:

void foo (string a);
void main ()
{
    foo(42);
}

Pick the better error message:

foo.d(4): Error: Function `void foo(string a)` is being called with arguments `(42)`
- OR -
foo.d(4): Error: cannot pass argument `42` of type `int` to parameter `string a`

I chose "ambiguous" for lack of a better word. Implicit string concatenation uses "error-prone", perhaps we should use that (disclaimer: I picked "error-prone") ?

[adamdruppe]

On Thu, May 27, 2021 at 05:55:49AM -0700, Mathias LANG wrote: And that's where the message is lacking. It should not only cater to those making a mistake, but also to those actually using it correctly.

That's why it says "use `(args) => () {}` if you intended for the lambda to return a delegate". It explains both cases and offers a trivial, two char change in either case.

As a matter of fact, since we're introducing a deprecation, it's quite likely that it will be seen by seasoned users first, before it is seen by newcomers...

There's exactly one case of this syntax between druntime and Phobos, in a unittest. There's one case of it in a comment in Ocean. It is used incorrectly, but since it is in a comment it doesn't matter. There's several in vibe-auth. It is used incorrectly in all cases. (leading to silently passing unittests because the test result is never actually run). For example: https://github.com/gedaiu/vibe.auth/blob/af7edaddbe1f2b8270d35c9dd83a5a827a1f14cd/tests/authenticators/OAuth2.d#L88 workermanager on dub has a use that uses it correctly. libfuture on dub has several cases that use it correctly in unittests. (but I actually thought it was wrong at first glance, it works because of an explicit overload in the Future class that calls the returned delegate. I actually would guess they added that specifically for this syntax and the unittest is to confirm their hack is correct.) spasm came up in grep, but those are all inline Javascript strings, so it is actually correct but also wouldn't come up by a compiler test ae has one use, done correctly. fluent-asserts has several, all incorrect. fileslurp has one use and it is incorrect. So the net result of this grep over my (incomplete) copy of the dub repo is 4 people will have to add () in a few places, and two will find their unit tests are silently broken and have been for years and will need to delete =>. That's a net gain. Then the dozens of new people I've had to correct over this will hopefully no longer need manual correcting too.

I chose "ambiguous" for lack of a better word. Implicit string concatenation uses "error-prone", perhaps we should use that (disclaimer: I picked "error-prone") ?

that'd be ok.

[Geod24]

Updated with all the feedback.

[RazvanN7]

LGTM