Further protection for GetDeclarations against internal errors (#3386)

* better error protection for language service

* protect against ICEs

src/fsharp/symbols/SymbolHelpers.fs

@@ -87,7 +87,7 @@ type FSharpErrorInfo(fileName, s:pos, e:pos, severity: FSharpErrorSeverity, mess
 [<Sealed>]
 type ErrorScope()  = 
     let mutable errors = [] 
-    static let mutable mostRecentError = None
+    let mutable firstError = None
     let unwindBP = PushThreadBuildPhaseUntilUnwind BuildPhase.TypeCheck
     let unwindEL =        
         PushErrorLoggerPhaseUntilUnwind (fun _oldLogger -> 
@@ -95,8 +95,8 @@ type ErrorScope()  =
                 member x.DiagnosticSink(exn, isError) = 
                       let err = FSharpErrorInfo.CreateFromException(exn,isError,false,range.Zero)
                       errors <- err :: errors
-                      if isError then 
-                          mostRecentError <- Some err
+                      if isError && firstError.IsNone then 
+                          firstError <- Some err.Message
                 member x.ErrorCount = errors.Length })
         
     member x.Errors = errors |> List.filter (fun error -> error.Severity = FSharpErrorSeverity.Error)
@@ -112,16 +112,33 @@ type ErrorScope()  =
               unwindEL.Dispose() (* unwind pushes when ErrorScope disposes *)
               unwindBP.Dispose()
 
-    static member MostRecentError = mostRecentError
+    member x.FirstError with get() = firstError and set v = firstError <- v
     
+    /// Used at entry points to FSharp.Compiler.Service (service.fsi) which manipulate symbols and
+    /// perform other operations which might expose us to either bona-fide F# error messages such 
+    /// "missing assembly" (for incomplete assembly reference sets), or, if there is a compiler bug,
+    /// may hit internal compiler failures.
+    ///
+    /// In some calling cases, we get a chance to report the error as part of user text. For example
+    /// if there is a "msising assembly" error while formatting the text of the description of an
+    /// autocomplete, then the error message is shown in replacement of the text (rather than crashing Visual
+    /// Studio, or swallowing the exception completely)
     static member Protect<'a> (m:range) (f:unit->'a) (err:string->'a): 'a = 
         use errorScope = new ErrorScope()
         let res = 
             try 
                 Some (f())
-            with e -> errorRecovery e m; None
+            with e -> 
+                // Here we only call errorRecovery to save the error message for later use by TryGetFirstErrorText.
+                try 
+                    errorRecovery e m
+                with _ -> 
+                    // If error recovery fails, then we have an internal compiler error. In this case, we show the whole stack
+                    // in the extra message, should the extra message be used.
+                    errorScope.FirstError <- Some (e.ToString())
+                None
         match res with 
-        | Some res ->res
+        | Some res -> res
         | None -> 
             match errorScope.TryGetFirstErrorText() with 
             | Some text -> err text

src/fsharp/vs/service.fs

@@ -80,7 +80,7 @@ module EnvMisc =
 [<RequireQualifiedAccess>]
 type FSharpFindDeclFailureReason = 
     // generic reason: no particular information about error
-    | Unknown
+    | Unknown of message: string
     // source code file is not available
     | NoSourceCode
     // trying to find declaration of ProvidedType without TypeProviderDefinitionLocationAttribute
@@ -1102,7 +1102,7 @@ type TypeCheckInfo
        (fun () -> 
           match GetDeclItemsForNamesAtPosition (ctok, None,Some(names), None, line, lineStr, colAtEndOfNames, ResolveTypeNamesToCtors,ResolveOverloads.Yes,(fun() -> []), fun _ -> false) with
           | None
-          | Some ([], _, _, _) -> FSharpFindDeclResult.DeclNotFound FSharpFindDeclFailureReason.Unknown
+          | Some ([], _, _, _) -> FSharpFindDeclResult.DeclNotFound (FSharpFindDeclFailureReason.Unknown "")
           | Some (item :: _, _, _, _) -> 
 
               // For IL-based entities, switch to a different item. This is because
@@ -1131,7 +1131,7 @@ type TypeCheckInfo
                   | _ -> FSharpFindDeclResult.DeclNotFound defaultReason
 
               match rangeOfItem g preferFlag item with
-              | None   -> fail FSharpFindDeclFailureReason.Unknown 
+              | None   -> fail (FSharpFindDeclFailureReason.Unknown "")
               | Some itemRange -> 
 
                   let projectDir = Filename.directoryName (if projectFileName = "" then mainInputFileName else projectFileName)
@@ -1143,7 +1143,7 @@ type TypeCheckInfo
        )
        (fun msg -> 
            Trace.TraceInformation(sprintf "FCS: recovering from error in GetDeclarationLocation: '%s'" msg)
-           FSharpFindDeclResult.DeclNotFound FSharpFindDeclFailureReason.Unknown)
+           FSharpFindDeclResult.DeclNotFound (FSharpFindDeclFailureReason.Unknown msg))
 
     member scope.GetSymbolUseAtLocation (ctok, line, lineStr, colAtEndOfNames, names) =
       ErrorScope.Protect Range.range0 
@@ -1852,7 +1852,7 @@ type FSharpCheckFileResults(filename: string, errors: FSharpErrorInfo[], scopeOp
             
     member info.GetDeclarationLocation (line, colAtEndOfNames, lineStr, names, ?preferFlag, ?userOpName: string) = 
         let userOpName = defaultArg userOpName "Unknown"
-        let dflt = FSharpFindDeclResult.DeclNotFound FSharpFindDeclFailureReason.Unknown
+        let dflt = FSharpFindDeclResult.DeclNotFound (FSharpFindDeclFailureReason.Unknown "")
         reactorOp userOpName "GetDeclarationLocation" dflt (fun ctok scope -> 
             scope.GetDeclarationLocation (ctok, line, lineStr, colAtEndOfNames, names, preferFlag))
 

src/fsharp/vs/service.fsi

@@ -37,8 +37,8 @@ type FSharpFindDeclFailureReason =
 type internal FSharpFindDeclFailureReason = 
 #endif
 
-    /// Generic reason: no particular information about error
-    | Unknown
+    /// Generic reason: no particular information about error apart from a message
+    | Unknown of message: string
 
     /// Source code file is not available
     | NoSourceCode

vsintegration/src/FSharp.LanguageService/GotoDefinition.fs

@@ -73,7 +73,7 @@ module internal GotoDefinition =
                           Trace.Write("LanguageService", sprintf "Goto definition failed: Reason %+A" reason)
                           let text = 
                               match reason with                    
-                              | FSharpFindDeclFailureReason.Unknown -> Strings.Errors.GotoDefinitionFailed()
+                              | FSharpFindDeclFailureReason.Unknown _message -> Strings.Errors.GotoDefinitionFailed()
                               | FSharpFindDeclFailureReason.NoSourceCode -> Strings.Errors.GotoDefinitionFailed_NoSourceCode()
                               | FSharpFindDeclFailureReason.ProvidedType(typeName) -> Strings.Errors.GotoDefinitionFailed_ProvidedType(typeName)
                               | FSharpFindDeclFailureReason.ProvidedMember(name) -> Strings.Errors.GotoFailed_ProvidedMember(name)