Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Parse array type in conditional operator #31050

Merged
merged 2 commits into from
Nov 13, 2018

Conversation

cston
Copy link
Member

@cston cston commented Nov 8, 2018

Customer scenario

Parse error reported for array types used in conditional operator expression: x is T[] ? y : z

Bugs this fixes

Internal issue

Workarounds, if any

Modify code. For instance, add parentheses: (x is T[]) ? y : z

Risk

Low.

Performance impact

None.

Is this a regression from a previous update?

Regression from C#7.3.

How was the bug found?

Customer report

@cston cston requested a review from a team as a code owner November 8, 2018 21:51
@cston cston changed the title Allow nullable array type in conditional operators Parse array type in conditional operator Nov 8, 2018
@cston
Copy link
Member Author

cston commented Nov 8, 2018

@dotnet/roslyn-compiler, @gafter, please review.

@gafter gafter self-requested a review November 8, 2018 22:59
@@ -6262,7 +6245,7 @@ private enum ParseTypeMode
while (this.IsPossibleRankAndDimensionSpecifier())
{
bool unused;
var rank = this.ParseArrayRankSpecifier(mode == ParseTypeMode.ArrayCreation, expectSizes, allowQuestionToken: true, out unused);
var rank = this.ParseArrayRankSpecifier(mode == ParseTypeMode.ArrayCreation, expectSizes, questionTokenModeOpt: mode, out unused);
ranks.Add(rank);
Copy link
Member

Choose a reason for hiding this comment

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

This appears to apply to all levels of a multidimensional array. Shouldn't it apply only to the last? That is, shouldn't we allow var z = e is int[]?[] ? x : y; (existing code)

Copy link
Member Author

@cston cston Nov 8, 2018

Choose a reason for hiding this comment

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

We allow e is int[]?[] ? x : y and e is int[]?[]? ? x : y. The issue was we were incorrectly parsing the first case by treating the ? from the conditional as a nullable qualifier on the array type.

I've added tests for arrays of arrays. Thanks.

Copy link
Member

@gafter gafter left a comment

Choose a reason for hiding this comment

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

I suspect there is an issue parsing multidimensional arrays.

@cston
Copy link
Member Author

cston commented Nov 8, 2018

@dotnet/roslyn-compiler, please review.
@jaredpar for approval.

@cston
Copy link
Member Author

cston commented Nov 9, 2018

@dotnet/roslyn-compiler please review.

Copy link
Member

@gafter gafter left a comment

Choose a reason for hiding this comment

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

:shipit:

@jcouv jcouv added this to the 16.0 milestone Nov 9, 2018
}

[Fact]
public void ConditionalOperator_NotNullableArray()
Copy link
Member

@jcouv jcouv Nov 9, 2018

Choose a reason for hiding this comment

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

ConditionalOperator_NotNullableArray [](start = 20, length = 36)

Would it be useful to test x is T?[] y too? I'm afraid we disallow it, but I think we should allow it (seems useful).

Never mind, I think we allow it :-) Maybe still good to test

Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LGTM Thanks

@jcouv jcouv self-assigned this Nov 9, 2018
@jcouv
Copy link
Member

jcouv commented Nov 12, 2018

@jaredpar What is the process for preview1 fixes? Thanks

@jcouv jcouv modified the milestones: 16.0, 16.0.P1 Nov 12, 2018
@jcouv
Copy link
Member

jcouv commented Nov 12, 2018

Since there is a workaround (add parentheses), I'm pushing this back to preview2 (updated label and re-targeted PR). FYI @jaredpar

@jcouv jcouv modified the milestones: 16.0.P1, 16.0.P2 Nov 12, 2018
@jcouv jcouv changed the base branch from dev16.0.x to dev16.0-preview2 November 12, 2018 18:27
@cston cston closed this Nov 13, 2018
@cston cston reopened this Nov 13, 2018
@cston
Copy link
Member Author

cston commented Nov 13, 2018

@jaredpar for approval for dev16.0-preview2.

@jaredpar
Copy link
Member

approved

@cston cston merged commit c4598d7 into dotnet:dev16.0-preview2 Nov 13, 2018
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.

4 participants