Results of Amplifying F# Sesssion

Co-authored-by: Jimmy Byrd <[email protected]> * Replaces spurious hover errors with log messages * Similar treatment for certain helper functions * Basic test for pipeline hints (needs more coverage)
ionide · Nov 6, 2023 · 683625a · 683625a
1 parent c6fd5c4
commit 683625a
Show file tree

Hide file tree

Showing 9 changed files with 163 additions and 63 deletions.
diff --git 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
@@ -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,27 +334,39 @@ 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<option<TryGetToolTipEnhancedResult>, string> =
+    : option<TryGetToolTipEnhancedResult> =
     let (|EmptyTooltip|_|) (ToolTipText elems) =
       match elems with
       | [] -> Some()
       | elems when elems |> List.forall ((=) ToolTipElement.None) -> Some()
       | _ -> 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
@@ -47,9 +47,9 @@ type ParseAndCheckResults =
   member TryFindIdentifierDeclaration: pos: Position -> lineStr: LineStr -> Async<Result<FindDeclarationResult, string>>
 
   member TryFindTypeDeclaration: pos: Position -> lineStr: LineStr -> Async<Result<FindDeclarationResult, string>>
-  member TryGetToolTip: pos: Position -> lineStr: LineStr -> Result<ToolTipText, string>
+  member TryGetToolTip: pos: Position -> lineStr: LineStr -> ToolTipText option
 
-  member TryGetToolTipEnhanced: pos: Position -> lineStr: LineStr -> Result<TryGetToolTipEnhancedResult option, string>
+  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
@@ -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<Activity>
-            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
@@ -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
@@ -14,9 +14,18 @@ open FSharp.UMX
 open CliWrap
 
 
+module ErrorMsgUtils =
+  let formatLineLookErr (x : {| FileName: string<LocalPath>; 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<LocalPath>; 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