Remove cancellation token from NavigateTo#332
Conversation
NavigateTo doesn't support cancellation when computing the display properties so the cancellation token is getting removed rather than having to manually handling OperationCanceledException.
|
LGTM |
|
👍 |
There was a problem hiding this comment.
You might want to add a comment why CancellationToken.None is being used.
|
👍 ✨ |
There was a problem hiding this comment.
Is there a known way that Symbol can be null here? I don't see how this is related to the rest of the change.
There was a problem hiding this comment.
Ultimately, we're calling through SemanticModel.GetDeclaredSymbolForNode() for which null is a valid return value. This null check isn't directly related to cancellation, but I just spotted a potential future NRE when checking all references to DeclaredSymbolNavigableItem.Symbol that I wanted to prevent (especially since we're guarding against another NRE before descending into SummaryText).
There was a problem hiding this comment.
In that case, should we have never created a SearchResult object in the first place?
There was a problem hiding this comment.
Ideally, you're right, but since NavigateTo resolves the symbol as late as possible for perf reasons (i.e., my recent NavigateTo changes), we don't have the symbol (or null) until it's too late. Realizing the symbol earlier in the process would knock our perf back down to what it was before.
Remove cancellation token from NavigateTo
theoy
added
Concept-API
7 participants
NavigateTo doesn't support cancellation when computing the display properties so the cancellation token is getting removed rather than having to manually handle an OperationCanceledException.