Fix 16001 - forbid lambda syntax followed by FunctionLiteralBody

[adamdruppe]

This trips up so many people and the alternatives are easy - more often than not, you just want to delete the =>.

Time to go ahead and get proactive.

[Geod24]

Use deprecationSupplemental for the suggestion.
Also, changelog.
Also, test.

But I thought this was warned against already, maybe in semantic ?

[adamdruppe]

There's no warning now, this has tripped up dozens of people (or more).

[ghost]

this has nothing to do in a parser. too much expectactions on the semantics.

[wilzbach]

@adamdruppe either way it still requires at least a test and changelog.

[Geod24]

Also, commit title is terrible for history. "Deprecate ambiguous chained delegate syntax (=> {})" with a proper commit message explaining that this syntax is widely used with another meaning in other language would be much better.

Form aside, I like the idea 👍

[ghost]

sorry but this has nothing to do with parsing. In no way this PR should be accepted. It this parses, then this should be rejected during decl sema let's say.

D is context free.

[dkorpel]

D is context free.

This rule can be expressed in a context-free grammar. However, since a function literal is a PrimaryExpression, it does mean that the entire rule for AssignExpression needs to get a deep-copy with the only exception that the {} function literal is removed, so it might be better to do this check in semantic indeed.

[adamdruppe]

I'm not sure I'd ever actually remove the syntax, I just want the compiler to warn against the common mistake.

[12345swordy]

Is there no other way of doing this without modifying the parser?

[adamdruppe]

My objection is not with the semantics. It is with this specific syntax. Syntax is handled by the parser.

[UplinkCoder]

what's one more wart on a warthog

[thewilsonator]

./druntime/import/core/internal/convert.d(786): Deprecation:` => { ... }`  is a lambda that returns a delegate. Use `(args) { ... }` for a multi-statement function literal or use ` => () { }` if you intended to return a delegate.
std/xml.d(635): Error: template instance `core.internal.hash.hashOf!string` error instantiating

[adamdruppe]

yeah there's a druntime and phobos thing that needs changing too. what a hassle.

i wish the error messages weren't so time consuming to fix. i'm tempted to just fork the compiler to have my private copy for my personal productivity

[WalterBright]

This trips up

What is 'This' referring to? Please, folks, let's stop with the commit messages that assumes everyone is intimately familiar with what you're doing and are able to manually parse the code commits to try and reverse engineer what it is about. The opening message of a PR must say:

1. What the PR does
2. Why

[WalterBright]

Of course, if it has a bugzilla entry with that information, a reference would suffice.

[dkorpel]

Of course, if it has a bugzilla entry with that information, a reference would suffice.

The issue is: https://issues.dlang.org/show_bug.cgi?id=16001
@adamdruppe please add 'fix issue 16001' to the commit message

[adamdruppe]

good

im gonna do an amended commit at some point anyway so ill rewrite the message then

[dlang-bot]

Thanks for your pull request and interest in making D better, @adamdruppe! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

• My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
• My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
• I have provided a detailed rationale explaining my changes
• New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

---

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✓	16001	enhancement	Lambda syntax: forbid use with FunctionLiteralBody: (x) => {assert(x);}

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#12528"

[adamdruppe]

i hate the first line of the commit message, that's even worse

but meh whatever

[dkorpel]

i hate the first line of the commit message, that's even worse

You can add a description after it:
"fix issue 16001 - forbid lambda syntax followed by FunctionLiteralBody"

PR description added

[adamdruppe]

What was that magic command to update the tests again? I know someone commented but I can't even find the comment now.

This test process sucks btw, improving error messages shouldn't cause test failures.

[MoonlightSentinel]

What was that magic command to update the tests again? I know someone commented but I can't even find the comment now.

test/run.d AUTO_UPDATE=1 ARGS=
(the ARGS= is just there to save some time)

This test process sucks btw, improving error messages shouldn't cause test failures.

Well, the tests also ensure that the message don't get worse ... the test runner cannot decide if a different error message is an improvement or not

[schveiguy]

sorry but this has nothing to do with parsing. In no way this PR should be accepted. It this parses, then this should be rejected during decl sema let's say.

This should be modeled after if(...); rejection. Here is that code, it's in the parser:

dmd/src/dmd/parse.d

Lines 5961 to 5971 in 9b5db6b

	case TOK.semicolon:
	if (!(flags & ParseStatementFlags.semiOk))
	{
	if (flags & ParseStatementFlags.semi)
	deprecation("use `{ }` for an empty statement, not `;`");
	else
	error("use `{ }` for an empty statement, not `;`");
	}
	nextToken();
	s = new AST.ExpStatement(loc, cast(AST.Expression)null);
	break;

[RazvanN7]

ping @adamdruppe

[adamdruppe]

The test process is broken. I did the output update and it still fails and I have no idea why. I can't even read these logs.

[MoonlightSentinel]

==============================
Test 'fail_compilation/fail16001.d' failed. The logged output:
/tmp/cirrus-ci-build/generated/freebsd/release/64/dmd -conf= -m64 -Ifail_compilation -verrors=0  -fPIC  -odtest_results/fail_compilation -oftest_results/fail_compilation/fail16001_0.o  -c fail_compilation/fail16001.d 
fail_compilation/fail16001.d(9): Deprecation: `(args) => { ... }` is a lambda that returns a delegate, not a multi-line lambda.
fail_compilation/fail16001.d(9):        Use `(args) { ... }` for a multi-statement function literal or use `(args) => () { }` if you intended for the lambda to return a delegate.
==============================
Test 'fail_compilation/fail16001.d' failed: Expected rc == 1, but exited with rc == 0

The test case currently passes because it's not compiled with -de, add

REQUIRED_ARGS: -de

---

The other tests hit a bad case where adding the new error messages as TEST_OUTPUT changed the line numbers (which is why some tests tend to add #line ...). You'll need to update the TEST_OUTPUT again. (That being said, d_do_test should automate that)

runnable/funclit.d:

-runnable/funclit.d(415): Deprecation: `(args) => { ... }` is a lambda that returns a delegate, not a multi-line lambda.
-runnable/funclit.d(415):        Use `(args) { ... }` for a multi-statement function literal or use `(args) => () { }` if you intended for the lambda to return a delegate.
+runnable/funclit.d(417): Deprecation: `(args) => { ... }` is a lambda that returns a delegate, not a multi-line lambda.
+runnable/funclit.d(417):        Use `(args) { ... }` for a multi-statement function literal or use `(args) => () { }` if you intended for the lambda to return a delegate.
 int delegate() pure nothrow @nogc @safe delegate() pure nothrow @nogc @safe delegate() pure nothrow @safe
 int delegate() pure nothrow @nogc @safe delegate() pure nothrow @nogc @safe delegate() pure nothrow @safe
 int

-fail_compilation/ice10212.d(13): Deprecation: `(args) => { ... }` is a lambda that returns a delegate, not a multi-line lambda.
-fail_compilation/ice10212.d(13):        Use `(args) { ... }` for a multi-statement function literal or use `(args) => () { }` if you intended for the lambda to return a delegate.
-fail_compilation/ice10212.d(13): Error: Expected return type of `int`, not `int function() pure nothrow @nogc @safe`:
-fail_compilation/ice10212.d(13):        Return type of `int` inferred here.
+fail_compilation/ice10212.d(15): Deprecation: `(args) => { ... }` is a lambda that returns a delegate, not a multi-line lambda.
+fail_compilation/ice10212.d(15):        Use `(args) { ... }` for a multi-statement function literal or use `(args) => () { }` if you intended for the lambda to return a delegate.
+fail_compilation/ice10212.d(15): Error: Expected return type of `int`, not `int function() pure nothrow @nogc @safe`:
+fail_compilation/ice10212.d(15):        Return type of `int` inferred here.

[adamdruppe]

finally

thx

[Geod24]

Common guys, where is the changelog ? We're deprecating a valid language construct because it is ambiguous. This requires an explanation to the end user, both in term of changelog and the actual message.

[adamdruppe]

On Wed, May 26, 2021 at 06:56:35PM -0700, Mathias LANG wrote: Common guys, where is the changelog ?

generated from the bugzilla entry. anyone who knows what this is knows it is problematic and anyone who doesn't know what this is will get the most benefit out of the deprecation message because they're 99% likely to be silently using it wrong.

[Geod24]

anyone who knows what this is knows it is problematic and anyone who
doesn't know what this is will get the most benefit out of the
deprecation message

No. Simply no. It's a valid language construct, you can't just assume that no one out there isn't using it accurately the way it's defined to work. In fact, the two tests you amended, which were extracted from Bugzilla entries, used it correctly.

It's quite likely that a large portion, if not most of the D users out there write code in a different fashion than you, me, or anyone contributing to DMD. And as a large amount of D code is private, we can't just go through all usages to know exactly how affected by this change people will be. We should always be aware of this when we do changes that impact users.
My most recent example is this PR where, while the re-ordering was "correct", it created high costs for one of the main corporate user of the language that no one had foreseen. Had I known before, I wouldn't have re-ordered them, as the cost/gain ratio was clearly not in favor of the re-order.

Deprecating valid language constructs like this one (or features), is about making a decision which trade-off we favor. Is the cost of the change we're pushing to our users worth the perceived gain from the same change ? Here we believe the answer to be a resounding yes, but the problem with deprecations is that the people pushing for it are the ones that have a lot to gain, while the people that are likely to pay the cost will only be notified about it later. We can't really eliminate this imbalance, but there are trivial steps we can take to reduce it, such as a proper explanation about the deprecation, and how to address it. A key part of the former is a comprehensive changelog entry.

I submitted #12592 to add said entry and improve the message. I'm not 100% satisfied with the wording so feel free to make suggestions.

[adamdruppe]

On Wed, May 26, 2021 at 08:47:38PM -0700, Mathias LANG wrote: It's quite likely that a large portion, if not most of the D users out there write code in a different fashion than you, me, or anyone contributing to DMD.

Yeah, I know. I personally have answered about 1/6 of all the user question threads over the last decade. Thousands of questions from hundreds of different people of varying skill level. This comes up a lot.