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

InlayHints: LSP 3.17 & TextEdits #943

Merged
merged 29 commits into from
Jun 27, 2022
Merged

Conversation

Booksbaum
Copy link
Contributor

@Booksbaum Booksbaum commented May 13, 2022

Add & Implement InlayHints from LSP 3.17 (as mentioned in #935)

  • Note: LSP 3.17 was just released (on 2022-05-10) -- but in this PR I'm still treating it as still in preview: most code I wrote before 3.17 and Types aren't yet in Ionide.LanguageServerProtocol -> in here still temporary
  • Note: InlayHint capabilities: not handled at all

Both textDocument/inlayHint and inlayHint/resolve are implemented -- with exception of Tooltips: That's still always None.

  • inlayHint/resolve: resolves Tooltip & TextEdits when missing from InlayHint -- but in reality it's actually completely unused & unnecessary:
    • As mentioned above: Tooltips aren't currently generated at all
    • TextEdits are always created when collecting all InlayHints and not delayed to be resolved later.
      Reason: I changed ident detection & validation (-> "should get a hint?") slightly for type hints. In the process I collect all data necessary for TextEdits too -> no reason to resolve later again...


Use TextEdits instead of just "insert string at Inlay Hint Location" (-> necessary for parens)

  • Note: fsharp/inlayHints still exists (-> compatibility with older version) -- but its InsertText can only insert at InlayHint location (-> no parens) -> might be wrong


Use same logic for type InlayHints & AddExplicitTypeAnnotation CodeFix

Enhancements around type annotations, like

  • already type annotated?
  • needs parens when type annotation gets inserted?
  • where must parens go
  • And some other special cases like ?a -> optional parameter, but type annotation cannot have option (: int instead of : int option)

I'm using this (at least in part) to decide if type Hints should be emitted.
-> This replaces some other (ast) checks
Though I'm not yet sure it still behaves the same -> there are still some additional tests necessary (-> PR is still WIP)


On enhancement is to handle function values:

let map f v = f (v+1)
//      ^

type A(a) =
  member _.F(b, f) = a + b |> f
//              ^

let f = fun a b -> a + b
//  ^

-> AddExplicitType can now add type annotations to f.
But should we show type hints too? (currently disabled)
It's nice for short cases -- but when it accepts many args that hint might get quite large and distracting.
(Ideal solution would be to make it a setting for user -- but not now. And even then we should use reasonable defaults)

(Note: currently not for functions (let f a b = a + b). Maybe someday -- but not now. Getting a function's return type is unfortunately not trivial (at least wasn't last time I tried))





Still WIP because:

  • No tests yet for textDocument/inlayHint

  • Still some testing to do around change in InlayHint triggering

  • Some cleanup (I left some unused code and TODOs)

  • Considering LSP 3.17 was just released: Maybe wait till update of Ionide.LanguageServerProtocol?
    -> Then we don't have to remember to remove LSP.Preview.fs sometime later (but instead do that directly in this PR here)

/// export type LSPArray = LSPAny[];
/// ```
/// -> `'Data` must adhere to specs
Data: 'Data option }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Data is of type LSPAny:

export type LSPAny = LSPObject | LSPArray | string | integer | uinteger |
	decimal | boolean | null;
export type LSPObject = { [key: string]: LSPAny };
export type LSPArray = LSPAny[];

Not really representable in F#
-> I decided to use a Generic instead

But then of course correctness is in user hand and not type checked any more. I still think it's way easier to use than anything else.

Copy link
Contributor

Choose a reason for hiding this comment

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

LSPAny looks a lot like JToken. In other cases like this we've used JToken/JToken[] as appropriate, because the core need is a JSON-serializable data blob.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes -- but it's so horrible to use in F# (compared to a normal record)....

(though I think it can be used with additional (de-)serialization -> same as generic -- but with a step between)

Copy link
Contributor

Choose a reason for hiding this comment

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

oh, I see - you're less concerned with 100% representing the API contract, because you're also trying to make sure people use the API correctly: by specifying a generic type, you let the JSON-RPC handler deserialize it consistently to prevent data out-of-sync errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wouldn't call it "out-of-sync error" -- it requires an erroneous or malicious client to actively change Data (Data is just a short-lived note from Server to itself which the client just has to preserve).
But generic Data places the responsibility for correctness (and error handling) of Data on LSP side (-> Data should be preserved -- otherwise violation of LSP contract (vs. violation of FSAC contract; for example with valid InitializeParams (LSP), but invalid FSharpConfigDto (FSAC) -> FSAC responsibility to handle error))


But when making Data generic, I only considered a Server implementation. In a client implementation, the client shouldn't care about Data at all (besides preserving it). But with a generic the client MUST specify some type -- the opposite of "not caring"... JToken on the other hand is just that: some json I can just ignore.
-> I'll change InlayHint to use JToken instead

@Booksbaum Booksbaum force-pushed the InlayHints-LSP317 branch from 733c2cc to b8ff486 Compare May 13, 2022 17:51
Pos = data.InsertAt
Kind = Type
// TODO: or use tyForAnno?
Text = ": " + (truncated ty)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Type and Type necessary for annotation don't necessary need to match:

static member F(?value) = value |> Option.map ((+) 1)
//               ^^^^^ type: `int option`
// with type annotation:
static member F(?value: int) = value |> Option.map ((+) 1)
//                      ^^^ just `int` without `option`

What should we use as type hint? (when inserted it correctly uses just int)

Currently I'm showing int option -- but the sole reason is: That's what was used before.
I'm always confused when type in annotation is without option -- but it's confusing too to show type hint with option but when it gets inserted the option disappears


Additional:
I'm currently only handle trailing option.
Does type.Format always produce a trailing option, or is Option<...> (maybe even with namespace) possible?

Copy link
Contributor

Choose a reason for hiding this comment

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

it's possible for type.Format to produce Option<...> if the displayContext used in the call to Format has the use postfix generic synax flag set. You can see an example of this here. That's not the default, though, and everything you're doing here should be able to assume that you will always get the trailing option.

// normal ctor in type: `type A(v) = ...`
| SyntaxNode.SynMemberDefn (SynMemberDefn.ImplicitCtor _) -> true
//TODO: when? example?
| SyntaxNode.SynTypeDefn (SynTypeDefn(typeRepr = SynTypeDefnRepr.Simple(simpleRepr = SynTypeDefnSimpleRepr.General(implicitCtorSynPats = Some (ctorPats))))) when
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When does this happen?
Case above is normal primary ctor (type A(v) = ...) -- but when does this here occur?

Copy link
Contributor

Choose a reason for hiding this comment

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

Unknown to me 🤷

@baronfel
Copy link
Contributor

Regarding the LSP types themselves, I have commit rights and publish rights to the language server protocol repo. If it's not too much work, I'd really like it if we could get these types there first and then just update the dependency here. I can get those turned around really quickly so that this PR isn't blocked.

@Booksbaum
Copy link
Contributor Author

-> ionide/LanguageServerProtocol#22

But no need to rush. Remaining tests (in this PR) will take some time to implement

@baronfel
Copy link
Contributor

Alright, Ionide.LanguageServerProtocol 0.4.1 has been pushed to NuGet whenever you're ready. There are other datatype changes, so if you prefer I can do that update in another dedicated branch, merge that, then you can rebase on top.

@Booksbaum
Copy link
Contributor Author

There are other datatype changes, so if you prefer I can do that update in another dedicated branch, merge that, then you can rebase on top.

That would be great. Thanks!

@Booksbaum
Copy link
Contributor Author

Is there a way to get documentation for a FSharpType or FSharpSymbol?
FCS seems to provide doc/tooltip only for actual source locations?

But for InlayHints I want a tooltip for type of that ident, not for the ident itself.

Comment on lines 728 to 897
checkAllInMarkedRange server
"""
open System.Collections.Immutable
$|let arr$0 = ImmutableArray.CreateBuilder()$|
arr.Add 2
"""
[
//Currently: `ImmutableArray`1.Builder<int>`
typeHint "ImmutableArray<int>.Builder"
"""
open System.Collections.Immutable
let arr: ImmutableArray<int>.Builder = ImmutableArray.CreateBuilder()
arr.Add 2
"""
]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a way to format the type correctly?
ImmutableArray`1.Builder<int> isn't valid.

Considering the tooltip in VS shows that as well, and nested types aren't possible in F# to create, I don't think there's something that handles type formatting correctly?

Copy link
Contributor

@baronfel baronfel May 27, 2022

Choose a reason for hiding this comment

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

I think you're right and there's a gap here in the type formatting from FCS for these types. I submitted dotnet/fsharp#13202 to address this, but it's good enough for now. Maybe have a test that describes the ideal scenario and mark it pending with a comment/link to this issue?

@baronfel
Copy link
Contributor

baronfel commented May 27, 2022

Is there a way to get documentation for a FSharpType or FSharpSymbol?

try FSharpType.TypeDefinition.XmlDoc for FSharpType, and for FSharpSymbol you'll have to do explicit casts to each of the subclasses that we know of for FSharpSymbol. This would be a prime location for an extension member:

type FSharpSymbol with
  member x.XDoc =
    match x with
    | :? FSharpEntity as e -> e.XmlDoc
    | :? FSharpUnionCase as u -> u.XmlDoc
    | :? FSharpField as f -> f.XmlDoc
    | :? FSharpActivePatternCase as c -> c.XmlDoc
    | :? FSharpGenericParameter as g -> g.XmlDoc
    | :? FSharpMemberOrFunctionOrValue as m -> m.XmlDoc
    | :? FSharpStaticParameter
    | :? FSharpParameter -> FSharpXmlDoc.None
    | _ -> failwith $"cannot fetch xmldoc for unknown FSharpSymbol subtype {x.GetType().FullName}"

  member x.XSig =
    match x with
    | :? FSharpEntity as e -> e.XmlDocSig
    | :? FSharpUnionCase as u -> u.XmlDocSig
    | :? FSharpField as f -> f.XmlDocSig
    | :? FSharpActivePatternCase as c -> c.XmlDocSig
    | :? FSharpMemberOrFunctionOrValue as m -> m.XmlDocSig
    | :? FSharpGenericParameter
    | :? FSharpStaticParameter
    | :? FSharpParameter -> ""
    | _ -> failwith $"cannot fetch XmlDocSig for unknown FSharpSymbol subtype {x.GetType().FullName}"

/// That's fixed in `main` FCS
/// -> This here is a copy of [`ServiceParseTreeWalk.fs`@`3a610e0`](https://github.com/dotnet/fsharp/blob/3a610e06d07f47f405168be5ea05495d48fcec6d/src/fsharp/service/ServiceParseTreeWalk.fs) with slight adjustments so it compiles
///
/// **Remove once it's available as nuget package and updated here in FSAC**
Copy link
Contributor

Choose a reason for hiding this comment

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

Man, I hate that we have to do this :) I get it, though.

Copy link
Contributor

Choose a reason for hiding this comment

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

guick note for myself that this is still the case as of FCS 41.0.5

Comment on lines 728 to 897
checkAllInMarkedRange server
"""
open System.Collections.Immutable
$|let arr$0 = ImmutableArray.CreateBuilder()$|
arr.Add 2
"""
[
//Currently: `ImmutableArray`1.Builder<int>`
typeHint "ImmutableArray<int>.Builder"
"""
open System.Collections.Immutable
let arr: ImmutableArray<int>.Builder = ImmutableArray.CreateBuilder()
arr.Add 2
"""
]
Copy link
Contributor

@baronfel baronfel May 27, 2022

Choose a reason for hiding this comment

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

I think you're right and there's a gap here in the type formatting from FCS for these types. I submitted dotnet/fsharp#13202 to address this, but it's good enough for now. Maybe have a test that describes the ideal scenario and mark it pending with a comment/link to this issue?

// normal ctor in type: `type A(v) = ...`
| SyntaxNode.SynMemberDefn (SynMemberDefn.ImplicitCtor _) -> true
//TODO: when? example?
| SyntaxNode.SynTypeDefn (SynTypeDefn(typeRepr = SynTypeDefnRepr.Simple(simpleRepr = SynTypeDefnSimpleRepr.General(implicitCtorSynPats = Some (ctorPats))))) when
Copy link
Contributor

Choose a reason for hiding this comment

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

Unknown to me 🤷

@Booksbaum
Copy link
Contributor Author

try FSharpType.TypeDefinition.XmlDoc for FSharpType and for FSharpSymbol [...XmlDoc...]

XmlDocs seems to be filled for entities specified directly in user code -- but not for external stuff:

/// Some Description
type T = class end

let f (a: T) (b: int) = ()
//        ^      xxx

T has description in XmlDoc, but XmlDoc for int is empty.

But I found TipFormatter.formatDocumentationFromXmlSig which creates a doc from some assembly and xmlSig (-> FSharpXmlDoc.FromXmlFile) (Used in Ionide for Info Panel).
That works fine.

Though it's a bit strange:
Why doesn't FCS emit a FSharpXmlDoc.FromXmlFile for external entities -- instead it's an empty FSharpXmlDoc.FromXmlText...?


This would be a prime location for an extension member: [...]

Exists already -- though with fewer cases

@Booksbaum Booksbaum force-pushed the InlayHints-LSP317 branch from 72403d6 to f69c410 Compare May 28, 2022 17:48
@Booksbaum
Copy link
Contributor Author

About error on MacOS:
I ran into similar errors while testing locally. Running tests again never produced the error again. I figured it was some test caching issue (maybe I accidentally ran with an old version). But now it came up on a clean MacOS test run.

Error:

---------- Expected: ------------------
Some (Missing { Ident = test.fsx (1,4--1,9)
                InsertAt = (1,9)
                Parens = Optional test.fsx (1,4--1,9)
                SpecialRules = [] })
---------- Actual: --------------------
Some (Missing { Ident = test.fsx (1,4--1,9)
                InsertAt = (1,9)
                Parens = Optional test.fsx (1,4--1,9)
                SpecialRules = [] })

I have no idea why this fails.
Based on the output both results are exactly the same...

@baronfel
Copy link
Contributor

baronfel commented Jun 1, 2022

Why doesn't FCS emit a FSharpXmlDoc.FromXmlFile for external entities -- instead it's an empty FSharpXmlDoc.FromXmlText...?

Can you get a simple, single example? If so we could submit it upstream.

@Booksbaum
Copy link
Contributor Author

Can you get a simple, single example? If so we could submit it upstream.

example:

git clone https://gist.github.com/fb5e2a2d1711faa719d3af8785b93370.git XmlDoc
cd XmlDoc
dotnet run

Booksbaum added 12 commits June 14, 2022 13:37
Note: for LSP 3.17.0 -> still in proposed state
`Data` is of type `LSPAny`:
```typescript
export type LSPAny = LSPObject | LSPArray | string | integer | uinteger |
	decimal | boolean | null;
export type LSPObject = { [key: string]: LSPAny };
export type LSPArray = LSPAny[];
```
-> Not really able to express in F#

-> ~ `any` (TS/JS) or `obj` (F#) with just basic types in leaves and array(-serializable) as container

For practical reasons: Generic instead of `obj`.
But no checking for correctness according to specs
Necessary because of bugs and missing features:
* Doesn't go into `SynMatchClause`
  * Fixed in `dotnet/fsharp` main, but not in just released FCS `41.0.4` (I think)
* Doesn't walk into `SynPat.As` & `SynPat.Record`
  * `SynPat.As` gets visited in `dotnet/fsharp` main, not not in FCS `41.0.4`
  * `SynPat.Record`: dotnet/fsharp#13114

-> Remove `ServiceParseTreeWalk` once FCS gets updated (probably `42.0`? -> lots of changes of Syntax Elements)
Change InlayHints to use multiple inserts instead of just single one
-> for parens

Enhance logic to determine edits to add explicit type should include parens or not
Note: not yet every possible position tests -> might still trigger for unwanted location

Use same logic for `InlayHint`s & `AddExplicitTypeToParameter`

Note: trigger logic was slightly changed (-> less tests, but more complex check for explicit type which includes some trigger logic)
-> not sure there are locations hints are missings or hints are shown when they shouldn't -> needs more tests

TODO: change name of `AddExplicitTypeToParameter`
-> Isn't limited to parameter any more but all (non-function) bindings

Note: No tests yet for `textDocument/inlayHint` & `inlayHint/resolve`
Note: Tooltips for InlayHints aren't yet implemented
Note: several tests fail: Existing InlayHint tests aren't updated to handle TextEdits
Note: still unfinished and lots of TODOs and necessary tests

Note: `inlayHint/resolve` isn't really necessary: `TextEdit`s get currently created when creating InlayHint (-> because needs Explicit Type checks which produce everything necessary for `TextEdit`s)

Note: No InlayHints Capabilities handling or checking

Note: Because of rebase: Remove `LSP.Preview.fs`
Fix: no `InsertText` for `fsharp/inlayHints`
Note: Simple, hacky solution which throws away everything not directly inserted at Hint Location and everything that's just parens
-> doesn't always produce correct solution -> use `textDocument/inlayHint` instead

Fix: tests fail because of missing inserts
Examples: (`f`):
* `let f = fun a b -> a + b`
* `let map f v = f v`
* Note: note functions with parameters:
  `let f a b = a + b`

Able to add type annotation via AddExplicitType CodeFix,
but disable for InlayHint (for now)
Reason: `AddExplicitType` now handles more than just parameters
-> triggers for let binding
Issue: Incorrect type format: ``ImmutableArray`1.Builder<int>`` instead of ``ImmutableArray<int>.Builder``
//TODO: how to format correctly?
Remove Tooltip from `inlayHint/resolve`

Add tests for LSP InlayHints
* Including TextEdits
* Move Tests from F# InlayHints to LSP Inlay Hints
  * F# Inlay Hints contain just very few basic tests to check it's still working
    but all real tests of InlayHints are now for LSP InlayHints

Enable `explicitTypeInfoTests` (calling of tests wasn't checked in before...)
-> should be easier to adopt new version once available
@Booksbaum Booksbaum force-pushed the InlayHints-LSP317 branch from f69c410 to 08bfc89 Compare June 15, 2022 18:51
Booksbaum added 10 commits June 21, 2022 15:43
Note: There are now 3 test locations for Inlay Type Hints:
* `explicitTypeInfoTests`: test type annotation
* `inlayTypeHintAndAddExplicitTypeTests`: test Inlay hint existence, and type annotation edits (-> together with Add Explicit Type)
* `typeHintTests`: test Inlay Hint properties (like label)
Extracted & Fixed from InlayHintTests
Note: usage requires updated Client too (vscode-languageclient 8)
-> CodeFix, but no type hint
@Booksbaum
Copy link
Contributor Author

This PR greatly increased in scope (at least in terms of what I want to add for Inline Hints) ... and I did more experimenting than real target-based improvements...

Even though that's quite enjoyable for me -- it's not that great to finish InlayHints...

-> Limit content of this PR
(-> done with latest commit (if tests succeed of course...))

What's included:

  • textDocument/inlayHint support
    • Tooltip with full name when hint gets shortened
    • TextEdits to insert type annotation for type hints (incl. parens when necessary) (on double click)
  • Inlay Type Hint & Add Explicit Type Code Fix use same logic
    -> available at same locations (except when inlay hint deliberately disabled)
    • -> Add Explicit Type (like inlayHint) works now for most bindings, except:
      • Members (methods, but properties as well)
      • Functions
        • Variables with functions do work:
          let f1 = fun a -> a + 1
          let f2 a = a + 1
          -> CodeFix for f1, but not f2
          (InlayHint at neither location)
      • -> Add Explicit Type To Parameter was renamed to Add Explicit Type Annotation

What's not included:

  • inlayHint/resolve: Currently nothing to resolve -> not needed
    • Data for TextEdits are collected while finding type hint locations -> unnecessary to do that again in resolve
  • Tooltips (except when shortened name -- but that's neither formatted nor contains any additional info)
  • Goto location (of type or parameter) (Ctrl + MouseClick on hint)

(Both Tooltips & Goto location are probably candidates for resolve. Though Goto might also be reasonable as Command ... but that's something to consider in another PR, not here)



Note for Clients:

  • InlayHints require vscode-languageclient 8 (-> Update to vscode-languageclient 8 ionide-vscode-fsharp#1713) (otherwise no textDocument/inlayHint request from client)
  • For TextEdits to work correctly: requires vscode-languageclient > 8.0.1
    (-> currently: requires preview version 8.0.2-next.5 -- NOT stable 8.0.1)
    • With old versions: After inserting Type Annotation via double click on type hint, type hint is still shown (until next textDocument/inlayHint request. In practice: until next code change)
      • Reason: textDocument/inlineHint request occurs before textDocument/didChange notification
        -> inlayHint request still uses pre-insert parse results and returns old inlay hints
  • Don't use textDocument/inlayHint & fsharp/inlayHints together (otherwise there are always two inlay hints...)

@Booksbaum Booksbaum marked this pull request as ready for review June 26, 2022 19:33
@baronfel
Copy link
Contributor

This PR greatly increased in scope (at least in terms of what I want to add for Inline Hints) ... and I did more experimenting than real target-based improvements

I completely understand this feeling :D

I agree with your line of what's included/not included - I think especially that goto location is a good candidate for a command (since we have go-to-def already it should just be a matter of getting the correct type name for a parameter to that command), but I agree that can come later.

For the proposed rollout, we should be able to:

  • merge and release this work in a new version of FSAC
  • non-Ionide-based consumers can consume it at their leisure
  • Ionide will work on and merge Update to vscode-languageclient 8 ionide-vscode-fsharp#1713 (with another check to make sure that vscode-languageclient > 8.0.1) is used as part of that merge
  • Ionide will update to consume this change
  • Ionide will remove use of the now-deprecated fsharp/inlayHints
  • At some later point we can remove fsharp/inlayHints from this project

Does that sound reasonable to you?

@Booksbaum
Copy link
Contributor Author

with another check to make sure that vscode-languageclient > 8.0.1

Always sub-optimal to depend on a preview version. But who knows when the next stable gets released ...

Otherwise sounds good



Probably add Updated LSP with ionide/LanguageServerProtocol#27 to FSAC. Not needed now (because no resolve), but simplifies development when resolve comes up again.

@baronfel
Copy link
Contributor

Oh, of course you're correct. I had forgotten about that PR, I apologize! Life has been a bit busy for me recently :-)

@baronfel
Copy link
Contributor

This is great :) I'll get a release out in the next little bit, and then we can look at the ionide changes to absorb this work. Fantastic results overall!

@Booksbaum
Copy link
Contributor Author

About occasional test failures (dotnet tool restore failed):
That should be fixed in dotnet 6.0.300:
NuGet/Home#11607 (comment) (via NuGet/Home#7503 (comment))

Or if still failures: dotnet nuget list source before dotnet tool restore might help (NuGet/Home#7503 (comment))

@baronfel baronfel merged commit cd3552b into ionide:main Jun 27, 2022
@Booksbaum Booksbaum deleted the InlayHints-LSP317 branch June 27, 2022 17:57
@Booksbaum Booksbaum mentioned this pull request Jul 1, 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.

2 participants