Suppress unused-opens/declarations false positives when file has type errors

src/Compiler/Service/ServiceAnalysis.fs

@@ -8,6 +8,7 @@ open System.Runtime.CompilerServices
 open Internal.Utilities.Library
 open FSharp.Compiler
 open FSharp.Compiler.CodeAnalysis
+open FSharp.Compiler.Diagnostics
 open FSharp.Compiler.Symbols
 open FSharp.Compiler.Syntax.PrettyNaming
 open FSharp.Compiler.Text
@@ -304,7 +305,13 @@ module UnusedOpens =
         async {
             use! _holder = Cancellable.UseToken()
 
-            if checkFileResults.OpenDeclarations.Length = 0 then
+            let hasErrors =
+                checkFileResults.Diagnostics
+                |> Array.exists (fun d -> d.Severity = FSharpDiagnosticSeverity.Error)
+
+            if hasErrors then
+                return []
+            elif checkFileResults.OpenDeclarations.Length = 0 then
                 return []
             else
                 let! ct = Async.CancellationToken
@@ -465,8 +472,15 @@ module UnusedDeclarations =
 
     let getUnusedDeclarations (checkFileResults: FSharpCheckFileResults, isScriptFile: bool) =
         async {
-            let! ct = Async.CancellationToken
-            let allSymbolUsesInFile = checkFileResults.GetAllUsesOfAllSymbolsInFile(ct)
-            let unusedRanges = getUnusedDeclarationRanges allSymbolUsesInFile isScriptFile
-            return unusedRanges
+            let hasErrors =
+                checkFileResults.Diagnostics
+                |> Array.exists (fun d -> d.Severity = FSharpDiagnosticSeverity.Error)
+
+            if hasErrors then
+                return Seq.empty
+            else
+                let! ct = Async.CancellationToken
+                let allSymbolUsesInFile = checkFileResults.GetAllUsesOfAllSymbolsInFile(ct)
+                let unusedRanges = getUnusedDeclarationRanges allSymbolUsesInFile isScriptFile
+                return unusedRanges
         }

vsintegration/tests/UnitTests/UnusedOpensTests.fs

@@ -788,7 +788,7 @@ module Nested =
 
 open Nested
 """
-    => [ 10, (5, 11) ]
+    => [ ]
 
 [<Fact>]
 let ``used inner module open declaration in rec module``() =
@@ -851,3 +851,204 @@ printfn "%A" MyModule.Thingy.Thing
     """
     => [6, (10, 25)]
 
+[<Fact>]
+let ``unused opens analysis - no false positive when open is used after a type error``() =
+    """
+module M
+open System.Collections.Generic
+let d = Dictionary<string, int>()
+let _ = d.Add("x", 1 + "not an int")
+let _ = d.["x"]
+"""
+    => []
+
+[<Fact>]
+let ``unused opens analysis - multiple opens, only post-error use, none flagged when file has errors``() =
+    """
+module M
+open System
+open System.Collections.Generic
+let _ = DateTime.Now
+let d = Dictionary<string, int>()
+let _ = d.Add("x", 1 + "not an int")
+let _ = d.["x"]
+"""
+    => []
+
+[<Fact>]
+let ``unused opens analysis - open used in function body, type error in different function``() =
+    """
+module M
+open System.Collections.Generic
+let f () = 1 + "not an int"
+let g () = Dictionary<string, int>()
+"""
+    => []
+
+[<Fact>]
+let ``unused opens analysis - open used only via type annotation after error``() =
+    """
+module M
+open System.Collections.Generic
+let _ = 1 + "not an int"
+let f (x: Dictionary<string, int>) = x
+"""
+    => []
+
+[<Fact>]
+let ``unused opens analysis - nested module open with error after usage``() =
+    """
+module M
+open System.Collections.Generic
+let d = Dictionary<string, int>()
+let _ = d.["x"]
+let _ = 1 + "not an int"
+"""
+    => []
+
+[<Fact>]
+let ``unused opens analysis - file with no errors still reports genuinely unused opens``() =
+    """
+module M
+open System
+open System.IO
+let _ = DateTime.Now
+"""
+    => [ 4, (5, 14) ]
+
+[<Fact>]
+let ``unused opens analysis - file with no errors and used opens reports nothing``() =
+    """
+module M
+open System
+let _ = DateTime.Now
+"""
+    => []
+
+[<Fact>]
+let ``unused opens analysis - warnings do not suppress analysis``() =
+    """
+module M
+open System
+open System.IO
+let f (x: int option) = match x with Some y -> y
+let _ = DateTime.Now
+"""
+    => [ 4, (5, 14) ]
+
+[<Fact>]
+let ``unused opens analysis - all opens used before error, none flagged``() =
+    """
+module M
+open System
+open System.Collections.Generic
+let d = Dictionary<string, int>()
+let _ = DateTime.Now
+let _ = d.Add("x", 1 + "not an int")
+"""
+    => []
+
+[<Fact>]
+let ``unused opens analysis - error on first line, no opens flagged``() =
+    """
+module M
+open System
+let _ = 1 + "not an int"
+let _ = DateTime.Now
+"""
+    => []
+
+[<Fact>]
+let ``unused opens analysis - multiple scattered type errors suppresses analysis``() =
+    """
+module M
+open System.Collections.Generic
+let _ = 1 + "not an int"
+let d = Dictionary<string, int>()
+let _ = "x" + 1
+let _ = d.["x"]
+"""
+    => []
+
+// This test exercises the Error-diagnostic guard in UnusedOpens.getUnusedOpens.
+// Without the guard, the snippet below produced [ 10, (5, 11) ] because the
+// `'open' must come first in rec` parse error left symbol uses incomplete and
+// the analysis flagged the open as unused. With the guard, the file's Error
+// diagnostic suppresses the analysis and the range list is empty.
+[<Fact>]
+let ``unused opens analysis - parse error in rec module suppresses analysis (guard regression)``() =
+    """
+module rec TopModule
+
+module Nested =
+    let x = 1
+    let f x = x
+    type T() = class end
+    type R = { F: int }
+
+open Nested
+"""
+    => [ ]
+
+// Verifies UnusedDeclarations.getUnusedDeclarations returns an empty seq<range>
+// when the file has any Error-severity diagnostic. The let-binding `unused`
+// would normally be flagged as an unused declaration, but the type error must
+// suppress the analysis.
+[<Fact>]
+let ``unused declarations analysis - file with type error returns empty``() =
+    let source = """
+module M
+let unused = 42
+let _ = 1 + "not an int"
+"""
+    let _, checkFileAnswer =
+        checker.ParseAndCheckFileInProject(filePath, 0, FSharp.Compiler.Text.SourceText.ofString source, projectOptions)
+        |> Async.RunSynchronously
+
+    let checkFileResults =
+        match checkFileAnswer with
+        | FSharpCheckFileAnswer.Aborted -> failwithf "ParseAndCheckFileInProject aborted"
+        | FSharpCheckFileAnswer.Succeeded(r) -> r
+
+    let hasError =
+        checkFileResults.Diagnostics
+        |> Array.exists (fun d -> d.Severity = FSharp.Compiler.Diagnostics.FSharpDiagnosticSeverity.Error)
+    Assert.True(hasError, "expected source to contain at least one Error diagnostic")
+
+    let unused =
+        UnusedDeclarations.getUnusedDeclarations(checkFileResults, false)
+        |> Async.RunSynchronously
+        |> Seq.toList
+    Assert.Equal<range list>([], unused)
+
+// Verifies UnusedDeclarations.getUnusedDeclarations still reports genuinely
+// unused declarations on a clean file (no Error diagnostics), so the guard
+// doesn't silently disable the analysis everywhere.
+[<Fact>]
+let ``unused declarations analysis - clean file still reports unused``() =
+    let source = """
+module M
+let private unused = 42
+let _ = 1
+"""
+    let _, checkFileAnswer =
+        checker.ParseAndCheckFileInProject(filePath, 0, FSharp.Compiler.Text.SourceText.ofString source, projectOptions)
+        |> Async.RunSynchronously
+
+    let checkFileResults =
+        match checkFileAnswer with
+        | FSharpCheckFileAnswer.Aborted -> failwithf "ParseAndCheckFileInProject aborted"
+        | FSharpCheckFileAnswer.Succeeded(r) -> r
+
+    let hasError =
+        checkFileResults.Diagnostics
+        |> Array.exists (fun d -> d.Severity = FSharp.Compiler.Diagnostics.FSharpDiagnosticSeverity.Error)
+    Assert.False(hasError, "expected clean source to have no Error diagnostics")
+
+    let unused =
+        UnusedDeclarations.getUnusedDeclarations(checkFileResults, false)
+        |> Async.RunSynchronously
+        |> Seq.toList
+    Assert.NotEmpty(unused)
+
+