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

[WIP and Blocked] Inline hints for F# #10295

Closed
wants to merge 26 commits into from
Closed

Conversation

cartermp
Copy link
Contributor

@cartermp cartermp commented Oct 21, 2020

Example showing type and parameter hints (functions and methods):

image

Note the following behavior:

  • Type hints are at definition/binding, not use
  • Type hints do not show if something already has an annotation
  • Parameter name hints appear for F# functions and methods, using the named argument syntax
  • Named parameters get no hints
  • Lambdas that are let-bound will have a type hint, but other let-bound F# functions will not

General checklist:

  • Colors for .NET-defined types are not correct. This will require a different API to get the structured layout
  • The hover shown is not the correct thing to do. It is actually the right thing for a parameter hint, which isn't in this prototype right now. What we need to do instead is get QuickInfo data for the actual type itself, not the value declaration
  • There is no current way to distinguish between values with an inferred type and values with an explicit type. This will likely need to involve the syntax API and (1) find the correct syntax node, and (2) check if it is a SynExpr.Typed ... or something along those lines
  • There are some other places where showing a symbol might be bad, like [x; y; rest] in a list pattern. The y and rest are already implied by whatever x is. I guess we can make a call on if that matters or it's better to just show it for each thing. The latter is less work of course. Maybe if the value is a literal we shouldn't show it either?
  • Not showing type hints on ctor or method parameters
  • Not showing type hints on member annotations like properties
  • Don't show hints in signature files
  • Don't show hints for top-level annotated values
  • Function hints should only show return type
  • Function annotation hides type hints on parameters like let ranchItUp x : string = string x
  • Hints for lambdas
  • Argument names for parameters to method calls and constructor calls
  • Handle curried parameter groups for functions and curried F# methods
  • Setting to respect key binding
  • Setting for colors
  • Setting for type hints
  • Setting for parameter name hints

Out of scope

  • Type hints for "normal" function declarations. It just looks bad and there isn't a good way to play hints without getting smushed together with the type hint for the last parameter. Lambda function declarations, yes.
  • Show inferred types for _ in a pattern (Nope - this can be done orthogonally and also support quickinfo)
  • Add QuickInfo - Nope, this can be done orthogonally. We need a way to resolve the symbol we're showing the type for and get quickinfo from. No such API exists in the compiler today

@cartermp cartermp changed the title [Proof of concept] Inline hints for F# [Proof of concept] Inline type hints for F# Oct 21, 2020
@cartermp

This comment has been minimized.

@cartermp

This comment has been minimized.

@cartermp cartermp changed the title [Proof of concept] Inline type hints for F# [Proof of concept] Inline hints for F# Oct 29, 2020
@cartermp cartermp changed the title [Proof of concept] Inline hints for F# Inline hints for F# Oct 30, 2020
@cartermp cartermp requested a review from dsyme October 30, 2020 01:07
src/fsharp/NicePrint.fs Outdated Show resolved Hide resolved
src/fsharp/service/FSharpCheckerResults.fs Outdated Show resolved Hide resolved
src/fsharp/service/ServiceParamInfoLocations.fsi Outdated Show resolved Hide resolved
src/fsharp/service/ServiceParamInfoLocations.fsi Outdated Show resolved Hide resolved
src/fsharp/service/ServiceUntypedParse.fsi Outdated Show resolved Hide resolved
src/fsharp/symbols/Symbols.fs Outdated Show resolved Hide resolved
src/fsharp/symbols/Symbols.fsi Outdated Show resolved Hide resolved
@cartermp cartermp requested a review from dsyme November 2, 2020 20:01
@cartermp cartermp force-pushed the inlinehints branch 2 times, most recently from 6f5ecfc to 88664d6 Compare November 3, 2020 22:42
@kerams
Copy link
Contributor

kerams commented Nov 4, 2020

What about hints for multi-line piping? Could come in handy when doing a complex transformation of a collection with group bys, collects, etc., or piping values of combined monads like Async and Result.

@cartermp
Copy link
Contributor Author

cartermp commented Nov 4, 2020

Pipeline hints might be nice, but we'll see. I don't want to introduce too much scope creep into this one since it's a pretty big PR already.

Co-authored-by: Chet Husk <baronfel@users.noreply.github.com>
@KevinRansom
Copy link
Member

@dsyme, are you satisfied that @cartermp has addressed your requested changes?

Thanks

Kevin

@cartermp cartermp added this to the 16.9 milestone Nov 17, 2020
@cartermp
Copy link
Contributor Author

Holding off on this one. Wiring up settings is going to require more plumbing of APIs through our Roslyn external access layer.

@cartermp cartermp changed the title Inline hints for F# [WIP and Blocked] Inline hints for F# Nov 20, 2020
@cartermp cartermp mentioned this pull request Nov 25, 2020
@cartermp cartermp modified the milestones: 16.9, Backlog Dec 23, 2020
@kerams
Copy link
Contributor

kerams commented Jan 23, 2021

Is there no way to release this in VS preview as an experimental feature with the only setting being on/off for now?

@cartermp
Copy link
Contributor Author

There is, but there would be no toggle. It's annoying to code with unless you have a quick keyboard toggle.

@nosami
Copy link
Contributor

nosami commented Jan 25, 2021

Looking forward to seeing this in VSMac!

@baronfel
Copy link
Member

thoughts on slicing off the FCS parts of this so that FSAC can get at this?

@cartermp
Copy link
Contributor Author

cartermp commented Apr 1, 2022

I think it should be pretty simple to do. In fact, I think some of what's in here is already in FCS now (more or less), so that would reduce the diff for sure. I won't speak for @vzarytovskii or @KevinRansom but I think it's quite reasonable to split those changes out.

@baronfel
Copy link
Member

baronfel commented Apr 1, 2022

I've actually been experimenting with your editor-level work in FSAC/Ionide this evening, and I can say it works brilliantly! The big gap is methods and their parameters, because that's where you added all the supporting infrastructure.

@vzarytovskii
Copy link
Member

I think it should be pretty simple to do. In fact, I think some of what's in here is already in FCS now (more or less), so that would reduce the diff for sure. I won't speak for @vzarytovskii or @KevinRansom but I think it's quite reasonable to split those changes out.

Oh yeah, 100% agree, we should probably split it into smaller parts.

@baronfel
Copy link
Member

baronfel commented Apr 1, 2022

Link to ionide version is here for those that want to see what it looks like

@dsyme
Copy link
Contributor

dsyme commented Apr 5, 2022

@cartermp Do you think you could bring this up-to-date? It's a good feature

@baronfel
Copy link
Member

baronfel commented Apr 5, 2022

@dsyme after talking with @cartermp I've split and updated his PRs here for the FCS only changes, and here for the VS integration pieces, which should allow the FCS ones to move forward independently if ready.

@cartermp
Copy link
Contributor Author

cartermp commented Apr 5, 2022

Yep, defer to @baronfel's work here

@cartermp cartermp closed this Apr 5, 2022
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.

7 participants