From cc725bcb3eda21746540672be98ef4f61eb69dc5 Mon Sep 17 00:00:00 2001 From: Petr Pokorny Date: Thu, 25 Jan 2024 15:10:59 +0100 Subject: [PATCH 1/7] Don't crash graph checking on invalid syntax --- .../GraphChecking/FileContentMapping.fs | 7 ++- .../Graph/FileContentMappingTests.fs | 43 +++++++++++++------ 2 files changed, 34 insertions(+), 16 deletions(-) diff --git a/src/Compiler/Driver/GraphChecking/FileContentMapping.fs b/src/Compiler/Driver/GraphChecking/FileContentMapping.fs index a09b64a3584..0100dd9d745 100644 --- a/src/Compiler/Driver/GraphChecking/FileContentMapping.fs +++ b/src/Compiler/Driver/GraphChecking/FileContentMapping.fs @@ -9,10 +9,9 @@ type Continuations = ((FileContentEntry list -> FileContentEntry list) -> FileCo let collectFromOption (mapping: 'T -> 'U list) (t: 'T option) : 'U list = List.collect mapping (Option.toList t) let longIdentToPath (skipLast: bool) (longId: LongIdent) : LongIdentifier = - if skipLast then - List.take (longId.Length - 1) longId - else - longId + match skipLast, longId with + | true, _::_ -> List.take (longId.Length - 1) longId + | _ -> longId |> List.map (fun ident -> ident.idText) let synLongIdentToPath (skipLast: bool) (synLongIdent: SynLongIdent) = diff --git a/tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/FileContentMappingTests.fs b/tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/FileContentMappingTests.fs index 279bb55e3dc..2a2e6fa41a2 100644 --- a/tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/FileContentMappingTests.fs +++ b/tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/FileContentMappingTests.fs @@ -122,17 +122,36 @@ module B = C | [ TopLevelNamespace "" [ PrefixedIdentifier "C" ] ] -> Assert.Pass() | content -> Assert.Fail($"Unexpected content: {content}") -[] -let ``Invalid nested module should just be ignored`` () = - let content = - getContent - false - """ -module A -module B.C -""" +module We_Shouldnt_Crash_With_Invalid_Syntax = - match content with - | [ TopLevelNamespace "" [] ] -> Assert.Pass() - | content -> Assert.Fail($"Unexpected content: {content}") + [] + let ``Nested module`` () = + let content = + getContent + false + """ + module A + + module B.C + """ + + match content with + | [ TopLevelNamespace "" [] ] -> Assert.Pass() + | content -> Assert.Fail($"Unexpected content: {content}") + + + [] + let ``Module above namespace`` () = + let content = + getContent + false + """ + module + + namespace A.B.C + """ + + match content with + | [ TopLevelNamespace "" [] ] -> Assert.Pass() + | content -> Assert.Fail($"Unexpected content: {content}") From d897fcb39c9e730871b4dc5cd850547264a1064a Mon Sep 17 00:00:00 2001 From: Petr Pokorny Date: Thu, 25 Jan 2024 15:14:38 +0100 Subject: [PATCH 2/7] release note --- docs/release-notes/.FSharp.Compiler.Service/8.0.300.md | 4 ++-- src/Compiler/Driver/GraphChecking/FileContentMapping.fs | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/release-notes/.FSharp.Compiler.Service/8.0.300.md b/docs/release-notes/.FSharp.Compiler.Service/8.0.300.md index 35128d40ad0..0c51859641d 100644 --- a/docs/release-notes/.FSharp.Compiler.Service/8.0.300.md +++ b/docs/release-notes/.FSharp.Compiler.Service/8.0.300.md @@ -2,11 +2,11 @@ * Code generated files with > 64K methods and generated symbols crash when loaded. Use infered sequence points for debugging. ([Issue #16399](https://github.com/dotnet/fsharp/issues/16399), [#PR 16514](https://github.com/dotnet/fsharp/pull/16514)) * `nameof Module` expressions and patterns are processed to link files in `--test:GraphBasedChecking`. ([PR #16550](https://github.com/dotnet/fsharp/pull/16550)) -* Graph Based Checking doesn't throw on invalid parsed input so it can be used for IDE scenarios ([PR #16575](https://github.com/dotnet/fsharp/pull/16575)) +* Graph Based Checking doesn't throw on invalid parsed input so it can be used for IDE scenarios ([PR #16575](https://github.com/dotnet/fsharp/pull/16575), [PR #16588](https://github.com/dotnet/fsharp/pull/16588)) ### Added -* The stackguard depth for ILPdbWriter.unshadowScopes can be modified via the environment variable `FSHARP_ILPdb_UnshadowScopes_StackGuardDepth`([PR #16583](https://github.com/dotnet/fsharp/pull/16583)) +* The stackguard depth for ILPdbWriter.unshadowScopes can be modified via the environment variable `FSHARP_ILPdb_UnshadowScopes_StackGuardDepth`([PR #16583](https://github.com/dotnet/fsharp/pull/16583)) * Parser recovers on complex primary constructor patterns, better tree representation for primary constructor patterns. ([PR #16425](https://github.com/dotnet/fsharp/pull/16425)) * Name resolution: keep type vars in subsequent checks ([PR #16456](https://github.com/dotnet/fsharp/pull/16456)) * Higher-order-function-based API for working with the untyped abstract syntax tree. ([PR #16462](https://github.com/dotnet/fsharp/pull/16462)) diff --git a/src/Compiler/Driver/GraphChecking/FileContentMapping.fs b/src/Compiler/Driver/GraphChecking/FileContentMapping.fs index 0100dd9d745..15355de5a30 100644 --- a/src/Compiler/Driver/GraphChecking/FileContentMapping.fs +++ b/src/Compiler/Driver/GraphChecking/FileContentMapping.fs @@ -10,7 +10,7 @@ let collectFromOption (mapping: 'T -> 'U list) (t: 'T option) : 'U list = List.c let longIdentToPath (skipLast: bool) (longId: LongIdent) : LongIdentifier = match skipLast, longId with - | true, _::_ -> List.take (longId.Length - 1) longId + | true, _ :: _ -> List.take (longId.Length - 1) longId | _ -> longId |> List.map (fun ident -> ident.idText) From df34008fddf6e0313f339d173318df9a3cb7c898 Mon Sep 17 00:00:00 2001 From: Petr Pokorny Date: Thu, 25 Jan 2024 18:29:41 +0100 Subject: [PATCH 3/7] Avoid crash due to cts disposal --- src/Compiler/Driver/GraphChecking/GraphProcessing.fs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/Compiler/Driver/GraphChecking/GraphProcessing.fs b/src/Compiler/Driver/GraphChecking/GraphProcessing.fs index afe491b4b74..cefd4d81500 100644 --- a/src/Compiler/Driver/GraphChecking/GraphProcessing.fs +++ b/src/Compiler/Driver/GraphChecking/GraphProcessing.fs @@ -231,7 +231,11 @@ let processGraphAsync<'Item, 'Result when 'Item: equality and 'Item: comparison> let processedCount = IncrementableInt(0) let raiseExn (item, ex: exn) = - localCts.Cancel() + try + localCts.Cancel() + with :? ObjectDisposedException -> + // If it's disposed already, it means that the processing has already finished, most likely due to cancellation or failure in another node. + () match ex with | :? OperationCanceledException -> completionSignal.TrySetCanceled() From 901b1701f895d55b960a9d841223b9b86100dc3e Mon Sep 17 00:00:00 2001 From: Petr Pokorny Date: Thu, 25 Jan 2024 18:44:39 +0100 Subject: [PATCH 4/7] f --- src/Compiler/Driver/GraphChecking/GraphProcessing.fs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Compiler/Driver/GraphChecking/GraphProcessing.fs b/src/Compiler/Driver/GraphChecking/GraphProcessing.fs index cefd4d81500..92fc009cecf 100644 --- a/src/Compiler/Driver/GraphChecking/GraphProcessing.fs +++ b/src/Compiler/Driver/GraphChecking/GraphProcessing.fs @@ -233,7 +233,7 @@ let processGraphAsync<'Item, 'Result when 'Item: equality and 'Item: comparison> let raiseExn (item, ex: exn) = try localCts.Cancel() - with :? ObjectDisposedException -> + with :? ObjectDisposedException -> // If it's disposed already, it means that the processing has already finished, most likely due to cancellation or failure in another node. () From 50bf08af766d9ec32f52902ddcab0b87a7a52c8b Mon Sep 17 00:00:00 2001 From: Petr Pokorny Date: Thu, 25 Jan 2024 18:58:02 +0100 Subject: [PATCH 5/7] Might as well fix this here --- .../src/FSharp.Editor/LanguageService/WorkspaceExtensions.fs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/vsintegration/src/FSharp.Editor/LanguageService/WorkspaceExtensions.fs b/vsintegration/src/FSharp.Editor/LanguageService/WorkspaceExtensions.fs index c0682607360..6e3de8133f1 100644 --- a/vsintegration/src/FSharp.Editor/LanguageService/WorkspaceExtensions.fs +++ b/vsintegration/src/FSharp.Editor/LanguageService/WorkspaceExtensions.fs @@ -447,7 +447,7 @@ module private CheckerExtensions = ) = cancellableTask { - if document.Project.UseTransparentCompiler then + if checker.UsesTransparentCompiler then return! checker.ParseAndCheckDocumentUsingTransparentCompiler(document, options, userOpName) else let allowStaleResults = From 02994ea49042c047b88e7549b21fef88a0da7f1f Mon Sep 17 00:00:00 2001 From: Petr Pokorny Date: Fri, 26 Jan 2024 13:20:52 +0100 Subject: [PATCH 6/7] rename --- .../TypeChecks/Graph/FileContentMappingTests.fs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/FileContentMappingTests.fs b/tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/FileContentMappingTests.fs index 2a2e6fa41a2..12f171de5fa 100644 --- a/tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/FileContentMappingTests.fs +++ b/tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/FileContentMappingTests.fs @@ -123,7 +123,7 @@ module B = C | content -> Assert.Fail($"Unexpected content: {content}") -module We_Shouldnt_Crash_With_Invalid_Syntax = +module InvalidSyntax = [] let ``Nested module`` () = From bee179cc69ffc86655584dec2a6f41facfd2011d Mon Sep 17 00:00:00 2001 From: Petr Pokorny Date: Fri, 26 Jan 2024 17:15:06 +0100 Subject: [PATCH 7/7] rename --- src/Compiler/Driver/GraphChecking/GraphProcessing.fs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Compiler/Driver/GraphChecking/GraphProcessing.fs b/src/Compiler/Driver/GraphChecking/GraphProcessing.fs index 92fc009cecf..37ecc35041d 100644 --- a/src/Compiler/Driver/GraphChecking/GraphProcessing.fs +++ b/src/Compiler/Driver/GraphChecking/GraphProcessing.fs @@ -230,7 +230,7 @@ let processGraphAsync<'Item, 'Result when 'Item: equality and 'Item: comparison> let processedCount = IncrementableInt(0) - let raiseExn (item, ex: exn) = + let handleExn (item, ex: exn) = try localCts.Cancel() with :? ObjectDisposedException -> @@ -256,7 +256,7 @@ let processGraphAsync<'Item, 'Result when 'Item: equality and 'Item: comparison> match res with | Choice1Of2() -> () - | Choice2Of2 ex -> raiseExn (node.Info.Item, ex) + | Choice2Of2 ex -> handleExn (node.Info.Item, ex) }, cts.Token )