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

Deconstructions do not detect self assignments #16870

Closed
alrz opened this issue Feb 1, 2017 · 8 comments · Fixed by #18937
Closed

Deconstructions do not detect self assignments #16870

alrz opened this issue Feb 1, 2017 · 8 comments · Fixed by #18937
Assignees
Labels
Area-Compilers Feature - Tuples Tuples Feature Request Resolution-Fixed The bug has been fixed and/or the requested behavior has been implemented
Milestone

Comments

@alrz
Copy link
Member

alrz commented Feb 1, 2017

Version Used: 4fa6e3f

Steps to Reproduce:

class C {
  void M(int x, int y) {
    (x, y) = (x, 1);
  }
}

Expected Behavior:

warning CS1717: Assignment made to same variable; did you mean to assign something else?

Actual Behavior:

No warnings.

@HaloFour
Copy link

HaloFour commented Feb 1, 2017

I imagine that this is because the tuple literal expression and the deconstruction assignment are considered two effectively unrelated things. If #16869 were considered and the "deconstruction" instead converted into two simple assignments then the warning would make sense.

@alrz
Copy link
Member Author

alrz commented Feb 1, 2017

@HaloFour I think that's just an implementation detail, in effect, you are self-assigning a variable anyways.

@HaloFour
Copy link

HaloFour commented Feb 1, 2017

@alrz

I agree, that is the net result. But the compiler might see it more like you're calling a constructor with x and then assigning the value in field Item1 back to x.

Either way, I agree that this is probably a useful warning.

@jnm2
Copy link
Contributor

jnm2 commented Feb 6, 2017

@HaloFour I think the actual mechanism from the compiler's point of view is not relevant and should not be conflated with the language feature. To the extent that you have to think about the implementation mechanics of the language feature, the implementation mechanics are lacking. Ideally you wouldn't have to consider them.

@HaloFour
Copy link

HaloFour commented Feb 6, 2017

@jnm2

I agree. However, to the compiler that distinction is important. As it stands the compiler has no knowledge of how ValueTuple<...> works. For a tuple literal all it does is assume that there is a type called System.ValueTuple<...> in scope with a generic arity of the number of elements and a constructor that accepts the elements. For deconstruction all it does is assume that it can bind to accessible members named Item1, Item2, etc. Beyond that the compiler feigns ignorance. It's completely possible for someone to write their own System.ValueTuple implementation which does not behave as expected.

You and I probably agree that this case is likely pathological and unrealistic. But that seems to be the primary argument against #16869 and having the compiler treat a tuple literal/deconstruction as shorthand for simple assignments. And as such that would have to be the primary argument against this proposal.

@jnm2
Copy link
Contributor

jnm2 commented Feb 6, 2017

Yes. I strongly believe that the theory around ValueTuple as implementation mechanism for tuples should be to serve the language feature at the expense of treating ValueTuple like a normal type and caring about observable side effects of optimizations. No one will implement their own ValueTuple except to cater to this language feature, and we shouldn't go out of our way to allow ValueTuple to break the principles of the language feature. (x, _) = (x, 1) should reduce to x = x no matter what the ValueTuple implementation actually does.

@jcouv jcouv self-assigned this Feb 11, 2017
@jcouv jcouv added this to the 2.1 milestone Feb 11, 2017
@jcouv jcouv modified the milestones: Unknown, 2.1 Feb 19, 2017
@jcouv
Copy link
Member

jcouv commented Feb 19, 2017

Relates to #1580 (support for warning waves)

@jcouv
Copy link
Member

jcouv commented Apr 27, 2017

Fixed by #18937 (this will ship in dev15.3)
Thanks for raising the issue

@jcouv jcouv added Resolution-Fixed The bug has been fixed and/or the requested behavior has been implemented and removed 4 - In Review A fix for the issue is submitted for review. labels Apr 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers Feature - Tuples Tuples Feature Request Resolution-Fixed The bug has been fixed and/or the requested behavior has been implemented
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants