Skip to content

Commit

Permalink
Don't crash graph checking on invalid syntax (#16588)
Browse files Browse the repository at this point in the history
* Don't crash graph checking on invalid syntax

* release note

* Avoid crash due to cts disposal

* f

* Might as well fix this here

* rename
  • Loading branch information
0101 authored Jan 29, 2024
1 parent dcf28e8 commit b3a7e29
Show file tree
Hide file tree
Showing 5 changed files with 44 additions and 22 deletions.
4 changes: 2 additions & 2 deletions docs/release-notes/.FSharp.Compiler.Service/8.0.300.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,12 @@

* 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))
* Keep parens for problematic exprs (`if`, `match`, etc.) in `$"{(…):N0}"`, `$"{(…),-3}"`, etc. ([PR #16578](https://github.com/dotnet/fsharp/pull/16578))

### 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))
Expand Down
7 changes: 3 additions & 4 deletions src/Compiler/Driver/GraphChecking/FileContentMapping.fs
Original file line number Diff line number Diff line change
Expand Up @@ -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) =
Expand Down
10 changes: 7 additions & 3 deletions src/Compiler/Driver/GraphChecking/GraphProcessing.fs
Original file line number Diff line number Diff line change
Expand Up @@ -230,8 +230,12 @@ let processGraphAsync<'Item, 'Result when 'Item: equality and 'Item: comparison>

let processedCount = IncrementableInt(0)

let raiseExn (item, ex: exn) =
localCts.Cancel()
let handleExn (item, ex: exn) =
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()
Expand All @@ -252,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
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -122,17 +122,36 @@ module B = C
| [ TopLevelNamespace "" [ PrefixedIdentifier "C" ] ] -> Assert.Pass()
| content -> Assert.Fail($"Unexpected content: {content}")

[<Test>]
let ``Invalid nested module should just be ignored`` () =
let content =
getContent
false
"""
module A

module B.C
"""
module InvalidSyntax =

match content with
| [ TopLevelNamespace "" [] ] -> Assert.Pass()
| content -> Assert.Fail($"Unexpected content: {content}")
[<Test>]
let ``Nested module`` () =
let content =
getContent
false
"""
module A
module B.C
"""

match content with
| [ TopLevelNamespace "" [] ] -> Assert.Pass()
| content -> Assert.Fail($"Unexpected content: {content}")


[<Test>]
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}")
Original file line number Diff line number Diff line change
Expand Up @@ -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 =
Expand Down

0 comments on commit b3a7e29

Please sign in to comment.