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

Use default tuple fields in deconstruction since fields from inferred names are marked not usable in C#7 #21029

Merged
merged 1 commit into from
Jul 24, 2017

Conversation

cston
Copy link
Member

@cston cston commented Jul 21, 2017

Customer scenario

Create a deconstruction assignment where the RHS contains two or more tuple literals with different types and inferred names.

Bugs this fixes:

#21028

Workarounds, if any

Rewrite the RHS to avoid multiple tuple literals with different types and inferred names.

Risk

Low.

Performance impact

None.

Is this a regression from a previous update?

Regression resulting from inferred tuple names.

How was the bug found?

Customer reported.

@cston
Copy link
Member Author

cston commented Jul 21, 2017

@dotnet/roslyn-compiler please review.

@cston cston force-pushed the 21028 branch 3 times, most recently from d4db4ff to 1297886 Compare July 21, 2017 19:34
@cston cston changed the title Ignore inferred names in TupleTypeSymbol.TupleElementNames with previous language version Use default tuple fields in deconstruction since fields from inferred names are marked not usable in C#7 Jul 21, 2017
@cston
Copy link
Member Author

cston commented Jul 21, 2017

@dotnet/roslyn-compiler please review this small change, thanks.

@@ -223,6 +223,10 @@ private static bool IsTupleExpression(BoundKind kind)
{
var field = fields[i];

// Use default field rather than implicitly named fields since
// fields from inferred names are not usable in C# 7.0.
field = field.CorrespondingTupleField ?? field;
Copy link
Member

Choose a reason for hiding this comment

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

When would CorrespondingTupleField ever be non-null here?

Also can you give an example of when the use site diagnostic check below would ever get hit with this change?

Copy link
Member Author

@cston cston Jul 21, 2017

Choose a reason for hiding this comment

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

CorrespondingTupleField is null for the default (unnamed) fields, and non null for named (explicitly or inferred) fields.

The use-site diagnostic check should still be hit for use-site errors on the default fields, although I have not found a test case yet.

Copy link
Member

Choose a reason for hiding this comment

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

use-site diagnostic here is likely for cases with broken/malformed ValueTuple - like when the corresponding field is missing or inaccessible for other reasons, we should produce an error not a crash or something like that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @VSadov, an inaccessible tuple field hits this code path. I've added a use-site test.

Copy link
Member

@VSadov VSadov left a comment

Choose a reason for hiding this comment

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

LGTM

@dpoeschl
Copy link
Contributor

retest windows_release_vs-integration_prtest please

@cston
Copy link
Member Author

cston commented Jul 24, 2017

@dotnet-bot test windows_release_vs-integration_prtest please

@cston cston merged commit cd90c30 into dotnet:master Jul 24, 2017
@cston cston deleted the 21028 branch July 24, 2017 22:21
@gafter gafter self-requested a review July 28, 2017 18:58
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.

LGTM

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!

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.

7 participants