Avoid crash with 'new ref[]' by jcouv · Pull Request #25505 · dotnet/roslyn

jcouv · 2018-03-15T21:13:47Z

new ref[] is parsed as an object creation where the type is a RefTypeSyntax whose type is ?[] (an array with missing type).
From discussion with @VSadov, trying to fix this in the parser would introduce context, so a binding fix is preferable, even if the parsing diagnostic isn't good.

Customer scenario

Type new ref[] to crash the compiler and VS.

Bugs this fixes

Fixes #25264

Risk

Performance impact

Is this a regression from a previous update?

No

Root cause analysis

How did we miss it? What tests are we adding to guard against it in the future?

How was the bug found?

Reported by customer.

AlekseyTs · 2018-03-16T17:48:23Z

return BadExpression(node, LookupResultKind.NotCreatable); [](start = 20, length = 58)

Instead of bailing out like that, we should continue binding for SemanticModel and IOperation benefits. For example, we might go into BindClassCreationExpression (because an array is a class) and handle the array type there specially, if that is even required (we might just get an acceptable error about missing constructor, etc.) #Closed

See how pointers are handled, the same approach might work here.

In reply to: 175167784 [](ancestors = 175167784)

BindClassCreationExpression ends up returning a bad expression anyways (line 4683) :-(
I can do the change here, but it's not going to help with semantic model or IOperation.
If we want to eliminate BadExpression, it's probably best to do as a separate effort.

BindClassCreationExpression ends up returning a bad expression anyways (line 4683) :-(

The goal is not to avoid producing bad expression, the goal is to not drop constituent pieces and it looks like the bad expression produced at line 4683 doesn't drop them. So they will be available to SemanticModel and IOperation. Tests should confirm that

In reply to: 175974841 [](ancestors = 175974841)

AlekseyTs · 2018-03-16T17:52:17Z

TestNewRefArray [](start = 20, length = 15)

We should test SemanticModel and IOperation, including scenarios when there is an object initializer and a collection initializer. #Closed

AlekseyTs · 2018-03-16T17:52:56Z

Done with review pass (iteration 1) #Closed

AlekseyTs · 2018-03-16T17:55:08Z

                return BadExpression(node, LookupResultKind.NotCreatable);

While we are here, we might want to fix this place too. #Closed

Refers to: src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs:3577 in d5dac0b. [](commit_id = d5dac0b55b5e61692c08c32c0e2dbf9fdb4ca435, deletion_comment = False)

jcouv · 2018-03-23T23:38:24Z

@AlekseyTs Please take another look when you get the chance. The binding now keeps children bound nodes in error case. Thanks

AlekseyTs

LGTM (iteration 3)

jcouv · 2018-03-27T16:14:57Z

@dotnet/roslyn-compiler for a second review. Thanks
Note: I rebased the changes and resolved conflicts (error code and resx file) after Aleksey's review.

jaredpar · 2018-03-27T16:55:17Z

@@ -1575,6 +1575,7 @@ internal enum ErrorCode
        WRN_TupleBinopLiteralNameMismatch = 8383,
        ERR_TupleSizesMismatchForBinOps = 8384,
        ERR_ExprCannotBeFixed = 9385,


9385, [](start = 32, length = 5)

Not your PR but this looks wrong.

I will file a bug.

In reply to: 177497359 [](ancestors = 177497359)

#25752

In reply to: 177497404 [](ancestors = 177497404,177497359)

I'll go ahead and fix in this PR.

In reply to: 177497759 [](ancestors = 177497759,177497404,177497359)

jaredpar

cston · 2018-03-27T16:56:50Z

@@ -1575,6 +1575,7 @@ internal enum ErrorCode
        WRN_TupleBinopLiteralNameMismatch = 8383,
        ERR_TupleSizesMismatchForBinOps = 8384,
        ERR_ExprCannotBeFixed = 9385,


Should this be 8385?

Good eye. Fixed

jaredpar · 2018-03-27T16:57:06Z

approved

cston · 2018-03-27T17:07:05Z

        WRN_TupleBinopLiteralNameMismatch = 8383,
        ERR_TupleSizesMismatchForBinOps = 8384,
-        ERR_ExprCannotBeFixed = 9385,
+        ERR_ExprCannotBeFixed = 8385,


Are there no references to CS9385 in tests?

I don't typically update all the textual references in VerifyDiagnostics (the comments). Only the error enum entry is really checked (for C# at least).

jcouv · 2018-03-27T18:09:40Z

test windows_debug_unit32_prtest please

jcouv · 2018-03-27T18:09:46Z

test windows_debug_unit64_prtest please

alrz · 2018-03-27T18:17:30Z

PDBUsingTests doesn't seem to be flaky. probably the baseline need to be updated. I see those failures in all of my recent PRs targeting dev15.7.x.

jcouv · 2018-03-27T18:21:07Z

@alrz See discussion in #25753

…features/dataflow * dotnet/dev15.7.x: Fix inference when tuple names differ (dotnet#25738) Avoid crash with 'new ref[]' (dotnet#25505) Skip some PDBUsingTests tests to unblock CI (dotnet#25753) Disable Spanish queue Tuple equality: Optimize cases where nullable always has value (dotnet#25644) Bump prerelease version to beta4 Fix failure to initialize RemoteWorkspace in the out-of-process host remove whitespace Removing package dependency for Microsoft.VisualStudio.Text.UI.Wpf in EditorFeatures Removing package dependency for Microsoft.VisualStudio.Text.UI.Wpf in CSharpEditorFeatures

jcouv added the Area-Compilers label Mar 15, 2018

jcouv added this to the 15.7 milestone Mar 15, 2018

jcouv self-assigned this Mar 15, 2018

jcouv requested a review from a team as a code owner March 15, 2018 21:13

AlekseyTs reviewed Mar 16, 2018

View reviewed changes

jcouv added PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. and removed PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. labels Mar 16, 2018

AlekseyTs approved these changes Mar 24, 2018

View reviewed changes

jcouv force-pushed the ref-crash branch from 91703c9 to a2d5263 Compare March 24, 2018 22:31

jcouv and others added 4 commits March 27, 2018 09:08

Avoid crash with 'new ref[]'

f1769de

Address PR feedback from Aleksey

4b380e5

Fix test regressions

a877c5c

Resolve merge conflict

65d0466

jcouv force-pushed the ref-crash branch from a2d5263 to 65d0466 Compare March 27, 2018 16:13

jaredpar reviewed Mar 27, 2018

View reviewed changes

jaredpar approved these changes Mar 27, 2018

View reviewed changes

cston reviewed Mar 27, 2018

View reviewed changes

jaredpar added the Approved to merge label Mar 27, 2018

jcouv mentioned this pull request Mar 27, 2018

ERR_ExprCannotBeFixed has wrong error code #25752

Closed

Fix error code 9385 to 8385

2c99bf3

cston approved these changes Mar 27, 2018

View reviewed changes

cston reviewed Mar 27, 2018

View reviewed changes

jcouv merged commit f697953 into dotnet:dev15.7.x Mar 27, 2018

jcouv deleted the ref-crash branch March 27, 2018 19:22

Conversation

jcouv commented Mar 15, 2018

Customer scenario

Bugs this fixes

Risk

Performance impact

Is this a regression from a previous update?

Root cause analysis

How was the bug found?

Uh oh!

AlekseyTs Mar 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Mar 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Mar 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs commented Mar 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcouv commented Mar 23, 2018

Uh oh!

AlekseyTs left a comment

Choose a reason for hiding this comment

Uh oh!

jcouv commented Mar 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaredpar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaredpar commented Mar 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv commented Mar 27, 2018

Uh oh!

jcouv commented Mar 27, 2018

Uh oh!

alrz commented Mar 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcouv commented Mar 27, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

AlekseyTs Mar 16, 2018 •

edited

Loading

AlekseyTs Mar 16, 2018 •

edited

Loading

AlekseyTs commented Mar 16, 2018 •

edited

Loading

AlekseyTs commented Mar 16, 2018 •

edited

Loading

alrz commented Mar 27, 2018 •

edited

Loading