Allow mixed declaration and assignment in deconstruction #44476

YairHalberstadt · 2020-05-21T11:15:47Z

Implements dotnet/csharplang#125
Test plan: #47746

I simply removed the error, and added tests. So far everything seems to work without any actual code changes, which worries me a little :-)

Please suggest scenarios for further testing.

RikkiGibson · 2020-05-21T15:16:18Z

are there any tests for nested deconstruction? i.e. (var x, (var y, z)) = new C(); #Resolved

…e/mixed-declaration-and-assignment

RikkiGibson · 2020-05-21T15:32:32Z

do we need to retarget this to a "compiler-next" branch? @jaredpar

YairHalberstadt · 2020-05-21T17:50:54Z

are there any tests for nested deconstruction? i.e. (var x, (var y, z)) = new C();

Thanks, added a test #Resolved

jcouv · 2020-05-27T00:28:40Z

So far everything seems to work without any actual code changes [just removing the error]

That's what we expected :-)

Just as a heads up, we likely won't have much time to review this PR before 6/10 (we're trying to get a big chunk of records in next preview).
Also, we'll want to double-check this feature by LDM. I don't feel strongly opposed, but I still don't much like the idea of mixing from a readability point of view :-/ #Resolved

alrz · 2020-05-28T08:40:34Z

Is there any consideration to permit tuple element names in deconstruction? The expected behavior is already established by recursive patterns for both Deconstruct and tuples. See dotnet/csharplang#3304

YairHalberstadt · 2020-05-28T08:42:20Z

@alrz

I don't see why that's related? This just removes a (somewhat arbitrary) error.

alrz · 2020-05-28T08:44:55Z

I don't see why that's related? This just removes a (somewhat arbitrary) error.

I believe this is also a syntactical relaxation for deconstruction and removes an error. The parser already accept such code.

jcouv · 2020-09-15T19:51:39Z

LDM approved yesterday for language version following C# 9 (9/15/2020). #Resolved

src/Compilers/CSharp/Portable/Binder/Binder_Deconstruct.cs

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenDeconstructTests.cs

jcouv

Done with review pass (iteration 5)

RikkiGibson · 2020-09-15T23:54:23Z

The feature is approved as preview feature for C# 10 (VS 16.9 under LangVersion.Preview). If this PR is to be merged in the next couple of weeks, then it will be necessary to update the base branch to release/dev16.9-preview1 (tenatively named).

If on the other hand it will be a while before the PR is merged, we can just wait until master represents VS 16.9.

/cc @jaredpar #Resolved

YairHalberstadt · 2020-09-16T05:49:58Z

The feature is approved as preview feature for C# 10 (VS 16.9 under LangVersion.Preview). If this PR is to be merged in the next couple of weeks, then it will be necessary to update the base branch to release/dev16.9-preview1 (tenatively named).

Would it not be possible to merge it now as a preview feature for C# 10? All the current C# 9 features are no longer in preview. There's zero risk of breaking C# 9.0 since all this does is replace a warning with a preview gate - no code gen changes at all.

Otherwise might as well wait a couple of weeks.

Either way I imagine that there's further tests that need to be done before this can be merged:

Off the top of my head I'm guessing Symbols and Control flow tests? Anything else? GoToDefinition tests?

jcouv · 2020-09-16T15:53:19Z

Yes, we discussed with Rikki offline and I think we can use the master branch right away since the feature is behind LangVer=preview.

Either way I imagine that there's further tests that need to be done before this can be merged

Yes. I opened a mini test-plan issue for tracking. I think we can just do the whole feature in a single PR.

For IDE side, I'll start a thread and get back to you.

From the compiler side:

A data flow test would be good (a test in the vein of RegionAnalysisTests.AnalysisOfExpressionInTupleEquality).
Let's check the semantic model in nested scenario (see VerifyModelForDeconstructionLocal which covers declaration, and add checks for GetTypeInfo and GetSymbolInfo on variables used in mixed declarations).
Add an IOperation test

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenDeconstructTests.cs

333fred

LGTM (commit 13). I'm not exactly sure when we'll be able to get this merged, hopefully sometime in the next week or 2. We're heads down on remaining C# 9 fixes for the next 2 weeks.

YairHalberstadt · 2020-10-13T17:51:53Z

@333fred any chance we can get this merged this week?

jcouv

LGTM Thanks (iteration 14)

333fred · 2020-10-14T00:11:06Z

Next week.

jcouv · 2020-12-10T19:43:51Z

Sorry for the delay. I'm going to run an IDE test pass today and find an IDE buddy.

jcouv · 2020-12-14T21:12:40Z

Ran this change through a quick IDE test pass on Friday, focusing on completion and QuickInfo (since those seems most likely to be affected). Things look good (behavior is same as for regular deconstruction). Pinged Jinu to get an buddy to get the IDE sign-off.

sharwell · 2021-01-19T23:24:41Z

It looks like there are several test gaps in IDE code. They aren't new for this PR, but we should take the opportunity to close them.

VarKeywordRecommenderTests, IntKeywordRecommenderTests, etc., should be updated:
- Verify keyword is provided for (x, $$
- Verify the keyword is not provided for var (x, $$
SymbolCompletionProviderTests needs to be updated to explicitly test (var x, $$ (reference provided) and var (x, $$ (reference not provided). See TupleTypeAtMemberLevel3 for an example and positioning within the file.
- Verify that x is not provided for (var x, $$ (while other symbols are)
Consider extending the above examples to cover nested deconstruction.

[jcouv update:] Sam also suggested to verify (var x, x) = (1, 2); and (var _, _) = (0, 0);. I don't recall if we covered that.

YairHalberstadt · 2021-01-20T05:35:41Z

Looks like (var _, _) = (0, 0); is allowed today. I have absolutely no idea why, but it would be a breaking change to remove.

YairHalberstadt · 2021-01-20T05:37:00Z

As is:

        (var x, _) = (0, 0);
        Console.WriteLine(x);

So it looks like mixed declaration and discard has always been allowed.

YairHalberstadt · 2021-01-20T09:22:14Z

Verify that x is not provided for (var x, $$ (while other symbols are)

Currently the logic to check is a local is in scope simply checks that's it's declared before the current syntax, so this is suggested, as is x in var x = $$.

I don't want to change that logic in the PR as it seems like a relatively complex change, and we do this already, so we're no worse off by merging this.

sharwell · 2021-01-20T13:18:10Z

How does the language define this code after your feature?

class T {
  int x;
  void M() {
    (var x, x) = (1, 2);
    Console.WriteLine(x);
    Console.WriteLine(this.x);
  }
}

YairHalberstadt · 2021-01-20T13:19:18Z

@sharwell that's an error

sharwell · 2021-01-20T14:56:24Z

And (x, var x) there would also be an error?

YairHalberstadt · 2021-01-20T14:59:50Z

Yes, and I have tests for both

jcouv · 2021-01-27T17:31:56Z

@sharwell We have the necessary sign-offs from the compiler side. Yair has added the IDE (thanks!) and they match what I'd verified manually (behave as expected).
We're ready to merge if you're all good. Thanks

* upstream/master: For the purpose of runtime capability check require RuntimeFeature type to be a static class. (dotnet#50829) Allow mixed declaration and assignment in deconstruction (dotnet#44476) Disable sdl Fix crash around handling delegating of constructors cross-project Clarify in a comment when some stuff loads Don't reference the implementation type directly if we don't need it Remove the CreateWorkspace() method that implies it's created there Delete AbstractPackage`2.IsInIdeMode() Remove some very outdated comments

jcouv · 2021-01-28T01:31:43Z

Merged/squashed. Thanks @YairHalberstadt! :-)

Allow mixed declaration and assignment in deconstruction

1fd7a41

YairHalberstadt requested a review from a team as a code owner May 21, 2020 11:15

YairHalberstadt added 2 commits May 21, 2020 18:21

Fix broken tests

590bb6a

Merge branch 'master' of https://github.com/dotnet/roslyn into featur…

e1aa2a6

…e/mixed-declaration-and-assignment

RikkiGibson added Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee. labels May 21, 2020

YairHalberstadt added 2 commits May 21, 2020 20:40

Gate feature on preview

c37fd84

Test nested deconstruction

a9ddfbf

jcouv self-assigned this Jun 3, 2020

jcouv reviewed Sep 15, 2020

View reviewed changes

src/Compilers/CSharp/Portable/Binder/Binder_Deconstruct.cs Outdated Show resolved Hide resolved

jcouv reviewed Sep 15, 2020

View reviewed changes

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenDeconstructTests.cs Outdated Show resolved Hide resolved

jcouv reviewed Sep 15, 2020

View reviewed changes

merged mrom master

075b55c

Changes based on review by @jcouv

8512dd3

jcouv mentioned this pull request Sep 16, 2020

Test plan for "Mix declarations and variables in deconstruction" #47746

Closed

5 tasks

jcouv reviewed Sep 16, 2020

View reviewed changes

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenDeconstructTests.cs Show resolved Hide resolved

jcouv reviewed Sep 16, 2020

View reviewed changes

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenDeconstructTests.cs Outdated Show resolved Hide resolved

jcouv reviewed Sep 16, 2020

View reviewed changes

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenDeconstructTests.cs Show resolved Hide resolved

jcouv reviewed Sep 16, 2020

View reviewed changes

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenDeconstructTests.cs Show resolved Hide resolved

RikkiGibson requested review from a team and removed request for a team September 23, 2020 13:25

333fred approved these changes Sep 23, 2020

View reviewed changes

Merge branch 'master' into feature/mixed-declaration-and-assignment

33af2f5

jcouv approved these changes Oct 13, 2020

View reviewed changes

merge from master

739bc4a

Add tests suggested by @sharwell

40d1a51

sharwell approved these changes Jan 27, 2021

View reviewed changes

jcouv merged commit c3b8b10 into dotnet:master Jan 28, 2021

ghost added this to the Next milestone Jan 28, 2021

tmat mentioned this pull request Jan 29, 2021

EnC: validate mixed declarations and variables in deconstruction #50901

Closed

RikkiGibson modified the milestones: Next, 16.10.P1 Feb 17, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow mixed declaration and assignment in deconstruction #44476

Allow mixed declaration and assignment in deconstruction #44476

YairHalberstadt commented May 21, 2020 •

edited by jcouv

Loading

RikkiGibson commented May 21, 2020 •

edited by jcouv

Loading

RikkiGibson commented May 21, 2020

YairHalberstadt commented May 21, 2020 •

edited by jcouv

Loading

jcouv commented May 27, 2020 •

edited

Loading

alrz commented May 28, 2020

YairHalberstadt commented May 28, 2020

alrz commented May 28, 2020

jcouv commented Sep 15, 2020 •

edited

Loading

jcouv left a comment

RikkiGibson commented Sep 15, 2020 •

edited by jcouv

Loading

YairHalberstadt commented Sep 16, 2020

jcouv commented Sep 16, 2020

333fred left a comment

YairHalberstadt commented Oct 13, 2020

jcouv left a comment

333fred commented Oct 14, 2020

jcouv commented Dec 10, 2020

jcouv commented Dec 14, 2020

sharwell commented Jan 19, 2021 •

edited by jcouv

Loading

YairHalberstadt commented Jan 20, 2021

YairHalberstadt commented Jan 20, 2021

YairHalberstadt commented Jan 20, 2021

sharwell commented Jan 20, 2021

YairHalberstadt commented Jan 20, 2021

sharwell commented Jan 20, 2021

YairHalberstadt commented Jan 20, 2021

jcouv commented Jan 27, 2021

jcouv commented Jan 28, 2021

Allow mixed declaration and assignment in deconstruction #44476

Allow mixed declaration and assignment in deconstruction #44476

Conversation

YairHalberstadt commented May 21, 2020 • edited by jcouv Loading

RikkiGibson commented May 21, 2020 • edited by jcouv Loading

RikkiGibson commented May 21, 2020

YairHalberstadt commented May 21, 2020 • edited by jcouv Loading

jcouv commented May 27, 2020 • edited Loading

alrz commented May 28, 2020

YairHalberstadt commented May 28, 2020

alrz commented May 28, 2020

jcouv commented Sep 15, 2020 • edited Loading

jcouv left a comment

Choose a reason for hiding this comment

RikkiGibson commented Sep 15, 2020 • edited by jcouv Loading

YairHalberstadt commented Sep 16, 2020

jcouv commented Sep 16, 2020

333fred left a comment

Choose a reason for hiding this comment

YairHalberstadt commented Oct 13, 2020

jcouv left a comment

Choose a reason for hiding this comment

333fred commented Oct 14, 2020

jcouv commented Dec 10, 2020

jcouv commented Dec 14, 2020

sharwell commented Jan 19, 2021 • edited by jcouv Loading

YairHalberstadt commented Jan 20, 2021

YairHalberstadt commented Jan 20, 2021

YairHalberstadt commented Jan 20, 2021

sharwell commented Jan 20, 2021

YairHalberstadt commented Jan 20, 2021

sharwell commented Jan 20, 2021

YairHalberstadt commented Jan 20, 2021

jcouv commented Jan 27, 2021

jcouv commented Jan 28, 2021

YairHalberstadt commented May 21, 2020 •

edited by jcouv

Loading

RikkiGibson commented May 21, 2020 •

edited by jcouv

Loading

YairHalberstadt commented May 21, 2020 •

edited by jcouv

Loading

jcouv commented May 27, 2020 •

edited

Loading

jcouv commented Sep 15, 2020 •

edited

Loading

RikkiGibson commented Sep 15, 2020 •

edited by jcouv

Loading

sharwell commented Jan 19, 2021 •

edited by jcouv

Loading