FailFast due to NullReferenceException when GetCompilationAsync returns null from an F# project

[dsyme]

Description:

See dotnet/fsharp#3499 (comment)

GetCompilationAsync returns null for F# projects. All calls to GetCompilationAsync need to be checked throughout the Roslyn codebase to make sure we are guarding against the possiblity of a null return.

Version Used: master

Steps to Reproduce:

1. Get dsyme/fsharp@c107a16
2. build vs debug
3. open VisualFSharp.sln
4. compile & launch
5. open VisualFSharp.sln in the launched VS using the new bits
6. Turn off Options --> Text Editor --> F# --> Performance --> In-memory project references
7. exit
8. compile & launch
9. open vsintegration\src\FSharp.ProjectSystem.Base\Project\ProjectOptions.cs
10. Go to a symbol with the caret

Expected Behavior:

all ok

Actual Behavior:

FailFast exception due to a NulLReferenceException at this line

[dsyme]

Two comments

• Please, can we avoid the use of FailFast in cases like this? We don't mind if a C# editor feature degrades when something goes slightly wrong via F# project references

• There are a huge number of calls to GetCompilationAsync in Roslyn, 354 of them.... Perhaps we can find a way to make it a stub implementation for F# projects, though I fear there are too many internals to do this....

[CyrusNajmabadi]

Fixed with #21721

[CyrusNajmabadi]

We don't mind if a C# editor feature degrades when something goes slightly wrong via F# project references

A core problem is that we do not know what the issue was that caused the problem. Being 'resilient' or 'gently degrading' is something we've tried in the past with terrible results. What tends to happen is that moments later VS ends up hard crashing due to some down-the-line issue, and we're left scratching our heads wondering what on earth was the root cause.

The project system has been an enormous source of headaches for that exact reason over hte years.

There are a huge number of calls to GetCompilationAsync in Roslyn, 354 of them.... Perhaps we can find a way to make it a stub implementation for F# projects, though I fear there are too many internals to do this....

Roslyn is already highly robust here as TS/JS do not support this API, and we have to be resilient to them. We're just now hitting a few missed cases due to F# having more functionality than TS/JS (i.e. supporting actual P2P refs with C# projects). We'll certainly have a few bugs to shake out there no matter what. With some paired work between the teams i'm certain we can get things in good workign order shortly. I know the IDE team would be happy to work with @brettfo on just iterating rapidly here to squash out all the remaining issues.

[dsyme]

@CyrusNajmabadi Thanks! That all makes sense - though I do wish there was another way besides FailFast (e.g. "Send dump to Microsoft and continue work, disabling this message for rest of session").

[tmat]

@dsyme There is -- Non-Fatal Watson report. We use it pretty extensively: http://source.roslyn.io/#Microsoft.CodeAnalysis.Workspaces/InternalUtilities/FatalError.cs,86d34e79b41227cb,references
@CyrusNajmabadi Have you considered using NFW?

[dsyme]

@tmat Lovely. Your mission, if you choose to accept it, is to replace all FailFast with that, pleeeeease :)

[tmat]

@dsyme I wouldn't say all of them can be replaced. We need to know that we can recover at the point. If some state might be corrupted then it's better to just crash. In many places we work with immutable objects so we can just throw away any calculated state and recover easily. But not everywhere. In general, I think any code that is cancellable is also likely (or should be) recoverable from an unexpected exception, as it already needs to handle cancellation exceptions.