Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create FS-1129-shorthand-anonymous-unary-functions #710

Merged
merged 4 commits into from
Nov 6, 2023

Conversation

tboby
Copy link
Contributor

@tboby tboby commented Sep 21, 2022

This is just a starting point for fsharp/fslang-suggestions#506 with a basic proof of concept implementation found at dotnet/fsharp#13907


```fsharp
_.Foo.Bar => fun o -> o.Foo.Bar
_.Foo.[5] => fun o -> o.Foo.[5]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update for new indexer syntax

_.Foo.Bar => fun o -> o.Foo.Bar
_.Foo.[5] => fun o -> o.Foo.[5]
_.Foo() => fun o -> o.Foo()
_.Foo(5).X => fun o -> o.Foo(5).X
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I look at this again, I am concerned that we're baking in method-application-without-space and not allowing curried-application-without-space here.

I do wonder if we should just go simple and only allow _.Foo for now.

@kerams
Copy link
Contributor

kerams commented Jun 30, 2023

Is intra-assembly closure reuse feasible?

let r1: Record = ...
let r2: Record = ...
let r1x = r1 |> Option.map _.x
let r2x = r2 |> Option.map _.x

There should ideally be only one generated closure per property on type in an assembly, and it could be given a descriptive name.

@vzarytovskii
Copy link
Contributor

Is intra-assembly closure reuse feasible?

let r1: Record = ...
let r2: Record = ...
let r1x = r1 |> Option.map _.x
let r2x = r2 |> Option.map _.x

There should ideally be only one generated closure per property on type in an assembly, and it could be given a descriptive name.

Behaviour should be no different to how full lambda works now, since we just rewrite _.Prop to regular lambda in untyped ast.

@T-Gro
Copy link
Contributor

T-Gro commented Jun 30, 2023

But even the full lambda as of now produces duplicates for identical operations.
https://sharplab.io/#v2:DYLgZgzgPg9gDgUwHYAIDKBPCAXBBbAWAChjjgFsUAnARgA9qaUBeRlKAPhQHk5sBLGEgB0eAIZwUACjABXVAwC0XOsIAqMNNir8kAcykBKQ2QrUATAyrmWF9l14ChoidLkKUylKo1ad+o0MgA==

I believe the comment from @kerams is valid && orthogonal => might be worth a separate suggestions for the optimizer to de-duplicate identical lambda-generated classes.

@vzarytovskii
Copy link
Contributor

But even the full lambda as of now produces duplicates for identical operations.
https://sharplab.io/#v2:DYLgZgzgPg9gDgUwHYAIDKBPCAXBBbAWAChjjgFsUAnARgA9qaUBeRlKAPhQHk5sBLGEgB0eAIZwUACjABXVAwC0XOsIAqMNNir8kAcykBKQ2QrUATAyrmWF9l14ChoidLkKUylKo1ad+o0MgA==

I believe the comment from @kerams is valid && orthogonal => might be worth a separate suggestions for the optimizer to de-duplicate identical lambda-generated classes.

I honestly think we shouldn't be doing such analysis, since it can be quite complex with little benefits.

@smoothdeveloper
Copy link
Contributor

smoothdeveloper commented Nov 2, 2023

The feature is going to be shipping but it is very difficult to get access to the RFC as it wasn't merged and the suggestion description doesn't mention it. Also no discussion thread was opened for the RFC itself.

I'm mostly concerned seeing the feature implementation is not restricted to properties only (which was Don main restraint for initial take on the feature) and will have a few rough edges on release in terms of tooling (which the F# users should be ok with, as things will be sorted out); but overall, we should make sure the suggestion discussion is closed in favor to a discussion related to the RFC, and that the RFC (even if not the last one) is visible in the repository itself, with the link to the discussion thread.

I'd prefer an almost empty RFC in the repository, with link to the discussion thread, and the suggestion being closed (with top post updated with links, as well as the closing comment), than the current situation.

(edit: and to be clear, I think users are going to love this feature, it is great feature that will have big impact on the codebases)

@vzarytovskii vzarytovskii marked this pull request as ready for review November 6, 2023 13:50
@vzarytovskii vzarytovskii merged commit 6307394 into fsharp:main Nov 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants