Conversation
|
@dotnet/roslyn-compiler PTAL |
There was a problem hiding this comment.
should this use RequiredUnderlyingNode?
There was a problem hiding this comment.
or is this actually a situation where we just want the compiler to accept it, we don't want to assert about it in debug builds?
There was a problem hiding this comment.
The use of UnderlyingNode! is deliberate here. The rational in the Nullable Annotation Guidance document is roughly:
- Use
!when the method already verifies a value is non-null but the compiler just can't follow along - Use
Debug.Assert, or more formal methods, when the method doesn't do the verification.
In this case the switch statement effectively verifies that the repsective nodes are non-null. Essentially _nodes will have a non-null value for whatever the _count of the list is. Hence the method already verifies so use !.
e8937a9 to
a9caef3
Compare
|
|
||
| case SyntaxKind.SkippedTokensTrivia: | ||
| ClassifySkippedTokens((SkippedTokensTriviaSyntax)trivia.GetStructure()); | ||
| ClassifySkippedTokens((SkippedTokensTriviaSyntax)trivia.GetStructure()!); |
There was a problem hiding this comment.
How do we know this suppression is safe from surrounding code? Should this instead be an assertion?
There was a problem hiding this comment.
This is another really sutbtle one. Essentially we can't get SyntaxKind.SkippedTokensTrivia unless trivia.UnderlyingNode is a non-null value. This particular node is by definition a structured one hence it will always be non-null if there is a non-null UnderlyingNode value.
I have no idea how to handle this though. There really isn't anything I can Debug.Assert here because it's a method return, not a property. I could add a new member to StructuredTriviaSyntax but that would be new public API surface. So far haven't added such a member for nullability yet (doesn't mean we shouldn't though).
| var list = this.ToList(); | ||
| list.RemoveAt(index); | ||
| return new SyntaxTriviaList(default(SyntaxToken), Node.CreateList(list.Select(n => n.UnderlyingNode)), 0, 0); | ||
| return new SyntaxTriviaList(default(SyntaxToken), Node!.CreateList(list.Select(n => n.RequiredUnderlyingNode)), 0, 0); |
There was a problem hiding this comment.
Node! [](start = 62, length = 5)
nit: Should this be an assertion? (ditto below)
There was a problem hiding this comment.
The code does validate that Node is not null but it's really subtle. Basically for the code to reach this point Count must be greater than zero. In that case Node is guaranteed to be non-null.
I'm open to how to make this clearer: either by using Debug.Assert or by adding a comment. I agree it's really subtle.
| public SyntaxNode? GetStructure() | ||
| { | ||
| return HasStructure ? UnderlyingNode.GetStructure(this) : null; | ||
| return HasStructure ? UnderlyingNode!.GetStructure(this) : null; |
There was a problem hiding this comment.
UnderlyingNode! [](start = 34, length = 15)
nit: Consider adding a comment to mark HasStructure with MemberNotNull(nameof(UnderlyingNode)) once we have the capability
There was a problem hiding this comment.
Good idea. Filed a general issue we can use to track all places like this.
jcouv
left a comment
There was a problem hiding this comment.
LGTM Thanks (iteration 7) with a couple nits/questions
|
@jaredpar This looks ready to merge. Thanks |
No description provided.