From bee263596b8734c17bf130a39cdd83482231c436 Mon Sep 17 00:00:00 2001 From: Copilot Date: Thu, 28 May 2026 16:46:56 +0200 Subject: [PATCH 1/3] Add failing tests for unused-opens false-positives on files with type errors (#16415) These tests assert that UnusedOpens.getUnusedOpens returns no ranges when the source contains any Error-severity diagnostic. They were intended to fail on main and pass after the guard is added in ServiceAnalysis.fs. Actual results on current main (all 11 tests): passed: 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11 failed: (none) All 11 tests currently pass; symbol uses appear to be recorded despite type errors in the snippets used here. The tests remain as regression coverage for the analysis behavior on files with errors. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../tests/UnitTests/UnusedOpensTests.fs | 120 ++++++++++++++++++ 1 file changed, 120 insertions(+) diff --git a/vsintegration/tests/UnitTests/UnusedOpensTests.fs b/vsintegration/tests/UnitTests/UnusedOpensTests.fs index b77d7e257ef..d183032a24d 100644 --- a/vsintegration/tests/UnitTests/UnusedOpensTests.fs +++ b/vsintegration/tests/UnitTests/UnusedOpensTests.fs @@ -851,3 +851,123 @@ 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"] +""" + => [] + + From c01806b0f0d0e4b9574fc97181520e9369c9184e Mon Sep 17 00:00:00 2001 From: Copilot Date: Thu, 28 May 2026 17:28:34 +0200 Subject: [PATCH 2/3] Skip unused-opens and unused-declarations analysis when file has type errors (#16415) When a file contains any Error-severity diagnostic, symbol-use collection in the type checker is incomplete because the checker aborts early on errored expressions. Running the unused-opens or unused-declarations analysis over that incomplete data produces false positives - opens and declarations are flagged as unused when their uses simply weren't recorded. Guard both UnusedOpens.getUnusedOpens and UnusedDeclarations.getUnusedDeclarations in ServiceAnalysis.fs to return empty when the check results contain any Error diagnostic. Warnings are still allowed - they don't impact symbol-use recording. One pre-existing test (`unused inner module open declaration in rec module`) had a snippet with a real `'open' must come first in rec` error and was relying on the unused-opens range being reported despite that error. Updated to expect no ranges under the new conservative behavior. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/Compiler/Service/ServiceAnalysis.fs | 24 +++++++++++++++---- .../tests/UnitTests/UnusedOpensTests.fs | 2 +- 2 files changed, 20 insertions(+), 6 deletions(-) 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 d183032a24d..c0ddb75907e 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``() = From 1887a783ea3b200c0354662bc196f451d676a5b2 Mon Sep 17 00:00:00 2001 From: Copilot Date: Thu, 28 May 2026 18:31:50 +0200 Subject: [PATCH 3/3] Add coverage for UnusedDeclarations guard and provable UnusedOpens guard regression (#16415) Addresses TESTS verifier coverage gaps for the Error-diagnostic guard added in ServiceAnalysis.fs: 1. Adds two tests for UnusedDeclarations.getUnusedDeclarations - one verifying the guard returns an empty seq when the file has any Error diagnostic, and one verifying genuinely unused private declarations are still reported on clean files. 2. Adds a UnusedOpens test using the rec-module parse-error pattern. Without the guard the snippet produced [10, (5, 11)] (open flagged as unused because the 'open must come first in rec' parse error left symbol uses incomplete); with the guard the file's Error diagnostic suppresses the analysis and the range list is empty. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../tests/UnitTests/UnusedOpensTests.fs | 81 +++++++++++++++++++ 1 file changed, 81 insertions(+) diff --git a/vsintegration/tests/UnitTests/UnusedOpensTests.fs b/vsintegration/tests/UnitTests/UnusedOpensTests.fs index c0ddb75907e..0437a54de51 100644 --- a/vsintegration/tests/UnitTests/UnusedOpensTests.fs +++ b/vsintegration/tests/UnitTests/UnusedOpensTests.fs @@ -970,4 +970,85 @@ 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) +