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

Improve implied lambda and delegate argument names #15277

Merged
merged 9 commits into from
Jun 5, 2023
Merged

Conversation

kerams
Copy link
Contributor

@kerams kerams commented May 30, 2023

Alleviates the problem in #11898 (without direct delegates) and improves fsharp/fslang-suggestions#1083 (comment).

> System.IO.File.OpenText;;
val it: path: string -> System.IO.StreamReader

> let handler (x: string) (y: int) =
-     System.Console.WriteLine(x, y)
-
- let invokeHandler (foo: System.Action<string, int>) =
-     for parameter in foo.Method.GetParameters() do
-         printfn "%s " parameter.Name
-
- invokeHandler (System.Action<_,_>(handler));;
x
y
val handler: x: string -> y: int -> unit
val invokeHandler: foo: System.Action<string,int> -> unit
val it: unit = ()

> let z = Ok;;
val z: ResultValue: 'a -> Result<'a,'b>

instead of

> System.IO.File.OpenText;;
val it: arg00: string -> System.IO.StreamReader

> let handler (x: string) (y: int) =
-     System.Console.WriteLine(x, y)
-
- let invokeHandler (foo: System.Action<string, int>) =
-     for parameter in foo.Method.GetParameters() do
-         printfn "%s " parameter.Name
-
- invokeHandler (System.Action<_,_>(handler));;
delegateArg0
delegateArg1
val handler: x: string -> y: int -> unit
val invokeHandler: foo: System.Action<string,int> -> unit
val it: unit = ()

> let z = Ok;;
val z: arg0: 'a -> Result<'a,'b>

@kerams kerams marked this pull request as ready for review May 31, 2023 13:37
@kerams kerams requested a review from a team as a code owner May 31, 2023 13:37
@kerams
Copy link
Contributor Author

kerams commented May 31, 2023

Is Test Unoptimized Declarations Project1 using different lang versions between WindowsCompressedMetadata desktop_release and WindowsCompressedMetadata coreclr_release or am I going crazy?

@vzarytovskii
Copy link
Member

Is Test Unoptimized Declarations Project1 using different lang versions between WindowsCompressedMetadata desktop_release and WindowsCompressedMetadata coreclr_release or am I going crazy?

Uh, it should run differently built fcs, I can look into it tomorrow.

@kerams
Copy link
Contributor Author

kerams commented May 31, 2023

Got it, desktop test options do not specify preview for the checking of scripts, so I forced it for the 2 tests in question.

@T-Gro
Copy link
Member

T-Gro commented Jun 5, 2023

Got it, desktop test options do not specify preview for the checking of scripts, so I forced it for the 2 tests in question.

Yep, the current passing of langversion:preview looks good to me.

Copy link
Member

@psfinaki psfinaki left a comment

Choose a reason for hiding this comment

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

@kerams awesome job, thanks a lot for the testing and examples!

If you want to continue working on this (refactoring, perf) - go ahead, otherwise feel free to resolve the comments and merge as-is.

@psfinaki psfinaki enabled auto-merge (squash) June 5, 2023 11:42
@psfinaki psfinaki merged commit fd25719 into dotnet:main Jun 5, 2023
@kerams kerams deleted the arg branch June 5, 2023 11:44
psfinaki pushed a commit that referenced this pull request Jun 6, 2023
* add review comment to sb files (#15288)

* add review comment to sb files

* add CODEOWNERS entry for source-build

* Don't show inline hint for arguments with same names as the parameters in DU (#15305)

* Signature of nested type with generic type parameter (#15259)

* Proof of concept

* Add generic parameter names to ModuleOrType.

* Revert ModuleOrType change

* Process ticks in demangledPath of TType_app.

* Only apply new logic when includeStaticParametersInTypeNames is active.

* Use FactForNETCOREAPP

* Fix build

---------

Co-authored-by: Tomas Grosup <[email protected]>

* Improve implied lambda and delegate argument names (#15277)

* Improve implied lambda and delegate argument names

* Fix

* Add tests

* Revert non-preview tests

* Sigh

* Re-revert

* Fix test

* Add testx

---------

Co-authored-by: Oleksandr Didyk <[email protected]>
Co-authored-by: Sudqi <[email protected]>
Co-authored-by: Florian Verdonck <[email protected]>
Co-authored-by: Tomas Grosup <[email protected]>
Co-authored-by: kerams <[email protected]>
T-Gro added a commit that referenced this pull request Jun 6, 2023
* add review comment to sb files (#15288)

* add review comment to sb files

* add CODEOWNERS entry for source-build

* Don't show inline hint for arguments with same names as the parameters in DU (#15305)

* Signature of nested type with generic type parameter (#15259)

* Proof of concept

* Add generic parameter names to ModuleOrType.

* Revert ModuleOrType change

* Process ticks in demangledPath of TType_app.

* Only apply new logic when includeStaticParametersInTypeNames is active.

* Use FactForNETCOREAPP

* Fix build

---------

Co-authored-by: Tomas Grosup <[email protected]>

* Improve implied lambda and delegate argument names (#15277)

* Improve implied lambda and delegate argument names

* Fix

* Add tests

* Revert non-preview tests

* Sigh

* Re-revert

* Fix test

* Add testx

---------

Co-authored-by: Oleksandr Didyk <[email protected]>
Co-authored-by: Sudqi <[email protected]>
Co-authored-by: Florian Verdonck <[email protected]>
Co-authored-by: Tomas Grosup <[email protected]>
Co-authored-by: kerams <[email protected]>
@0-gravity
Copy link

Will it be released any time soon?

@vzarytovskii
Copy link
Member

vzarytovskii commented Jul 6, 2023

Will it be released any time soon?

Probability should be in the next vs/dotnet release

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants