Fix a NullReferenceException in DocumentExtensions

[aelij]

I encountered this while using the Classifier. This happens if you specify a span outside the the document. I wasn't sure if it should return the entire document's semantic model or throw an ArgumentOutOfRangeException; I chose the former.

Stack trace:

at Microsoft.CodeAnalysis.CSharp.CSharpSyntaxFactsService.GetMemberBodySpanForSpeculativeBinding(SyntaxNode node)
at async Microsoft.CodeAnalysis.Shared.Extensions.DocumentExtensions.GetSemanticModelForNodeAsync(ISemanticModelService semanticModelService, ISyntaxFactsService syntaxFactService, Document document, SyntaxNode node, TextSpan span, CancellationToken cancellationToken)
at async Microsoft.CodeAnalysis.Shared.Extensions.DocumentExtensions.GetSemanticModelForSpanAsync(?)
at async Microsoft.CodeAnalysis.Classification.Classifier.GetClassifiedSpansAsync(?)

[Pilchie]

Hi @aelij,

Thanks for the submission! However, I think I personally would prefer to have this throw. It's generally a programming error to ask for a span outside the span of the document. Did you run into this with a regular file in Visual Studio, or did was it when you were using the API?

Anyone else on @dotnet/roslyn-ide want to comment? @mattwar?

[Pilchie]

Style Nit: We generally prefer a blank line after a }.

[jasonmalinowski]

Agreed -- this should throw in that case. (I wonder what bugs we'll uncover with that!)

[aelij]

Is throw new ArgumentOutOfRangeException(nameof(span)) enough or should I add some message?

@Pilchie I ran into this while using the API to hook up syntax highlighting in AvalonEdit.

[CyrusNajmabadi]

Hrmmm... how could node be null? When would we have a span that wasn't contained in the full span of some node in the document?

[CyrusNajmabadi]

I would "throw an ArgumentOutOfRangeException". The span is not within bounds. There is something very wrong with the caller.

[aelij]

In case it got missed - I pushed the requested changes to my branch.

[davkean]

@dotnet/roslyn-ide Can you take another look at it?

[jasonmalinowski]

Seems more reasonable, should we just do if (!root.FullSpan.Contains(span)) earlier and throw instead? This seems to be a fairly circuitous way to check the simpler case.

[DustinCampbell]

@Pilchie, @CyrusNajmabadi, @jasonmalinowski: What's the status on this PR?

[jasonmalinowski]

I guess we are waiting for @aelij to respond to my suggestion?

[aelij]

Oh, I was waiting for something actionable... :)

I'd leave it as is, since obviously this isn't a common code path, so it's a bit of a waste to check the span's range on every call.

[jaredpar]

We apologize, but we are closing this PR due to code drift. We regret letting this PR go so long without attention, but at this point the code base has changed too much for us to revisit older PRs. Going forward, we will manage PRs better under an SLA currently under draft in issue #26266 – we encourage you to add comments to that draft as you see fit. Although that SLA is still in draft form, we nevertheless want to move forward with the identification of older PRs to give contributors as much early notice as possible to review and update them.

If you are interested in pursuing this PR, please reset it against the head of master and we will reopen it. Thank you!