From 683625a25eb32b5148e3bf5e2ceaa8281625c15b Mon Sep 17 00:00:00 2001 From: Paul Blasucci Date: Mon, 6 Nov 2023 23:33:25 +0100 Subject: [PATCH] Results of Amplifying F# Sesssion Co-authored-by: Jimmy Byrd * Replaces spurious hover errors with log messages * Similar treatment for certain helper functions * Basic test for pipeline hints (needs more coverage) --- src/FsAutoComplete.Core/Commands.fs | 5 +- .../ParseAndCheckResults.fs | 58 ++++++++++++---- .../ParseAndCheckResults.fsi | 4 +- .../LspServers/AdaptiveFSharpLspServer.fs | 52 +++++++------- .../LspServers/AdaptiveServerState.fs | 6 +- src/FsAutoComplete/LspServers/Common.fs | 11 ++- .../FsAutoComplete.Tests.Lsp.fsproj | 20 ++---- .../InlineHintsTests.fs | 69 +++++++++++++++++++ test/FsAutoComplete.Tests.Lsp/Program.fs | 1 + 9 files changed, 163 insertions(+), 63 deletions(-) create mode 100644 test/FsAutoComplete.Tests.Lsp/InlineHintsTests.fs diff --git a/src/FsAutoComplete.Core/Commands.fs b/src/FsAutoComplete.Core/Commands.fs index 0c2856a17..1caef0080 100644 --- a/src/FsAutoComplete.Core/Commands.fs +++ b/src/FsAutoComplete.Core/Commands.fs @@ -536,7 +536,6 @@ module Commands = let typesig (tyRes: ParseAndCheckResults) (pos: Position) lineStr = tyRes.TryGetToolTip pos lineStr - |> Result.bimap CoreResponse.Res CoreResponse.ErrorRes // Calculates pipeline hints for now as in fSharp/pipelineHint with a bit of formatting on the hints let inlineValues (contents: IFSACSourceText) (tyRes: ParseAndCheckResults) : Async<(pos * String)[]> = @@ -546,7 +545,7 @@ module Commands = option { let! lineStr = contents.GetLine pos - let! tip = tyRes.TryGetToolTip pos lineStr |> Option.ofResult + let! tip = tyRes.TryGetToolTip pos lineStr return TipFormatter.extractGenericParameters tip } @@ -618,7 +617,7 @@ module Commands = option { let! lineStr = contents.GetLine pos - let! tip = tyRes.TryGetToolTip pos lineStr |> Option.ofResult + let! tip = tyRes.TryGetToolTip pos lineStr return TipFormatter.extractGenericParameters tip } diff --git a/src/FsAutoComplete.Core/ParseAndCheckResults.fs b/src/FsAutoComplete.Core/ParseAndCheckResults.fs index feae26dfc..5c3533802 100644 --- a/src/FsAutoComplete.Core/ParseAndCheckResults.fs +++ b/src/FsAutoComplete.Core/ParseAndCheckResults.fs @@ -318,7 +318,11 @@ type ParseAndCheckResults member __.TryGetToolTip (pos: Position) (lineStr: LineStr) = match Lexer.findLongIdents (pos.Column, lineStr) with - | None -> ResultOrString.Error "Cannot find ident for tooltip" + | None -> + logger.info ( + Log.setMessageI $"Cannot find ident for tooltip: {pos.Column:column} in {lineStr:lineString}" + ) + None | Some(col, identIsland) -> let identIsland = Array.toList identIsland // TODO: Display other tooltip types, for example for strings or comments where appropriate @@ -330,15 +334,23 @@ type ParseAndCheckResults match identIsland with | [ ident ] -> match KeywordList.keywordTooltips.TryGetValue ident with - | true, tip -> Ok tip - | _ -> ResultOrString.Error "No tooltip information" - | _ -> ResultOrString.Error "No tooltip information" - | _ -> Ok(tip) + | true, tip -> Some tip + | _ -> + logger.info ( + Log.setMessageI $"Cannot find ident for tooltip: {pos.Column:column} in {lineStr:lineString}" + ) + None + | _ -> + logger.info ( + Log.setMessageI $"Cannot find ident for tooltip: {pos.Column:column} in {lineStr:lineString}" + ) + None + | _ -> Some tip member x.TryGetToolTipEnhanced (pos: Position) (lineStr: LineStr) - : Result, string> = + : option = let (|EmptyTooltip|_|) (ToolTipText elems) = match elems with | [] -> Some() @@ -346,11 +358,15 @@ type ParseAndCheckResults | _ -> None match Completion.atPos (pos, x.GetParseResults.ParseTree) with - | Completion.Context.StringLiteral -> Ok None + | Completion.Context.StringLiteral -> None | Completion.Context.SynType | Completion.Context.Unknown -> match Lexer.findLongIdents (pos.Column, lineStr) with - | None -> Error "Cannot find ident for tooltip" + | None -> + logger.info ( + Log.setMessageI $"Cannot find ident for tooltip: {pos.Column:column} in {lineStr:lineString}" + ) + None | Some(col, identIsland) -> let identIsland = Array.toList identIsland // TODO: Display other tooltip types, for example for strings or comments where appropriate @@ -371,12 +387,23 @@ type ParseAndCheckResults Footer = "" SymbolInfo = TryGetToolTipEnhancedResult.Keyword ident } |> Some - |> Ok - | _ -> Error "No tooltip information" - | _ -> Error "No tooltip information" + | _ -> + logger.info ( + Log.setMessageI $"Cannot find ident for tooltip: {pos.Column:column} in {lineStr:lineString}" + ) + None + | _ -> + logger.info ( + Log.setMessageI $"Cannot find ident for tooltip: {pos.Column:column} in {lineStr:lineString}" + ) + None | _ -> match symbol with - | None -> Error "No tooltip information" + | None -> + logger.info ( + Log.setMessageI $"Cannot find ident for tooltip: {pos.Column:column} in {lineStr:lineString}" + ) + None | Some symbol -> // Retrieve the FSharpSymbol instance so we can find the XmlDocSig @@ -386,7 +413,11 @@ type ParseAndCheckResults let resolvedType = symbol.Symbol.GetAbbreviatedParent() match SignatureFormatter.getTooltipDetailsFromSymbolUse symbol with - | None -> Error "No tooltip information" + | None -> + logger.info ( + Log.setMessageI $"Cannot find tooltip for {symbol:symbol} ({pos.Column:column} in {lineStr:lineString})" + ) + None | Some(signature, footer) -> { ToolTipText = tip Signature = signature @@ -396,7 +427,6 @@ type ParseAndCheckResults {| XmlDocSig = resolvedType.XmlDocSig Assembly = symbol.Symbol.Assembly.SimpleName |} } |> Some - |> Ok member __.TryGetFormattedDocumentation (pos: Position) (lineStr: LineStr) = match Lexer.findLongIdents (pos.Column, lineStr) with diff --git a/src/FsAutoComplete.Core/ParseAndCheckResults.fsi b/src/FsAutoComplete.Core/ParseAndCheckResults.fsi index 73ae4f0dc..f3099f45a 100644 --- a/src/FsAutoComplete.Core/ParseAndCheckResults.fsi +++ b/src/FsAutoComplete.Core/ParseAndCheckResults.fsi @@ -47,9 +47,9 @@ type ParseAndCheckResults = member TryFindIdentifierDeclaration: pos: Position -> lineStr: LineStr -> Async> member TryFindTypeDeclaration: pos: Position -> lineStr: LineStr -> Async> - member TryGetToolTip: pos: Position -> lineStr: LineStr -> Result + member TryGetToolTip: pos: Position -> lineStr: LineStr -> ToolTipText option - member TryGetToolTipEnhanced: pos: Position -> lineStr: LineStr -> Result + member TryGetToolTipEnhanced: pos: Position -> lineStr: LineStr -> TryGetToolTipEnhancedResult option member TryGetFormattedDocumentation: pos: Position -> diff --git a/src/FsAutoComplete/LspServers/AdaptiveFSharpLspServer.fs b/src/FsAutoComplete/LspServers/AdaptiveFSharpLspServer.fs index 170cff38f..63449a4b6 100644 --- a/src/FsAutoComplete/LspServers/AdaptiveFSharpLspServer.fs +++ b/src/FsAutoComplete/LspServers/AdaptiveFSharpLspServer.fs @@ -540,7 +540,7 @@ type AdaptiveFSharpLspServer return None // An empty file has empty completions. Otherwise we would error down there else - let! lineStr = volatileFile.Source |> tryGetLineStr pos |> Result.ofStringErr + let! lineStr = volatileFile.Source |> tryGetLineStr pos |> Result.lineLookupErr if lineStr.StartsWith "#" then let completionList = @@ -568,7 +568,11 @@ type AdaptiveFSharpLspServer asyncResult { let! volatileFile = state.GetOpenFileOrRead filePath - let! lineStr = volatileFile.Source |> tryGetLineStr pos + let! lineStr = + volatileFile.Source + |> tryGetLineStr pos + |> Result.mapError ErrorMsgUtils.formatLineLookErr + //TODO ⮝⮝⮝ good candidate for better error model -- review! // TextDocumentCompletion will sometimes come in before TextDocumentDidChange // This will require the trigger character to be at the place VSCode says it is @@ -871,11 +875,11 @@ type AdaptiveFSharpLspServer let (filePath, pos) = getFilePathAndPosition p let! volatileFile = state.GetOpenFileOrRead filePath |> AsyncResult.ofStringErr - let! lineStr = volatileFile.Source |> tryGetLineStr pos |> Result.ofStringErr + let! lineStr = volatileFile.Source |> tryGetLineStr pos |> Result.lineLookupErr and! tyRes = state.GetOpenFileTypeCheckResultsCached filePath |> AsyncResult.ofStringErr match tyRes.TryGetToolTipEnhanced pos lineStr with - | Ok(Some tooltipResult) -> + | Some tooltipResult -> logger.info ( Log.setMessage "TryGetToolTipEnhanced : {params}" >> Log.addContextDestructured "params" tooltipResult @@ -936,13 +940,9 @@ type AdaptiveFSharpLspServer | TipFormatter.TipFormatterResult.None -> return None - | Ok(None) -> + | None -> + return None - return! LspResult.internalError $"No TryGetToolTipEnhanced results for {filePath}" - | Error e -> - trace.RecordError(e, "TextDocumentHover.Error") |> ignore - logger.error (Log.setMessage "Failed with {error}" >> Log.addContext "error" e) - return! LspResult.internalError e with e -> trace |> Tracing.recordException e @@ -964,7 +964,7 @@ type AdaptiveFSharpLspServer let (filePath, pos) = getFilePathAndPosition p let! volatileFile = state.GetOpenFileOrRead filePath |> AsyncResult.ofStringErr - let! lineStr = volatileFile.Source |> tryGetLineStr pos |> Result.ofStringErr + let! lineStr = volatileFile.Source |> tryGetLineStr pos |> Result.lineLookupErr let! tyRes = state.GetOpenFileTypeCheckResults filePath |> AsyncResult.ofStringErr let! (_, _, range) = @@ -987,7 +987,7 @@ type AdaptiveFSharpLspServer let (filePath, pos) = getFilePathAndPosition p let! volatileFile = state.GetOpenFileOrRead filePath |> AsyncResult.ofStringErr - let! lineStr = volatileFile.Source |> tryGetLineStr pos |> Result.ofStringErr + let! lineStr = volatileFile.Source |> tryGetLineStr pos |> Result.lineLookupErr and! tyRes = state.GetOpenFileTypeCheckResults filePath |> AsyncResult.ofStringErr // validate name and surround with backticks if necessary @@ -1061,7 +1061,7 @@ type AdaptiveFSharpLspServer let (filePath, pos) = getFilePathAndPosition p let! volatileFile = state.GetOpenFileOrRead filePath |> AsyncResult.ofStringErr - let! lineStr = volatileFile.Source |> tryGetLineStr pos |> Result.ofStringErr + let! lineStr = volatileFile.Source |> tryGetLineStr pos |> Result.lineLookupErr and! tyRes = state.GetOpenFileTypeCheckResults filePath |> AsyncResult.ofStringErr let! decl = tyRes.TryFindDeclaration pos lineStr |> AsyncResult.ofStringErr return decl |> findDeclToLspLocation |> GotoResult.Single |> Some @@ -1091,7 +1091,7 @@ type AdaptiveFSharpLspServer let (filePath, pos) = getFilePathAndPosition p let! volatileFile = state.GetOpenFileOrRead filePath |> AsyncResult.ofStringErr - let! lineStr = volatileFile.Source |> tryGetLineStr pos |> Result.ofStringErr + let! lineStr = volatileFile.Source |> tryGetLineStr pos |> Result.lineLookupErr and! tyRes = state.GetOpenFileTypeCheckResults filePath |> AsyncResult.ofStringErr let! decl = tyRes.TryFindTypeDeclaration pos lineStr |> AsyncResult.ofStringErr return decl |> findDeclToLspLocation |> GotoResult.Single |> Some @@ -1120,7 +1120,7 @@ type AdaptiveFSharpLspServer let (filePath, pos) = getFilePathAndPosition p let! volatileFile = state.GetOpenFileOrRead filePath |> AsyncResult.ofStringErr - let! lineStr = tryGetLineStr pos volatileFile.Source |> Result.ofStringErr + let! lineStr = tryGetLineStr pos volatileFile.Source |> Result.lineLookupErr and! tyRes = state.GetOpenFileTypeCheckResults filePath |> AsyncResult.ofStringErr let! usages = @@ -1156,7 +1156,7 @@ type AdaptiveFSharpLspServer let (filePath, pos) = getFilePathAndPosition p let! volatileFile = state.GetOpenFileOrRead filePath |> AsyncResult.ofStringErr - let! lineStr = tryGetLineStr pos volatileFile.Source |> Result.ofStringErr + let! lineStr = tryGetLineStr pos volatileFile.Source |> Result.lineLookupErr and! tyRes = state.GetOpenFileTypeCheckResults filePath |> AsyncResult.ofStringErr match @@ -1198,7 +1198,7 @@ type AdaptiveFSharpLspServer let (filePath, pos) = getFilePathAndPosition p let! volatileFile = state.GetOpenFileOrRead filePath |> AsyncResult.ofStringErr - let! lineStr = tryGetLineStr pos volatileFile.Source |> Result.ofStringErr + let! lineStr = tryGetLineStr pos volatileFile.Source |> Result.lineLookupErr and! tyRes = state.GetOpenFileTypeCheckResults filePath |> AsyncResult.ofStringErr logger.info ( @@ -1557,7 +1557,7 @@ type AdaptiveFSharpLspServer ) let! (sourceText: IFSACSourceText) = state.GetOpenFileSource filePath |> AsyncResult.ofStringErr - let! lineStr = sourceText |> tryGetLineStr pos |> Result.ofStringErr + let! lineStr = sourceText |> tryGetLineStr pos |> Result.lineLookupErr let typ = data.[1] let! r = Async.Catch(f arg pos tyRes sourceText lineStr typ filePath) @@ -2068,7 +2068,7 @@ type AdaptiveFSharpLspServer let filePath = Path.FileUriToLocalPath p.Item.Uri |> Utils.normalizePath let pos = protocolPosToPos p.Item.SelectionRange.Start let! volatileFile = state.GetOpenFileOrRead filePath |> AsyncResult.ofStringErr - let! lineStr = tryGetLineStr pos volatileFile.Source |> Result.ofStringErr + let! lineStr = tryGetLineStr pos volatileFile.Source |> Result.lineLookupErr // Incoming file may not be "Opened" so we need to force a typecheck let! tyRes = state.GetTypeCheckResultsForFile filePath |> AsyncResult.ofStringErr @@ -2155,7 +2155,7 @@ type AdaptiveFSharpLspServer |> getFilePathAndPosition let! volatileFile = state.GetOpenFileOrRead filePath |> AsyncResult.ofStringErr - let! lineStr = tryGetLineStr pos volatileFile.Source |> Result.ofStringErr + let! lineStr = tryGetLineStr pos volatileFile.Source |> Result.lineLookupErr and! tyRes = state.GetOpenFileTypeCheckResults filePath |> AsyncResult.ofStringErr let! decl = tyRes.TryFindDeclaration pos lineStr |> AsyncResult.ofStringErr @@ -2237,9 +2237,9 @@ type AdaptiveFSharpLspServer let (filePath, pos) = getFilePathAndPosition p let! volatileFile = state.GetOpenFileOrRead filePath |> AsyncResult.ofStringErr - let! lineStr = volatileFile.Source |> tryGetLineStr pos |> Result.ofStringErr + let! lineStr = volatileFile.Source |> tryGetLineStr pos |> Result.lineLookupErr and! tyRes = state.GetOpenFileTypeCheckResults filePath |> AsyncResult.ofStringErr - let! tip = Commands.typesig tyRes pos lineStr |> Result.ofCoreResponse + let tip = Commands.typesig tyRes pos lineStr return tip @@ -2272,7 +2272,7 @@ type AdaptiveFSharpLspServer let filePath = p.TextDocument.GetFilePath() |> Utils.normalizePath let! volatileFile = state.GetOpenFileOrRead filePath |> AsyncResult.ofStringErr - let! lineStr = volatileFile.Source |> tryGetLineStr pos |> Result.ofStringErr + let! lineStr = volatileFile.Source |> tryGetLineStr pos |> Result.lineLookupErr and! tyRes = state.GetOpenFileTypeCheckResults filePath |> AsyncResult.ofStringErr let! (typ, parms, generics) = tyRes.TryGetSignatureData pos lineStr |> Result.ofStringErr @@ -2308,7 +2308,7 @@ type AdaptiveFSharpLspServer let (filePath, pos) = getFilePathAndPosition p let! volatileFile = state.GetOpenFileOrRead filePath |> AsyncResult.ofStringErr - let! lineStr = volatileFile.Source |> tryGetLineStr pos |> Result.ofStringErr + let! lineStr = volatileFile.Source |> tryGetLineStr pos |> Result.lineLookupErr and! tyRes = state.GetOpenFileTypeCheckResults filePath |> AsyncResult.ofStringErr match! @@ -2656,7 +2656,7 @@ type AdaptiveFSharpLspServer let (filePath, pos) = getFilePathAndPosition p let! volatileFile = state.GetOpenFileOrRead filePath |> AsyncResult.ofStringErr - let! lineStr = volatileFile.Source |> tryGetLineStr pos |> Result.ofStringErr + let! lineStr = volatileFile.Source |> tryGetLineStr pos |> Result.lineLookupErr and! tyRes = state.GetOpenFileTypeCheckResults filePath |> AsyncResult.ofStringErr match! Commands.Help tyRes pos lineStr |> Result.ofCoreResponse with @@ -2687,7 +2687,7 @@ type AdaptiveFSharpLspServer let (filePath, pos) = getFilePathAndPosition p let! volatileFile = state.GetOpenFileOrRead filePath |> AsyncResult.ofStringErr - let! lineStr = volatileFile.Source |> tryGetLineStr pos |> Result.ofStringErr + let! lineStr = volatileFile.Source |> tryGetLineStr pos |> Result.lineLookupErr and! tyRes = state.GetOpenFileTypeCheckResults filePath |> AsyncResult.ofStringErr lastFSharpDocumentationTypeCheck <- Some tyRes diff --git a/src/FsAutoComplete/LspServers/AdaptiveServerState.fs b/src/FsAutoComplete/LspServers/AdaptiveServerState.fs index 11c2daa98..cd76b66b0 100644 --- a/src/FsAutoComplete/LspServers/AdaptiveServerState.fs +++ b/src/FsAutoComplete/LspServers/AdaptiveServerState.fs @@ -1634,7 +1634,11 @@ type AdaptiveState(lspClient: FSharpLspClient, sourceTextFactory: ISourceTextFac let tryGetParseResultsForFile filePath pos = asyncResult { let! (file) = forceFindOpenFileOrRead filePath - let! lineStr = file.Source |> tryGetLineStr pos + let! lineStr = + file.Source + |> tryGetLineStr pos + |> Result.mapError ErrorMsgUtils.formatLineLookErr + //TODO ⮝⮝⮝ good candidate for better error model -- review! and! tyRes = forceGetOpenFileTypeCheckResults filePath return tyRes, lineStr, file.Source } diff --git a/src/FsAutoComplete/LspServers/Common.fs b/src/FsAutoComplete/LspServers/Common.fs index cc256a613..cef575b55 100644 --- a/src/FsAutoComplete/LspServers/Common.fs +++ b/src/FsAutoComplete/LspServers/Common.fs @@ -14,9 +14,18 @@ open FSharp.UMX open CliWrap +module ErrorMsgUtils = + let formatLineLookErr (x : {| FileName: string; Position: FcsPos |}) = + let position = fcsPosToLsp x.Position + $"No line in {x.FileName} at position {position}" + + module Result = let ofStringErr r = r |> Result.mapError JsonRpc.Error.InternalErrorMessage + let lineLookupErr (r : Result<'T, {| FileName: string; Position: FcsPos |}>) = + r |> Result.mapError (ErrorMsgUtils.formatLineLookErr >> JsonRpc.Error.InternalErrorMessage) + let ofCoreResponse (r: CoreResponse<'a>) = match r with | CoreResponse.Res a -> Ok(Some a) @@ -187,7 +196,7 @@ module Helpers = let tryGetLineStr pos (text: IFSACSourceText) = text.GetLine(pos) - |> Result.ofOption (fun () -> $"No line in {text.FileName} at position {pos}") + |> Result.ofOption (fun () -> {| FileName = text.FileName; Position = pos |}) let fullPathNormalized = Path.GetFullPath >> Utils.normalizePath >> UMX.untag diff --git a/test/FsAutoComplete.Tests.Lsp/FsAutoComplete.Tests.Lsp.fsproj b/test/FsAutoComplete.Tests.Lsp/FsAutoComplete.Tests.Lsp.fsproj index 3bb6270cd..674e2e9d8 100644 --- a/test/FsAutoComplete.Tests.Lsp/FsAutoComplete.Tests.Lsp.fsproj +++ b/test/FsAutoComplete.Tests.Lsp/FsAutoComplete.Tests.Lsp.fsproj @@ -8,7 +8,6 @@ true true true - @@ -47,27 +46,16 @@ - + - + - - - + + - diff --git a/test/FsAutoComplete.Tests.Lsp/InlineHintsTests.fs b/test/FsAutoComplete.Tests.Lsp/InlineHintsTests.fs new file mode 100644 index 000000000..5f5cc51b5 --- /dev/null +++ b/test/FsAutoComplete.Tests.Lsp/InlineHintsTests.fs @@ -0,0 +1,69 @@ +module FsAutoComplete.Tests.InlineHints + +open Expecto +open Helpers +open Ionide.LanguageServerProtocol.Types +open FsAutoComplete +open Utils.Server +open Utils.Utils +open Utils.TextEdit +open Utils.ServerTests +open Helpers.Expecto.ShadowedTimeouts + +let private testInlineHints (server : CachedServer) text checkResp = + async { + let range, text = + text + |> Text.trimTripleQuotation + |> Cursor.assertExtractRange + + let! (doc, diags) = server |> Server.createUntitledDocument text + use doc = doc + + let inlineValueRequest: InlineValueParams = { + Context = { + FrameId = 0 + StoppedLocation = range + } + Range = range + TextDocument = doc.TextDocumentIdentifier + } + let! resp = doc.Server.Server.TextDocumentInlineValue inlineValueRequest + checkResp resp + } + +let good = """ +let test value = + $0 + value + |> String.replicate 3 + |> String.filter (fun c -> Char.IsAscii(c)) + |> String.length + $0 +value "foo" +""" + +let tests state = + serverTestList "inline hints" state defaultConfigDto None (fun server -> [ + testCaseAsync + "Gets inline hints for simple pipeline" + (testInlineHints + server + good + (fun resp -> + Expect.isOk resp "should get inline hints for valid code fragment" + let resp = resp |> Result.defaultWith (fun _ -> failwith "Unpossible!") + Expect.isSome resp "should get inline hints for valid code fragment" + let resp = resp |> Option.get + Expect.hasLength resp 4 "THERE ARE FOUR LIGHTS!!" + let hints = + resp + |> Array.choose ( + function + | InlineValue.InlineValueText hint -> Some hint.Text + | _ -> None + ) + Expect.containsAll hints [nameof string; nameof int] "should be 3 strings and an int" + ) + ) + ]) diff --git a/test/FsAutoComplete.Tests.Lsp/Program.fs b/test/FsAutoComplete.Tests.Lsp/Program.fs index 437c073ad..793d938b1 100644 --- a/test/FsAutoComplete.Tests.Lsp/Program.fs +++ b/test/FsAutoComplete.Tests.Lsp/Program.fs @@ -96,6 +96,7 @@ let lspTests = analyzerTests createServer signatureTests createServer SignatureHelp.tests createServer + InlineHints.tests createServer CodeFixTests.Tests.tests sourceTextFactory createServer Completion.tests createServer GoTo.tests createServer