VS crash. C# code analysis out of stack due to recursion in AsyncAwaitHighlighter.HighlightRelatedKeywords

[tamlin-mike]

Currently running a "home-built" version due to the fix for #39092 not going to get published until at least VS 16.4, but it seems extremely unlikely this condition is yet handled in the shipping code.

While trying to find a bug in a (Microsoft tool-) generated source file, viewing that file provokes this bug and crashes VS.

stacktrace

Microsoft.CodeAnalysis.CSharp.EditorFeatures.dll!Microsoft.CodeAnalysis.Editor.CSharp.KeywordHighlighting.KeywordHighlighters.AsyncAwaitHighlighter.HighlightRelatedKeywords(Microsoft.CodeAnalysis.SyntaxNode node, System.Collections.Generic.List<Microsoft.CodeAnalysis.Text.TextSpan> spans)	C#
Microsoft.CodeAnalysis.CSharp.EditorFeatures.dll!Microsoft.CodeAnalysis.Editor.CSharp.KeywordHighlighting.KeywordHighlighters.AsyncAwaitHighlighter.HighlightRelatedKeywords(Microsoft.CodeAnalysis.SyntaxNode node, System.Collections.Generic.List<Microsoft.CodeAnalysis.Text.TextSpan> spans) Line 108	C#

[... over 3000 recursions removed from display] 	

Microsoft.CodeAnalysis.CSharp.EditorFeatures.dll!Microsoft.CodeAnalysis.Editor.CSharp.KeywordHighlighting.KeywordHighlighters.AsyncAwaitHighlighter.GetHighlightsForNode(Microsoft.CodeAnalysis.SyntaxNode node, System.Threading.CancellationToken cancellationToken) Line 30	C#
Microsoft.CodeAnalysis.EditorFeatures.dll!Microsoft.CodeAnalysis.Editor.Implementation.Highlighting.AbstractKeywordHighlighter.GetHighlights(Microsoft.CodeAnalysis.SyntaxNode root, int position, System.Threading.CancellationToken cancellationToken) Line 34	C#
Microsoft.CodeAnalysis.EditorFeatures.dll!Microsoft.CodeAnalysis.Editor.Implementation.Highlighting.HighlightingService.GetHighlights.AnonymousMethod__1(System.Lazy<Microsoft.CodeAnalysis.Editor.IHighlighter, Microsoft.CodeAnalysis.Host.Mef.LanguageMetadata> h) Line 31	C#
System.Core.dll!System.Linq.Enumerable.WhereSelectListIterator<System.Lazy<Microsoft.CodeAnalysis.Editor.IHighlighter, Microsoft.CodeAnalysis.Host.Mef.LanguageMetadata>, System.Collections.Generic.IEnumerable<Microsoft.CodeAnalysis.Text.TextSpan>>.MoveNext()	Unknown
System.Core.dll!System.Linq.Enumerable.WhereEnumerableIterator<System.Collections.Generic.IEnumerable<Microsoft.CodeAnalysis.Text.TextSpan>>.MoveNext()	Unknown
System.Core.dll!System.Linq.Enumerable.SelectManyIterator<System.Collections.Generic.IEnumerable<Microsoft.CodeAnalysis.Text.TextSpan>, Microsoft.CodeAnalysis.Text.TextSpan>(System.Collections.Generic.IEnumerable<System.Collections.Generic.IEnumerable<Microsoft.CodeAnalysis.Text.TextSpan>> source, System.Func<System.Collections.Generic.IEnumerable<Microsoft.CodeAnalysis.Text.TextSpan>, System.Collections.Generic.IEnumerable<Microsoft.CodeAnalysis.Text.TextSpan>> selector)	Unknown
System.Core.dll!System.Linq.Enumerable.DistinctIterator<Microsoft.CodeAnalysis.Text.TextSpan>(System.Collections.Generic.IEnumerable<Microsoft.CodeAnalysis.Text.TextSpan> source, System.Collections.Generic.IEqualityComparer<Microsoft.CodeAnalysis.Text.TextSpan> comparer)	Unknown
Microsoft.CodeAnalysis.EditorFeatures.dll!Microsoft.CodeAnalysis.Editor.Implementation.Highlighting.HighlighterViewTaggerProvider.ProduceTagsAsync(Microsoft.CodeAnalysis.Editor.Tagging.TaggerContext<Microsoft.CodeAnalysis.Editor.Implementation.Highlighting.KeywordHighlightTag> context, Microsoft.CodeAnalysis.Editor.DocumentSnapshotSpan documentSnapshotSpan, int? caretPosition) Line 103	C#
Microsoft.CodeAnalysis.EditorFeatures.dll!Microsoft.CodeAnalysis.Editor.Tagging.AbstractAsynchronousTaggerProvider<Microsoft.CodeAnalysis.Editor.Implementation.Highlighting.KeywordHighlightTag>.ProduceTagsAsync(Microsoft.CodeAnalysis.Editor.Tagging.TaggerContext<Microsoft.CodeAnalysis.Editor.Implementation.Highlighting.KeywordHighlightTag> context) Line 202	C#
Microsoft.CodeAnalysis.EditorFeatures.dll!Microsoft.CodeAnalysis.Editor.Tagging.AbstractAsynchronousTaggerProvider<Microsoft.CodeAnalysis.Editor.Implementation.Highlighting.KeywordHighlightTag>.TagSource.RecomputeTagsAsync(object oldState, Microsoft.VisualStudio.Text.SnapshotPoint? caretPosition, Microsoft.CodeAnalysis.Text.TextChangeRange? textChangeRange, System.Collections.Immutable.ImmutableArray<Microsoft.CodeAnalysis.Editor.DocumentSnapshotSpan> spansToTag, System.Collections.Immutable.ImmutableDictionary<Microsoft.VisualStudio.Text.ITextBuffer, Microsoft.CodeAnalysis.Editor.Shared.Tagging.TagSpanIntervalTree<Microsoft.CodeAnalysis.Editor.Implementation.Highlighting.KeywordHighlightTag>> oldTagTrees, bool initialTags, System.Threading.CancellationToken cancellationToken) Line 467	C#
Microsoft.CodeAnalysis.EditorFeatures.dll!Microsoft.CodeAnalysis.Editor.Tagging.AbstractAsynchronousTaggerProvider<System.__Canon>.TagSource.RecomputeTagsForeground.AnonymousMethod__0(System.Threading.CancellationToken ct) Line 316	C#
Microsoft.CodeAnalysis.EditorFeatures.dll!Microsoft.CodeAnalysis.Editor.Shared.Threading.AsynchronousSerialWorkQueue.EnqueueBackgroundTask.AnonymousMethod__0(System.Threading.Tasks.Task _) Line 131	C#
mscorlib.dll!System.Threading.Tasks.ContinuationResultTaskFromTask<System.Threading.Tasks.Task>.InnerInvoke() Line 111	C#
mscorlib.dll!System.Threading.Tasks.Task.Execute() Line 2498	C#
mscorlib.dll!System.Threading.Tasks.Task.ExecutionContextCallback(object obj) Line 2861	C#
mscorlib.dll!System.Threading.ExecutionContext.RunInternal(System.Threading.ExecutionContext executionContext, System.Threading.ContextCallback callback, object state, bool preserveSyncCtx) Line 980	C#
mscorlib.dll!System.Threading.ExecutionContext.Run(System.Threading.ExecutionContext executionContext, System.Threading.ContextCallback callback, object state, bool preserveSyncCtx) Line 928	C#
mscorlib.dll!System.Threading.Tasks.Task.ExecuteWithThreadLocal(ref System.Threading.Tasks.Task currentTaskSlot) Line 2827	C#
mscorlib.dll!System.Threading.Tasks.Task.ExecuteEntry(bool bPreventDoubleExecution) Line 2767	C#
mscorlib.dll!System.Threading.Tasks.Task.System.Threading.IThreadPoolWorkItem.ExecuteWorkItem() Line 2704	C#
mscorlib.dll!System.Threading.ThreadPoolWorkQueue.Dispatch() Line 820	C#
mscorlib.dll!System.Threading._ThreadPoolWaitCallback.PerformWaitCallback() Line 1161	C#

If the code has changed considerably since my copy, it's the loop foreach (var child in node.ChildNodes()).

While it could be aborted with a stack probe, perhaps a more sane version would keep track of recursion count and not recurse past... 1k? The over 3k recursions provoking this is evidently too much.

While in the neighbourhood, it could probably be a good idea to move the comment (// Highlight async keyword) at the top of that method to where it actually belongs -- or simply remove it, as it's pretty much pointless.

[sharwell]

This method needs to be converted from recursive to iterative.

[tamlin-mike]

Long-term I agree it should become iterative, but since limiting recursion depth would at least prevent VS from going tits-up I'd consider that fix first priority. I believe 1k would be more than enough. It's not like you can see that much code in a view anyway.

Such a limit could have the additional benefit of reducing the time VS is held in an unusable state by the analyzer, not to mention the fix can be implemented in less than 3 minutes, leaving the recursive->iterative rewrite for a rainy day.

[tamlin-mike]

Another method exhibiting the same-ish problem is Microsoft.CodeAnalysis.CSharp.CSharpSyntaxRewriter.Visit().
_recursionDepth was 2443.

Though this one actually has a stack guard, it ends up in Microsoft.CodeAnalysis.ErrorReporting.FatalError.Report(), where it still ends up as an unhandled exception, and then VS is once again dead - terminated without a trace or indication as to why, unless you actually debug it.

This is starting to look like a larger systemic problem in Roslyn, that it could beneft from a slightly more defensive programming approach; Sanity checks for recursions is a good place to start. I would like to think everyone sane would agree it would be better to gracefully fail an operation, than just charge ahead and crash Visual Studio.

It also displays a larger generic problem in VS - it doesn't have code to handle conditions such as this (i.e. unhandled exceptions). Preferrably it should have a handler, and tell the user the reason for its death; In this case "Thank you for using Microsoft software. You will now lose data. Reason: Stack exhausted. Have a nice day!". Not sure if this is something Roslyn can do, however, or if it needs to be taken to VS platform. If the latter, since I know of no way to contact that team, please forward the issue.

[CyrusNajmabadi]

Long-term I agree it should become iterative, but since limiting recursion depth would at least prevent VS from going tits-up I'd consider that fix first priority.

It's easier to simply make it iterative. I've done so here: #40211

[CyrusNajmabadi]

This is starting to look like a larger systemic problem

It's a systemic issue in .net. There's no generalized way (that I know of at least) to protect against this in .net. So every piece of code needs to be hardened manually.