-
Notifications
You must be signed in to change notification settings - Fork 156
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
& TextEdit
s
#943
Conversation
src/FsAutoComplete/LSP.Preview.fs
Outdated
/// export type LSPArray = LSPAny[]; | ||
/// ``` | ||
/// -> `'Data` must adhere to specs | ||
Data: 'Data option } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
733c2cc
to
b8ff486
Compare
Pos = data.InsertAt | ||
Kind = Type | ||
// TODO: or use tyForAnno? | ||
Text = ": " + (truncated ty) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unknown to me 🤷
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. |
-> ionide/LanguageServerProtocol#22 But no need to rush. Remaining tests (in this PR) will take some time to implement |
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. |
That would be great. Thanks! |
Is there a way to get documentation for a But for InlayHints I want a tooltip for type of that ident, not for the ident itself. |
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 | ||
""" | ||
] |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
try 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** |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
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 | ||
""" | ||
] |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unknown to me 🤷
/// Some Description
type T = class end
let f (a: T) (b: int) = ()
// ^ xxx
But I found Though it's a bit strange:
Exists already -- though with fewer cases |
72403d6
to
f69c410
Compare
About error on MacOS:
I have no idea why this fails. |
Can you get a simple, single example? If so we could submit it upstream. |
|
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)
(Already) handles other bindings too
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
f69c410
to
08bfc89
Compare
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)
CodeFix was already renamed
Extracted & Fixed from InlayHintTests
Note: usage requires updated Client too (vscode-languageclient 8)
-> CodeFix, but no type hint
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 What's included:
What's not included:
(Both Tooltips & Goto location are probably candidates for Note for Clients:
|
I completely understand this feeling :D I agree with your line of what's included/not included - I think especially that For the proposed rollout, we should be able to:
Does that sound reasonable to you? |
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 |
Oh, of course you're correct. I had forgotten about that PR, I apologize! Life has been a bit busy for me recently :-) |
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! |
About occasional test failures ( Or if still failures: |
Add & Implement
InlayHint
s from LSP3.17
(as mentioned in #935)3.17
was just released (on2022-05-10
) -- but in this PR I'm still treating it as still in preview: most code I wrote before3.17
and Types aren't yet inIonide.LanguageServerProtocol
-> in here still temporaryBoth
textDocument/inlayHint
andinlayHint/resolve
are implemented -- with exception ofTooltip
s: That's still alwaysNone
.inlayHint/resolve
: resolvesTooltip
&TextEdits
when missing fromInlayHint
-- but in reality it's actually completely unused & unnecessary:TextEdits
are always created when collecting all InlayHints and not delayed to beresolve
d later.Reason: I changed ident detection & validation (-> "should get a hint?") slightly for type hints. In the process I collect all data necessary for
TextEdit
s too -> no reason to resolve later again...Use
TextEdit
s instead of just "insert string at Inlay Hint Location" (-> necessary for parens)fsharp/inlayHints
still exists (-> compatibility with older version) -- but itsInsertText
can only insert at InlayHint location (-> no parens) -> might be wrongUse same logic for type
InlayHint
s &AddExplicitTypeAnnotation
CodeFixEnhancements around type annotations, like
?a
-> optional parameter, but type annotation cannot haveoption
(: 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:
->
AddExplicitType
can now add type annotations tof
.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 ofIonide.LanguageServerProtocol
?-> Then we don't have to remember to remove
LSP.Preview.fs
sometime later (but instead do that directly in this PR here)