null check a token's parent to prevent a NullReferenceException

[brettfo]

This enables us to remove a try/catch block that was specifically added to find this NRE.

Fixes #1187.

[brettfo]

The true diff is adding the conditional access on this line token.Parent?.GetAncestorsOrThis and on one line below.

[balajikris]

👍

[basoundr]

👍

[VladimirReshetnikov]

👍

[Pilchie]

Okay, now we know that we are getting asked to trigger signature help on a token with a null parent. Where did that token come from?

[brettfo]

My WinDbg-fu is weak, but from what I can tell from the stack objects here's what I was able to re-create:

1. The C# AttributeSignatureHelpProvider got invoked inside the first UsingDirectiveSyntax in a file, presumably because a '(' or ',' was typed (one of the trigger characters.)
2. This led to AttributeSignatureHelpProvider::GetItemsWorker() -> TryGetAttributeExpression -> CommonSignatureHelpUtilities::TryGetSyntax().
3. ISyntaxFactsService.FindTokenOnLeftOfPosition() defaulted out and returned a CompilationUnitSyntax (see http://source.roslyn.io/#Microsoft.CodeAnalysis.CSharp.Workspaces/Extensions/SyntaxNodeExtensions.cs,802) which has no parent and lead to the NRE.

The only way I could see this happening was if the trigger character was the first non-trivia token in the file, but I could never find a manual repro.

[Pilchie]

I wonder if this happens when the token is in skipped trivia because of some previous error.

[brettfo]

Digging deeper into this with @Pilchie's help we determined that the issue was not a CompilationUnitSyntax that didn't have a parent, but rather syntaxFacts.FindTokenOnLeftOfPosition() returned the first using keyword token in the file and that didn't have a parent (which should have been the using statement), but we weren't able to figure out how that's possible. Then with @jasonmalinowski's help we learned that the crash occurred during typing when he was adding a named parameter to a call to AsyncLazy::StartAsynchronousComputation here, but a manual repro could not be found. Given that we can at least prevent the crash by adding the conditional access operator, I'm going to merge in the changes but leave these notes here for future reference.

[brettfo]

test this please

[jasonmalinowski]

👍

[SLaks]

I have another repro of what looks like the same issue.

Can you please check?

Source: https://github.com/SLaks/DroidMaster/blob/roslyn-crash-1467/DroidMaster/UI/DeviceListViewModel_Commands.cs#L83

Place the cursor at the end of that line, open signature help, then press Ctrl+Home.

[brettfo]

@SLaks yes, this is the same issue. Thanks for finding a manual repro for this! I couldn't find one earlier so I was fixing this blindly.

This repro looks like a different entry point than I originally encountered, but it's interesting nonetheless in that it causes the same null parent. In this case, signature help is open, Ctrl+Home is typed which moves the cursor to the top of the file, then syntaxFacts.FindToken.OnLeftOfPosition() is called with its position parameter to 0 which returns SyntaxToken.None which doesn't have a parent and leads to the null ref fixed by this PR. Examining the call stack, it looks like we're doing the correct thing by letting the added conditional access expression cause TryGetSyntax() to fail, because otherwise it feels messy to explicitly look for SyntaxToken.None and special-case it here.

tl;dr - with these changes, TryGetSyntax() is doing the correct thing and simply not finding the requested node in an error case.