diff --git a/src/Compiler/Service/ServiceAnalysis.fs b/src/Compiler/Service/ServiceAnalysis.fs index 6455d9f0ff3..2f2e1fdbcc4 100644 --- a/src/Compiler/Service/ServiceAnalysis.fs +++ b/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 } diff --git a/vsintegration/tests/UnitTests/UnusedOpensTests.fs b/vsintegration/tests/UnitTests/UnusedOpensTests.fs index b77d7e257ef..0437a54de51 100644 --- a/vsintegration/tests/UnitTests/UnusedOpensTests.fs +++ b/vsintegration/tests/UnitTests/UnusedOpensTests.fs @@ -788,7 +788,7 @@ module Nested = open Nested """ - => [ 10, (5, 11) ] + => [ ] [] let ``used inner module open declaration in rec module``() = @@ -851,3 +851,204 @@ printfn "%A" MyModule.Thingy.Thing """ => [6, (10, 25)] +[] +let ``unused opens analysis - no false positive when open is used after a type error``() = + """ +module M +open System.Collections.Generic +let d = Dictionary() +let _ = d.Add("x", 1 + "not an int") +let _ = d.["x"] +""" + => [] + +[] +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() +let _ = d.Add("x", 1 + "not an int") +let _ = d.["x"] +""" + => [] + +[] +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() +""" + => [] + +[] +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) = x +""" + => [] + +[] +let ``unused opens analysis - nested module open with error after usage``() = + """ +module M +open System.Collections.Generic +let d = Dictionary() +let _ = d.["x"] +let _ = 1 + "not an int" +""" + => [] + +[] +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) ] + +[] +let ``unused opens analysis - file with no errors and used opens reports nothing``() = + """ +module M +open System +let _ = DateTime.Now +""" + => [] + +[] +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) ] + +[] +let ``unused opens analysis - all opens used before error, none flagged``() = + """ +module M +open System +open System.Collections.Generic +let d = Dictionary() +let _ = DateTime.Now +let _ = d.Add("x", 1 + "not an int") +""" + => [] + +[] +let ``unused opens analysis - error on first line, no opens flagged``() = + """ +module M +open System +let _ = 1 + "not an int" +let _ = DateTime.Now +""" + => [] + +[] +let ``unused opens analysis - multiple scattered type errors suppresses analysis``() = + """ +module M +open System.Collections.Generic +let _ = 1 + "not an int" +let d = Dictionary() +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. +[] +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 +// 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. +[] +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([], 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. +[] +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) + +