Skip to content

Fixed VS2017 crash with :: operator#23946

Merged
sharwell merged 2 commits intodotnet:dev15.6.xfrom
zaytsev-victor:Fixed23940
Jan 23, 2018
Merged

Fixed VS2017 crash with :: operator#23946
sharwell merged 2 commits intodotnet:dev15.6.xfrom
zaytsev-victor:Fixed23940

Conversation

@zaytsev-victor
Copy link
Copy Markdown
Contributor

@zaytsev-victor zaytsev-victor commented Dec 27, 2017

Customer scenario

A customer attempts to use the alias qualification operator :: to refer to a namespace.

Bugs this fixes

Fixes #23940

Workarounds, if any

Avoid using this syntax.

Risk

Low.

Performance impact

Negligible. Adds a type check that is only needed in this case.

Is this a regression from a previous update?

No.

Root cause analysis

No one considered this syntax in testing. @sharwell wasn't even aware it was valid.

How was the bug found?

Reported via developer community.

Test documentation updated?

No.

@zaytsev-victor zaytsev-victor requested a review from a team as a code owner December 27, 2017 14:57

if (node is AliasQualifiedNameSyntax aliasQualifiedName && aliasQualifiedName.Name != null)
{
return aliasQualifiedName.Name;
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.

If this fixes a crash, then it seems like that would be because returning null below causes a problem. Which means the caller of this method shoul dbe updated to be resilient to getting null back, as that's part of the contract of this method.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good point, thanks!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@zaytsev-victor Should we undo this change then, since the change in NameSyntaxClassifier.cs is the right one?

Copy link
Copy Markdown
Contributor Author

@zaytsev-victor zaytsev-victor Jan 3, 2018

Choose a reason for hiding this comment

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

@jasonmalinowski No, we should not undo this change, because this is the fix. Without this change
Col::List would not be recognized that List is the type.

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.

Consider renamign this method to TryGetRightmostName. That will allow us to audit all the callers.

Copy link
Copy Markdown
Member

@jasonmalinowski jasonmalinowski left a comment

Choose a reason for hiding this comment

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

Now that we have the correct fix, we can delete the unnecessary code.

@sharwell sharwell changed the base branch from master to dev15.6.x January 23, 2018 01:24
@sharwell
Copy link
Copy Markdown
Contributor

@Pilchie for ask mode

@Pilchie
Copy link
Copy Markdown
Member

Pilchie commented Jan 23, 2018

Needs passing tests.

@Pilchie
Copy link
Copy Markdown
Member

Pilchie commented Jan 23, 2018

Approved for 15.6 Preview 4 (pending passing tests)

@sharwell sharwell merged commit 11e08c6 into dotnet:dev15.6.x Jan 23, 2018
@sharwell sharwell added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Jan 23, 2018
@sharwell sharwell self-assigned this Jan 23, 2018
@sharwell sharwell added this to the 15.6 milestone Jan 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Approved to merge Community The pull request was submitted by a contributor who is not a Microsoft employee.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants