From 5af7fade94f7a1e88f7fc5c4e4bbfd007c94cf74 Mon Sep 17 00:00:00 2001 From: Vlad Zarytovskii Date: Fri, 15 Sep 2023 15:26:24 +0200 Subject: [PATCH 1/3] Workaround for #15972 --- .../Diagnostics/DocumentDiagnosticAnalyzer.fs | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/vsintegration/src/FSharp.Editor/Diagnostics/DocumentDiagnosticAnalyzer.fs b/vsintegration/src/FSharp.Editor/Diagnostics/DocumentDiagnosticAnalyzer.fs index 1e547e86335..4329596ea49 100644 --- a/vsintegration/src/FSharp.Editor/Diagnostics/DocumentDiagnosticAnalyzer.fs +++ b/vsintegration/src/FSharp.Editor/Diagnostics/DocumentDiagnosticAnalyzer.fs @@ -76,6 +76,8 @@ type internal FSharpDocumentDiagnosticAnalyzer [] () = 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 @@ -88,6 +90,23 @@ type internal FSharpDocumentDiagnosticAnalyzer [] () = 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 if errors.Count = 0 then return ImmutableArray.Empty From aaf06f3ebbad04c5fe5e78d066fcc0c4e6ed3932 Mon Sep 17 00:00:00 2001 From: Vlad Zarytovskii Date: Fri, 15 Sep 2023 16:59:17 +0200 Subject: [PATCH 2/3] Ok, I'm not proud of these tests workarounds but at least they will fail once we rollback the change, but forget to rollback tests --- .../DocumentDiagnosticAnalyzerTests.fs | 79 ++++++++++++++++--- 1 file changed, 70 insertions(+), 9 deletions(-) diff --git a/vsintegration/tests/FSharp.Editor.Tests/DocumentDiagnosticAnalyzerTests.fs b/vsintegration/tests/FSharp.Editor.Tests/DocumentDiagnosticAnalyzerTests.fs index 8f8dbd37189..990ca2e9927 100644 --- a/vsintegration/tests/FSharp.Editor.Tests/DocumentDiagnosticAnalyzerTests.fs +++ b/vsintegration/tests/FSharp.Editor.Tests/DocumentDiagnosticAnalyzerTests.fs @@ -91,6 +91,67 @@ type DocumentDiagnosticAnalyzerTests() = actualError.Location.SourceSpan.End |> Assert.shouldBeEqualWith expectedEnd "Error end positions should match" + 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 = + getDiagnostics fileContents + |> Seq.filter (fun e -> e.Severity = expectedSeverity) + |> Seq.toArray + + errors.Length + |> Assert.shouldBeEqualWith 2 "There should be two errors generated" + + let actualError = errors.[0] + Assert.Equal(expectedSeverity, actualError.Severity) + + actualError.GetMessage() + |> Assert.shouldBeEqualWith expectedMessage "Error messages should match" + + let expectedStart = fileContents.IndexOf(startMarker) + startMarker.Length + + actualError.Location.SourceSpan.Start + |> Assert.shouldBeEqualWith expectedStart "Error start positions should match" + + let expectedEnd = fileContents.IndexOf(endMarker) + + actualError.Location.SourceSpan.End + |> 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(fileContents: string, expectedMarker: string, ?expectedMessage: string) = + let errors = + getDiagnostics fileContents + |> Seq.filter (fun e -> e.Severity = DiagnosticSeverity.Error) + |> Seq.toArray + + errors.Length + |> Assert.shouldBeEqualWith 2 "There should be exactly two errors generated" + + let actualError = errors.[0] + + if expectedMessage.IsSome then + actualError.GetMessage() + |> Assert.shouldBeEqualWith expectedMessage.Value "Error messages should match" + + Assert.Equal(DiagnosticSeverity.Error, actualError.Severity) + let expectedStart = fileContents.IndexOf(expectedMarker) + + actualError.Location.SourceSpan.Start + |> Assert.shouldBeEqualWith expectedStart "Error start positions should match" + + let expectedEnd = expectedStart + expectedMarker.Length + + actualError.Location.SourceSpan.End + |> Assert.shouldBeEqualWith expectedEnd "Error end positions should match" + member private this.VerifyErrorBetweenMarkers(fileContents: string, expectedMessage: string) = this.VerifyDiagnosticBetweenMarkers(fileContents, expectedMessage, DiagnosticSeverity.Error) @@ -99,7 +160,7 @@ type DocumentDiagnosticAnalyzerTests() = [] member public this.Error_Expression_IllegalIntegerLiteral() = - this.VerifyErrorBetweenMarkers( + this.VerifyErrorBetweenMarkers_HACK_PLEASE_REFER_TO_COMMENT_INSIDE( fileContents = """ let _ = 1 @@ -110,7 +171,7 @@ let a = 0.1(*start*).(*end*)0 [] member public this.Error_Expression_IncompleteDefine() = - this.VerifyErrorBetweenMarkers( + this.VerifyErrorBetweenMarkers_HACK_PLEASE_REFER_TO_COMMENT_INSIDE( fileContents = """ let a = (*start*);(*end*) @@ -120,7 +181,7 @@ let a = (*start*);(*end*) [] member public this.Error_Expression_KeywordAsValue() = - this.VerifyErrorBetweenMarkers( + this.VerifyErrorBetweenMarkers_HACK_PLEASE_REFER_TO_COMMENT_INSIDE( fileContents = """ let b = @@ -255,7 +316,7 @@ let f () = [] member public this.Error_Identifer_IllegalFloatPointLiteral() = - this.VerifyErrorBetweenMarkers( + this.VerifyErrorBetweenMarkers_HACK_PLEASE_REFER_TO_COMMENT_INSIDE( fileContents = """ let x: float = 1.2(*start*).(*end*)3 @@ -368,7 +429,7 @@ async { if true then return 1 } |> ignore [] member public this.ExtraEndif() = - this.VerifyErrorBetweenMarkers( + this.VerifyErrorBetweenMarkers_HACK_PLEASE_REFER_TO_COMMENT_INSIDE( fileContents = """ #if UNDEFINED @@ -383,7 +444,7 @@ async { if true then return 1 } |> ignore [] member public this.Squiggles_HashNotFirstSymbol_If() = - this.VerifyErrorAtMarker( + this.VerifyErrorAtMarker_HACK_PLEASE_REFER_TO_COMMENT_INSIDE( fileContents = """ (*comment*) #if UNDEFINED @@ -398,7 +459,7 @@ async { if true then return 1 } |> ignore [] member public this.Squiggles_HashNotFirstSymbol_Endif() = - this.VerifyErrorAtMarker( + this.VerifyErrorAtMarker_HACK_PLEASE_REFER_TO_COMMENT_INSIDE( fileContents = """ #if DEBUG @@ -413,7 +474,7 @@ async { if true then return 1 } |> ignore [] member public this.Squiggles_HashIfWithMultilineComment() = - this.VerifyErrorAtMarker( + this.VerifyErrorAtMarker_HACK_PLEASE_REFER_TO_COMMENT_INSIDE( fileContents = """ #if DEBUG (*comment*) @@ -425,7 +486,7 @@ async { if true then return 1 } |> ignore [] member public this.Squiggles_HashIfWithUnexpected() = - this.VerifyErrorAtMarker( + this.VerifyErrorAtMarker_HACK_PLEASE_REFER_TO_COMMENT_INSIDE( fileContents = """ #if DEBUG TEST From a77ea17ccd3e6ff62a8ac54cf90e56be1558f24c Mon Sep 17 00:00:00 2001 From: Vlad Zarytovskii Date: Fri, 15 Sep 2023 18:31:13 +0200 Subject: [PATCH 3/3] sigh, fantomas --- .../FSharp.Editor.Tests/DocumentDiagnosticAnalyzerTests.fs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/vsintegration/tests/FSharp.Editor.Tests/DocumentDiagnosticAnalyzerTests.fs b/vsintegration/tests/FSharp.Editor.Tests/DocumentDiagnosticAnalyzerTests.fs index 990ca2e9927..cf05a66cdd6 100644 --- a/vsintegration/tests/FSharp.Editor.Tests/DocumentDiagnosticAnalyzerTests.fs +++ b/vsintegration/tests/FSharp.Editor.Tests/DocumentDiagnosticAnalyzerTests.fs @@ -126,7 +126,12 @@ type DocumentDiagnosticAnalyzerTests() = // 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(fileContents: string, expectedMarker: string, ?expectedMessage: string) = + member private this.VerifyErrorAtMarker_HACK_PLEASE_REFER_TO_COMMENT_INSIDE + ( + fileContents: string, + expectedMarker: string, + ?expectedMessage: string + ) = let errors = getDiagnostics fileContents |> Seq.filter (fun e -> e.Severity = DiagnosticSeverity.Error)