Fix #16360: stop duplicating editor diagnostics (revert Roslyn workaround)

docs/release-notes/.VisualStudio/18.vNext.md

@@ -5,6 +5,7 @@
 * Find All References for external DLL symbols now only searches projects that reference the specific assembly. ([Issue #10227](https://github.com/dotnet/fsharp/issues/10227), [PR #19252](https://github.com/dotnet/fsharp/pull/19252))
 * Improve static compilation of state machines. ([PR #19297](https://github.com/dotnet/fsharp/pull/19297))
 * Make Alt+F1 (momentary toggle) work for inlay hints. ([PR #19421](https://github.com/dotnet/fsharp/pull/19421))
+* Fix doubled F# diagnostics in tooltips. ([Issue #16360](https://github.com/dotnet/fsharp/issues/16360))
 
 ### Changed
 

vsintegration/src/FSharp.Editor/Diagnostics/DocumentDiagnosticAnalyzer.fs

@@ -79,8 +79,6 @@ type internal FSharpDocumentDiagnosticAnalyzer [<ImportingConstructor>] () =
 
             let! parseResults = document.GetFSharpParseResultsAsync("GetDiagnostics")
 
-            // Old logic, rollback once https://github.com/dotnet/fsharp/issues/15972 is fixed (likely on Roslyn side, since we're returning diagnostics, but they're not getting to VS).
-            (*
             match diagnosticType with
             | DiagnosticsType.Syntax ->
                 for diagnostic in parseResults.Diagnostics do
@@ -93,23 +91,6 @@ type internal FSharpDocumentDiagnosticAnalyzer [<ImportingConstructor>] () =
                     errors.Add(diagnostic) |> ignore
 
                 errors.ExceptWith(parseResults.Diagnostics)
-            *)
-
-            // TODO: see comment above, this is a workaround for issue we have in current VS/Roslyn
-            match diagnosticType with
-            | DiagnosticsType.Syntax ->
-                for diagnostic in parseResults.Diagnostics do
-                    errors.Add(diagnostic) |> ignore
-
-            // We always add syntactic, and do not exclude them when semantic is requested
-            | DiagnosticsType.Semantic ->
-                for diagnostic in parseResults.Diagnostics do
-                    errors.Add(diagnostic) |> ignore
-
-                let! _, checkResults = document.GetFSharpParseAndCheckResultsAsync("GetDiagnostics")
-
-                for diagnostic in checkResults.Diagnostics do
-                    errors.Add(diagnostic) |> ignore
 
             let! unnecessaryParentheses =
                 match diagnosticType with

vsintegration/tests/FSharp.Editor.Tests/DocumentDiagnosticAnalyzerTests.fs

@@ -29,6 +29,21 @@ type DocumentDiagnosticAnalyzerTests() =
 
         task.Result
 
+    member private _.getSyntaxAndSemantic(fileContents: string) =
+        let task =
+            cancellableTask {
+                let document =
+                    RoslynTestHelpers.CreateSolution(fileContents)
+                    |> RoslynTestHelpers.GetSingleDocument
+
+                let! syntactic = FSharpDocumentDiagnosticAnalyzer.GetDiagnostics(document, DiagnosticsType.Syntax)
+                let! semantic = FSharpDocumentDiagnosticAnalyzer.GetDiagnostics(document, DiagnosticsType.Semantic)
+                return Seq.toArray syntactic, Seq.toArray semantic
+            }
+            |> CancellableTask.start CancellationToken.None
+
+        task.Result
+
     member private this.VerifyNoErrors(fileContents: string, ?additionalFlags: string[]) =
         let errors = this.getDiagnostics (fileContents, ?additionalFlags = additionalFlags)
 
@@ -91,14 +106,13 @@ type DocumentDiagnosticAnalyzerTests() =
     member private this.VerifyDiagnosticBetweenMarkers_HACK_PLEASE_REFER_TO_COMMENT_INSIDE
         (fileContents: string, expectedMessage: string, expectedSeverity: DiagnosticSeverity)
         =
-        // TODO: once workaround (https://github.com/dotnet/fsharp/pull/15982) will not be needed, this should be reverted back to normal method (see PR)
         let errors =
             this.getDiagnostics fileContents
             |> Seq.filter (fun e -> e.Severity = expectedSeverity)
             |> Seq.toArray
 
         errors.Length
-        |> Assert.shouldBeEqualWith 2 "There should be two errors generated"
+        |> Assert.shouldBeEqualWith 1 "There should be exactly one error generated"
 
         let actualError = errors.[0]
         Assert.Equal(expectedSeverity, actualError.Severity)
@@ -117,7 +131,6 @@ type DocumentDiagnosticAnalyzerTests() =
         |> Assert.shouldBeEqualWith expectedEnd "Error end positions should match"
 
     member private this.VerifyErrorBetweenMarkers_HACK_PLEASE_REFER_TO_COMMENT_INSIDE(fileContents: string, expectedMessage: string) =
-        // TODO: once workaround (https://github.com/dotnet/fsharp/pull/15982) will not be needed, this should be reverted back to normal method (see PR)
         this.VerifyDiagnosticBetweenMarkers_HACK_PLEASE_REFER_TO_COMMENT_INSIDE(fileContents, expectedMessage, DiagnosticSeverity.Error)
 
     member private this.VerifyErrorAtMarker_HACK_PLEASE_REFER_TO_COMMENT_INSIDE
@@ -129,7 +142,7 @@ type DocumentDiagnosticAnalyzerTests() =
             |> Seq.toArray
 
         errors.Length
-        |> Assert.shouldBeEqualWith 2 "There should be exactly two errors generated"
+        |> Assert.shouldBeEqualWith 1 "There should be exactly one error generated"
 
         let actualError = errors.[0]
 
@@ -523,3 +536,63 @@ printf "%d" x
             """,
             additionalFlags = [| "--times" |]
         )
+
+    [<Fact>]
+    member this.``Parse diagnostic not duplicated in semantic results``() =
+        let source = "let x =   // incomplete expression - parse error\n"
+        let syntaxDiags, semanticDiags = this.getSyntaxAndSemantic source
+
+        let duplicates =
+            semanticDiags
+            |> Array.filter (fun (d: Diagnostic) ->
+                syntaxDiags
+                |> Array.exists (fun (s: Diagnostic) -> s.Id = d.Id && s.Location.SourceSpan = d.Location.SourceSpan))
+
+        Assert.Empty(duplicates)
+
+    [<Fact>]
+    member this.``Type error appears only in semantic results``() =
+        let source = "let x: int = \"hello\"\n"
+        let _syntaxDiags, semanticDiags = this.getSyntaxAndSemantic source
+        Assert.Contains(semanticDiags, fun (d: Diagnostic) -> d.Id = "FS0001")
+
+    [<Fact>]
+    member this.``Parse errors still reported in syntax pass``() =
+        let source = "let x =\n"
+        let syntaxDiags, _ = this.getSyntaxAndSemantic source
+        Assert.NotEmpty(syntaxDiags)
+
+    [<Fact>]
+    member this.``Clean code has no diagnostics``() =
+        let source = "let x = 42\nlet y = x + 1\n"
+        let syntaxDiags, semanticDiags = this.getSyntaxAndSemantic source
+        Assert.Empty(syntaxDiags)
+        Assert.Empty(semanticDiags)
+
+    [<Fact>]
+    member this.``Multiple parse errors not duplicated``() =
+        let source = "let x =\nlet y =\n"
+        let syntaxDiags, semanticDiags = this.getSyntaxAndSemantic source
+        let allDiags = Array.append syntaxDiags semanticDiags
+
+        let uniqueCount =
+            allDiags
+            |> Array.distinctBy (fun (d: Diagnostic) -> d.Id, d.Location.SourceSpan)
+            |> Array.length
+
+        Assert.Equal(allDiags.Length, uniqueCount)
+
+    [<Fact>]
+    member this.``Warning not duplicated across passes``() =
+        let source = "let x = 42\n"
+        let syntaxDiags, semanticDiags = this.getSyntaxAndSemantic source
+
+        let allWarnings =
+            Array.append syntaxDiags semanticDiags
+            |> Array.filter (fun (d: Diagnostic) -> d.Severity = DiagnosticSeverity.Warning)
+
+        let uniqueWarnings =
+            allWarnings
+            |> Array.distinctBy (fun (d: Diagnostic) -> d.Id, d.Location.SourceSpan)
+
+        Assert.Equal(allWarnings.Length, uniqueWarnings.Length)