Implement shortened methods with => syntax by adamdruppe · Pull Request #11833 · dlang/dmd

adamdruppe · 2020-10-08T18:53:10Z

For function literals, D offers a short syntax: x => y,
which expands to function (x) { return y; }. However, for
normal function definitions, there was no such shortening.
For various applications, there can be more syntax than
meaning, such as property accessors.

This commit changes that by expanding the same => syntax
we already have to work in declarations as well. It expands
to the same thing: int foo() => x; is simply
int foo() { return x; }; this is just shortened syntax.
Combined with existing rules like auto returns, a property
getter can be be as simple as @property x() => x_; or a
range-based pipeline may appear like
auto common_operation() => this[].sort.uniq; giving D's
existing functional strengths more syntax sugar.

C# has demonstrated the utility of such shortened methods
since its version 7. However, while C# allows it for
constructors as well, this commit will parse it but fail
in semantic because you cannot return a value from a
constructor. I am not convinced it is worth special-casing
the simple "=> y ALWAYS means "{ return y; }" rule for
this case and thus left it alone.

dlang-bot · 2020-10-08T18:53:12Z

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
✓	7176	enhancement	Lambda => syntax for function and methods too

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

12345swordy · 2020-10-08T19:04:47Z

You need a preview switch for this.

thewilsonator · 2020-10-09T00:11:16Z

this commit will parse it but fail in semantic because you cannot return a value from a constructor

Is it possible to detect this and give a nice error message?

adamdruppe · 2020-10-09T00:13:37Z

No need; there already is one:

class A { this() => 7; }

cool.d(2): Error: cannot return expression from constructor

adamdruppe · 2020-10-09T02:49:30Z

src/dmd/parse.d

+            nextToken();
+            f.fbody = new AST.ReturnStatement(returnloc, parseExpression());
+            check(TOK.semicolon);
+            f.endloc = endloc;


I think I did this wrong. I just copied from below but p sure this is specifically for }.... should I instead set it to token.loc for the semicolon?

Seems reasonable to me, and I can't think of any more appropriate token to point it at. It seems to be used only for line mappings for debug info (and I supposed DMD as a Library for whatever).

You should probably update this comment too.

That comment is the one on the parser class, this is on the f object which is the FuncDeclaration class. (It was that comment that tipped me I was doing the wrong thing though.)

thewilsonator · 2020-10-09T04:14:34Z

Have you made a forum post about this yet? If so please link.

UplinkCoder · 2020-10-09T07:11:47Z

🤷

Geod24 · 2020-10-09T11:33:55Z

Been wanting this a few times, so obviously 👍

Few things:

Are there any context where the syntax could be ambiguous ? auto comes to mind (delegate member variable vs function).
Need spec PR, obviously;
I'd also open a forum thread, as discussed;
This probably needs either a -preview or a DIP;
Does this work with mixins ?

UplinkCoder · 2020-10-09T11:42:49Z

It's a visible language change.
Needs a DIP.

adamdruppe · 2020-10-09T12:55:57Z

On Thu, Oct 08, 2020 at 09:14:47PM -0700, Nicholas Wilson wrote: Have you made a forum post about this yet? If so please link.

No, this was just from a short chat on the discord. People were talking about the idea, I said that should be easy to implement, they said "lol prove it" and i was like "lol k". We can make one in a bit though.

adamdruppe · 2020-10-09T13:04:42Z

On Fri, Oct 09, 2020 at 04:34:15AM -0700, Mathias LANG wrote: - Are there any context where the syntax could be ambiguous ? `auto` comes to mind (delegate member variable vs function).

I very much doubt it; it should have failed the tests if it was. The key difference is the parens. auto a = () => 4; // parens on rhs of =, a is a variable assigned a delegate auto c() => 5; // parens on lhs of =, c is a function The parens after is what the parser looks for to determine it is a declaration which is then on a different branch than expression parsing. Since the latter is always illegal in today's D, it is a purely additive change and should break nothing. The auto tester seems to agree, aside from what appears to be spurious nonsense in the (Windows_LDC_Debug x64-debug-ldc) thing...

- Does this work with mixins ?

Can't imagine why it wouldn't. A quick test of mixin("auto c() => 5;"); seems to agree.

adamdruppe · 2020-10-09T14:35:28Z

anyone what's up with the azure-pipelines Azure pipelines (Windows_LDC_MinGW win32-ldc) test?

thewilsonator · 2020-10-09T14:37:06Z

I've seen it on a few other PRs.

adamdruppe · 2020-10-09T14:44:46Z

Forum post up: https://forum.dlang.org/post/cmgqwabzdqbtngmjidfw@forum.dlang.org

UplinkCoder · 2020-10-09T14:44:57Z

Druntime for mingw printf format specifiers are wrong

…

On Fri, Oct 9, 2020, 4:37 PM Nicholas Wilson ***@***.***> wrote: I've seen it on a few other PRs. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#11833 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAVWSCFDNRSCA562CKSPCLTSJ4N2VANCNFSM4SJEDSAA> .

rainers · 2020-10-09T17:08:49Z

Druntime for mingw printf format specifiers are wrong

The respective build is not done with -de so the compiler doesn't error out. unittest.exe is built successfully and is run, but crashes. IIRC DM make misinterprets the exit code and reports not found.

adamdruppe · 2020-10-10T15:11:51Z

Old bugzilla link came up on the forum:
https://issues.dlang.org/show_bug.cgi?id=7176

can perhaps use that for the changelog.

CyberShadow · 2020-10-18T07:05:48Z

You need a preview switch for this.

I thought we only need preview switches for breaking changes. Why would strict additions need a compiler switch?

wilzbach · 2020-10-18T08:21:21Z

You need a preview switch for this.

I thought we only need preview switches for breaking changes. Why would strict additions need a compiler switch?

With a -preview switch it can be merged right now and formally approved via a DIP later.

12345swordy · 2020-10-19T18:42:24Z

I concur. This needs a preview switch.

Imperatorn · 2020-10-20T05:14:21Z

Just be sure to add documentation and examples so it's clear to users how to use it. Other than that, 10 x 👍

andre2007 · 2020-12-10T09:50:37Z

@adamdruppe any chance you can continue with this pr (add the missing parts)? This pr is highly interesting.

adamdruppe · 2020-12-10T22:03:03Z

ummm i guess i can look at it later, maybe over the weekend. I don't see any real value in the preview switch but it is probably simple enough to do.

PetarKirov · 2020-12-20T07:37:57Z

Can't the changelog be generated from the commit message? I don't know how these things work.

The changelog for each project is generally generated based on two sources:

Fixed Bugzilla bugs & enhancements (which are auto-closed if the commit message follows the right format as mentioned by @dlang-bot above)
Manually written changelog entries in this folder: https://github.com/dlang/dmd/tree/master/changelog

That said, even if the changelog could be fully auto-generated, based on Bugzilla, we always use manually written changelog entries for more notable changes, such as this one.

Imperatorn · 2020-12-28T18:51:41Z

So, what's the status on this one now? Ready for merge? Anything left to be done?

12345swordy · 2020-12-28T22:20:11Z

You need see the red tags, as there they are very impotent.

thewilsonator · 2020-12-29T03:01:44Z

Test 'compilable/previewhelp.d' failed: 
...
diff:
----
   =rvaluerefparam   enable rvalue arguments to ref parameters
   =nosharedaccess   disable access to shared memory objects
   =in               `in` on parameters means `scope const [ref]` and accepts rvalues
+  =shortenedMethods allow use of => for methods and top-level functions in addition to lambas
   =inclusiveincontracts 'in' contracts of overridden methods must be a superset of parent contract
----

test/compilable/shortened_methods.d

For function literals, D offers a short syntax: `x => y`, which expands to `function (x) { return y; }`. However, for normal function definitions, there was no such shortening. For various applications, there can be more syntax than meaning, such as property accessors. This commit changes that by expanding the same `=>` syntax we already have to work in declarations as well. It expands to the same thing: `int foo() => x;` is simply `int foo() { return x; }`; this is just shortened syntax. Combined with existing rules like auto returns, a property getter can be be as simple as `@property x() => x_;` or a range-based pipeline may appear like `auto common_operation() => this[].sort.uniq;` giving D's existing functional strengths more syntax sugar. C# has demonstrated the utility of such shortened methods since its version 7. However, while C# allows it for constructors as well, this commit will parse it but fail in semantic because you cannot return a value from a constructor. I am not convinced it is worth special-casing the simple "=> y ALWAYS means "{ return y; }" rule for this case and thus left it alone.

adamdruppe · 2020-12-30T02:47:38Z

@thewilsonator thanks for the help things like this are never high on my todo list

Geod24 · 2020-12-30T03:41:36Z

🎉

WalterBright · 2020-12-30T08:47:25Z

@thewilsonator @Geod24 This is a language change. Why is there no changelog and no spec pull? Both were called out with labels. There's no indication of what the new BNF grammar is.

Geod24 · 2020-12-30T08:52:44Z

@WalterBright : I didn't pull, just got the notification, but those are good point. Even though there's an issue for it (so it appears in the changelog), this definitely warrants its oown changelog entry.
Regarding the spec PR, I usually ask for it (and I did) but most people tend to forget. At least this one is behind a -preview switch.

WalterBright · 2020-12-30T09:08:13Z

We have standards for how a PR should be done. I certainly do not advocate blindly follow those standards, but for deviations there ought to be a decent reason. I expect all those with Pull privileges to help ensure this. If a PR doesn't meet those standards, an offer to help the submitter would be helpful.

I'm especially concerned about this because of the many legitimate complaints that the spec is out of date. This problem should be getting better over time, not worse. I added the "Need Changelog" label which was removed without explanation. I know you requested the Spec change, but that was ignored.

@12345swordy wrote as well "You need see the red tags, as there they are very impotent." The typo is appropriate.

adamdruppe · 2020-12-30T16:09:06Z

I don't speak spec and my understanding* is the rebase means the changelog picked it up. But maybe I'll get around to figuring them out but it will be several months, if ever. So if you want those kind of things done, it is far, far, far more efficient for someone who already knows it to do that part. Specialization of labor isn't just for building horseless carriages.

I'm not terribly invested in this PR. I just wrote a patch to prove to someone in chat it wasn't hard to implement. It isn't something I'll likely use myself since I avoid new features anyway (my libs promise compatibility with 3+ year old compilers...)

BTW this readme
https://github.com/dlang/dmd/tree/master/changelog

should be linked from this page https://github.com/dlang/dmd/blob/master/CONTRIBUTING.md

Imperatorn · 2020-12-30T16:27:12Z

While I'd like this to be merged, which it seems it did, we also have to follow some standards. Sometimes there are multiple forces pulling in different directions. At those times, I actually think Walter should have the final word (if it's not completely insane). Let's respect our founder(s)

schveiguy · 2021-02-13T21:58:44Z

Just seeing that this was merged. For 2.096.0, this will be in the compiler, it needs a spec udpate and a better changelog entry.

12345swordy · 2021-02-19T20:59:23Z

This shouldn't be merge without a spec PR have been made and merged. If the next compiler release contain this feature without a spec update, it will create unnecessarily confusion for new users.

Imperatorn · 2021-02-22T07:15:20Z

Who can update the spec? One week until release 👀

Geod24 · 2021-02-22T07:19:25Z

Someone needs to make a PR. And this is why we ask for spec PRs before. Once the feature is merged, there's less incentive to do so.

Imperatorn · 2021-02-26T08:39:25Z

@adamdruppe I think this might fall on you to do. If you cannot do it yourself, provide an alternative solution to how it could/should be done and what people should/have to be involved.

schveiguy · 2021-02-28T19:57:40Z

#12241

adamdruppe force-pushed the shortened_methods branch 2 times, most recently from bde26e9 to deab5d5 Compare October 8, 2020 19:04

thewilsonator added the Review:Needs Spec PR A PR updating the language specification needs to be submitted to dlang.org label Oct 8, 2020

adamdruppe commented Oct 9, 2020

View reviewed changes

adamdruppe force-pushed the shortened_methods branch from deab5d5 to 6b15744 Compare October 9, 2020 13:26

WalterBright added the Review:Needs Changelog A changelog entry needs to be added to /changelog label Oct 10, 2020

rainers mentioned this pull request Oct 11, 2020

Debug azure #11850

Closed

adamdruppe force-pushed the shortened_methods branch from acd15ce to 6b15744 Compare October 11, 2020 19:38

MoonlightSentinel reviewed Dec 29, 2020

View reviewed changes

test/compilable/shortened_methods.d Outdated Show resolved Hide resolved

thewilsonator force-pushed the shortened_methods branch from 52fa7a8 to 6817c29 Compare December 30, 2020 02:13

dlang-bot added the Severity:Enhancement label Dec 30, 2020

thewilsonator removed the Review:Needs Changelog A changelog entry needs to be added to /changelog label Dec 30, 2020

thewilsonator force-pushed the shortened_methods branch from 6817c29 to 2e8a671 Compare December 30, 2020 02:23

thewilsonator force-pushed the shortened_methods branch from 2e8a671 to e6850f8 Compare December 30, 2020 02:41

thewilsonator approved these changes Dec 30, 2020

View reviewed changes

thewilsonator added the Merge:auto-merge label Dec 30, 2020

dlang-bot merged commit ed994a4 into dlang:master Dec 30, 2020

adamdruppe mentioned this pull request Jan 4, 2021

Implementation of Properties. #12097

Closed

5 tasks

WalterBright mentioned this pull request Jan 5, 2021

extend PR #11846 to add symbol support for AliasAssign #12029

Merged

pbackus mentioned this pull request Feb 26, 2021

Add -preview=shortenedMethods to language spec dlang/dlang.org#2956

Merged

Uh oh!

Conversation

adamdruppe commented Oct 8, 2020

Uh oh!

dlang-bot commented Oct 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bugzilla references

Testing this PR locally

Uh oh!

12345swordy commented Oct 8, 2020

Uh oh!

thewilsonator commented Oct 9, 2020

Uh oh!

adamdruppe commented Oct 9, 2020

Uh oh!

adamdruppe Oct 9, 2020

Choose a reason for hiding this comment

Uh oh!

thewilsonator Oct 9, 2020

Choose a reason for hiding this comment

Uh oh!

adamdruppe Oct 9, 2020

Choose a reason for hiding this comment

Uh oh!

thewilsonator commented Oct 9, 2020

Uh oh!

UplinkCoder commented Oct 9, 2020

Uh oh!

Geod24 commented Oct 9, 2020

Uh oh!

UplinkCoder commented Oct 9, 2020

Uh oh!

adamdruppe commented Oct 9, 2020 via email

Uh oh!

adamdruppe commented Oct 9, 2020 via email

Uh oh!

adamdruppe commented Oct 9, 2020

Uh oh!

thewilsonator commented Oct 9, 2020

Uh oh!

adamdruppe commented Oct 9, 2020

Uh oh!

UplinkCoder commented Oct 9, 2020 via email

Uh oh!

rainers commented Oct 9, 2020

Uh oh!

adamdruppe commented Oct 10, 2020

Uh oh!

CyberShadow commented Oct 18, 2020

Uh oh!

wilzbach commented Oct 18, 2020

Uh oh!

12345swordy commented Oct 19, 2020

Uh oh!

Imperatorn commented Oct 20, 2020

Uh oh!

andre2007 commented Dec 10, 2020

Uh oh!

adamdruppe commented Dec 10, 2020

Uh oh!

PetarKirov commented Dec 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Imperatorn commented Dec 28, 2020

Uh oh!

12345swordy commented Dec 28, 2020

Uh oh!

thewilsonator commented Dec 29, 2020

Uh oh!

Uh oh!

adamdruppe commented Dec 30, 2020

Uh oh!

Geod24 commented Dec 30, 2020

Uh oh!

WalterBright commented Dec 30, 2020

Uh oh!

Geod24 commented Dec 30, 2020

Uh oh!

WalterBright commented Dec 30, 2020

Uh oh!

dlang-bot commented Oct 8, 2020 •

edited

Loading

PetarKirov commented Dec 20, 2020 •

edited

Loading