Fix GetNext/PreviousDirective's handling of duplicate trivia#62760
Conversation
80a86ce to
51b6b69
Compare
| "); | ||
| var c = tree.GetCompilationUnitRoot().Members[0]; | ||
|
|
||
| // Duplicate the leading trivia on the class |
|
Done with review pass (commit 1) |
GetNext/PreviousDirective work by walking the list of trivia and when they see the SyntaxTrivia that represents 'this', they set a flag and then continue to traverse and return the next directive found. There was a bug though in handling of duplicate trivia: if somebody were to create a node that has the same trivia twice, that may share the underlying green node. The boolean flag of "return next directive" was set if the green node matched, but no checking was done of the span or index to verify it was the correct instance of the green node. This meant that somebody calling GetNextDirective on the second directive would get the second directive back again, since internally the loop would see the first directive, go "oh, that's me!" and return the second one. Fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1568850, or at least the fact it causes crashes. An IDE bug is causing us to accidentally introduce duplicate trivia, but in the process it's causing compiler functions that depend on GetNext/PreviousDirective to end up in a stack overflow, because they assume progress will be made by these functions.
51b6b69 to
b551bad
Compare
|
@AlekseyTs Feedback addressed. And generally to all, I rebased this against 17.3 since we'll be targeting that there; my apologies if that screws up any review tooling. |
| if (tr.IsDirective) | ||
| { | ||
| if (tr.IsDirective) | ||
| var d = (DirectiveTriviaSyntax)tr.GetStructure()!; |
There was a problem hiding this comment.
@AlekseyTs suggested setting the "next" parameter by also doing an object equality check against the structure; at this point both branches were going to read this.
There was a problem hiding this comment.
The second branch only needs the structure when we already think we have the previous token.
Is it worth using the original if cases and checking (object)tr.GetStructure() == this in the second branch, to avoid unnecessary calls to GetStructure()?
There was a problem hiding this comment.
@cston I'd imagine that'd only be happening if you have multiple directives in the same block of trivia, which would be a very tiny amount of extra work. I'd aim for simpler code here.
There was a problem hiding this comment.
that'd only be happening if you have multiple directives in the same block of trivia
Thanks, I'd missed that.
GetNext/PreviousDirective work by walking the list of trivia and when they see the SyntaxTrivia that represents 'this', they set a flag and then continue to traverse and return the next directive found. There was a bug though in handling of duplicate trivia: if somebody were to create a node that has the same trivia twice, that may share the underlying green node. The boolean flag of "return next directive" was set if the green node matched, but no checking was done of the span or index to verify it was the correct instance of the green node. This meant that somebody calling GetNextDirective on the second directive would get the second directive back again, since internally the loop would see the first directive, go "oh, that's me!" and return the second one.
Fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1568850, or at least the fact it causes crashes. An IDE bug is causing us to accidentally introduce duplicate trivia, but in the process it's causing compiler functions that depend on GetNext/PreviousDirective to end up in a stack overflow, because they assume progress will be made by these functions.