Skip to content

Fix crash in GetDeconstructionInfo#27605

Merged
jcouv merged 1 commit intodotnet:masterfrom
jcouv:decon-crash
Jun 8, 2018
Merged

Fix crash in GetDeconstructionInfo#27605
jcouv merged 1 commit intodotnet:masterfrom
jcouv:decon-crash

Conversation

@jcouv
Copy link
Copy Markdown
Member

@jcouv jcouv commented Jun 8, 2018

Customer scenario

Type an incomplete foreach (char in s) { } and hover your mouse over the in. The IDE will pop-up an exception dialog or crash.

Bugs this fixes

Fixes #27520
Fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/628435

Risk

Performance impact

Low. This fix is just adding a null check.

Is this a regression from a previous update?

Yes, this is a regression from 15.7.

Root cause analysis

Although the API (including this underlying bug) was introduced in 15.6, it was only used for Find All References. But in 15.8, I made a change to use this API in QuickInfo, which is much more common, especially on incomplete code.

How was the bug found?

Watson and bug report from a team member

@jcouv jcouv added this to the 15.8 milestone Jun 8, 2018
@jcouv jcouv self-assigned this Jun 8, 2018
@jcouv jcouv requested a review from a team as a code owner June 8, 2018 00:04
{
static void M(string s)
{
foreach (char in s) { }
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

foreach (char in s) { } [](start = 8, length = 23)

I am curious why we are parsing this as ForEachVariableStatement

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the code (without stepping through it to confirm), here's what I think happens:

  • we reach the open paren
  • try to parse a declaration expression (ParseExpressionOrDeclaration)
  • but IsPossibleDeclarationExpression scans ahead of the type to check for a designation (missing, keyword in is found)
  • having not found a declaration expression, we go down the route of interpreting the foreach as not-a-simple-foreach, but rather a deconstruction foreach.

Copy link
Copy Markdown
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (iteration 1)

@jcouv
Copy link
Copy Markdown
Member Author

jcouv commented Jun 8, 2018

Marking as approved to merge for 15.8 preview 4.
FYI @jaredpar

@jcouv jcouv merged commit da60463 into dotnet:master Jun 8, 2018
@jcouv jcouv deleted the decon-crash branch June 8, 2018 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Visual Studio crashed with NullReference incomplete foreach

3 participants