Results of Amplifying F# Sesssion (#1195)

* 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) * Fantomas
ionide · Nov 16, 2023 · ee65267 · ee65267
1 parent e4c242c
commit ee65267
Show file tree

Hide file tree

Showing 9 changed files with 167 additions and 69 deletions.
diff --git a/src/FsAutoComplete.Core/Commands.fs b/src/FsAutoComplete.Core/Commands.fs
@@ -534,9 +534,7 @@ module Commands =
       | Error e -> return CoreResponse.ErrorRes e
     }
 
-  let typesig (tyRes: ParseAndCheckResults) (pos: Position) lineStr =
-    tyRes.TryGetToolTip pos lineStr
-    |> Result.bimap CoreResponse.Res CoreResponse.ErrorRes
+  let typesig (tyRes: ParseAndCheckResults) (pos: Position) lineStr = tyRes.TryGetToolTip pos lineStr
 
   // 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 +544,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 +616,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
@@ -320,7 +320,9 @@ 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
@@ -332,27 +334,30 @@ 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)
-
-  member x.TryGetToolTipEnhanced
-    (pos: Position)
-    (lineStr: LineStr)
-    : Result<option<TryGetToolTipEnhancedResult>, string> =
+          | 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) : 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
@@ -373,12 +378,20 @@ 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
@@ -388,7 +401,12 @@ 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
@@ -398,7 +416,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("#", StringComparison.Ordinal) then
               let completionList =
@@ -568,7 +568,12 @@ 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
@@ -874,11 +879,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
@@ -939,13 +944,8 @@ 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
 
@@ -967,7 +967,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) =
@@ -990,7 +990,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
@@ -1064,7 +1064,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
@@ -1094,7 +1094,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
@@ -1123,7 +1123,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 =
@@ -1159,7 +1159,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
@@ -1201,7 +1201,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 (
@@ -1560,7 +1560,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)
@@ -2071,7 +2071,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
 
@@ -2158,7 +2158,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
@@ -2240,9 +2240,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
@@ -2275,7 +2275,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
@@ -2311,7 +2311,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!
@@ -2659,7 +2659,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
@@ -2690,7 +2690,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,12 @@ 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,27 @@ 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)
@@ -186,7 +204,9 @@ 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