Semantic model should use the first non-empty token of statement to determine binder #16505
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jcouv
force-pushed
the
outvar-semcrash
branch
from
b20f6bb
to
d4ab4ec
Compare
|
@dotnet/roslyn-compiler for review please (RTM bug). |
jcouv
added
New Language Feature - Out Variable Declaration
gafter
reviewed
Jan 17, 2017
| @@ -1172,6 +1172,15 @@ protected int GetAdjustedNodePosition(SyntaxNode node) | |||
|
|
|||
| var fullSpan = this.Root.FullSpan; | |||
| var position = node.SpanStart; | |||
| if (node is StatementSyntax) | |||
There was a problem hiding this comment.
Why is this specific to statements?
There was a problem hiding this comment.
Please either relax this condition (so it applies unconditionally), or file a new issue to relax this in milestone 2.1.
There was a problem hiding this comment.
Filed follow-up issue: #16558
gafter
approved these changes
Jan 17, 2017
This was referenced Jan 17, 2017
|
@dotnet/roslyn-compiler for a second review please (RTM). |
AlekseyTs
approved these changes
Jan 19, 2017
|
@MattGertz for ask mode approval. |
|
Was approved by shiproom. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Customer scenario
While the user is editing, some bad code that includes some declaration expression will cause the semantic model to throw. This causes VS to crash.
In the following example,
GetSymbolInfoonxtriggers binding of the embedded statement, but that would have failed because the wrong binder was used.Bugs this fixes:
Fixes https://devdiv.visualstudio.com/DefaultCollection/DevDiv/_workitems?id=363727
Workarounds, if any
Valid code would not cause this problem. Also, any variant of the code that doesn't cause a missing token to be parsed on the second line (for example, deleting
&&).Risk
The risk is low. The fix was kept tight to minimize risk. Only statements will get this adjustment and only empty tokens are skipped.
Performance impact
Low. The additional computation is negligible.
Is this a regression from a previous update?
No. This was already broken in RC.2 and I think all the way back in June.
Root cause analysis:
The wrong binder was picked because the embedded statement is parsed as a missing token (at the start of the second line), followed by
&&(with the spaces attached as trivia). The various adjustments of the position for finding the binder would result in the closing paren of the first line to be selected. Now, the position of the&&is selected (the syntax with zero-width within a statement is ignored).How was the bug found?
Watson crash.
@dotnet/roslyn-compiler for review.