Update SyntaxNodeAnalysisContext.Compilation to not return null#47537
Update SyntaxNodeAnalysisContext.Compilation to not return null#47537sharwell merged 1 commit intodotnet:masterfrom
Conversation
| /// <see cref="CodeAnalysis.Compilation"/> containing the <see cref="SyntaxNode"/>. | ||
| /// </summary> | ||
| public Compilation? Compilation => _semanticModel?.Compilation; | ||
| public Compilation Compilation => _semanticModel?.Compilation ?? throw new InvalidOperationException(); |
There was a problem hiding this comment.
Can _semanticModel ever be null? Can we check and throw an ArgumentNullException in the SyntaxNodeAnalysisContext constructors below?
There was a problem hiding this comment.
_semanticModel is already annotated to not be null from construction. However, this is a value type so the ?. here is used to avoid a NullReferenceException since non-defaultable value types aren't a thing yet.
There was a problem hiding this comment.
Could you clarify why it is more desirable to null-check and throw InvalidOperationException versus not checking and just letting the NullReferenceException happen?
There was a problem hiding this comment.
It could really go either way. Eventually I'd like to see non-defaultable value types cover the scenario. Ideally the places where the explicit check is included would use a generic message for the exception indicating that use of a default instance of this type is not valid. Maybe even a custom exception type derived from this one.
|
@sharwell please make sure to rerun tests before merging to make sure nothing else snuck in. |
No description provided.