Roslyn-analyzers: [Proposal] Compiler warnings with unobvious assignment

[Kir-Antipov]

C# is a beautiful and flexible language that will not allow a beginner to "shoot off his leg"... or it will?

---

Issue

Let's imagine that we have the following class:

class Foo
{
    public int Value;
    public Foo Next;
}

Then we want to do something like this:

var a = new Foo();
var b = new Foo { Value = 1, Next = new Foo { Value = 2 } };
a.Next = a = b;
Console.WriteLine(a.Next == a);

What we'll see in the console's output?
Most of you're now thinking something like this:

It's kinda simple!
First we assign "b" to "a", so "a" and "b" are now the same
Then we assign "a" (or "b", cause they're the same now) to the "Next" field of object "a" (or "b")
So a.Next == a is true!

And I have some bad news for you, cause it's false :D
If you're surprised, then let me surprise you even more:

a = b;
a.Next = a; // or b
Console.WriteLine(a.Next == a); // true

---

Explanation

When we have construction like this:

a = b;
a.x = b;

As a result, we'll get this IL-code:

;store b into a
ldloc b
stloc a 

;store b into a.x
ldloc a
ldloc b 
stfld class Foo Foo::X

But if we want to simplify this, and we'll use the following construction (that should mean the same as the example above):

a.x = a = b; 

IL will look like this:

ldloc a ;load "a" to the top of the stack
ldloc b ;load "b" to the top of the stack
dup     ;load "b" to the top of the stack
stloc a ;store "b" to "a"
stfld class Foo Foo::X ;store "b" to an object on top of the stack that is no longer "a"!

As we see, the assignment happens to the object we didn't expected (cause we know that assignment goes from right to left).

var aClone = a;
a.x = a = b; 
Console.WriteLine(a.x == a); // false
Console.WriteLine(a.x == b); // false
Console.WriteLine(aClone.x == a); // true
Console.WriteLine(aClone.x == b); // true

---

Proposal

I found this behaviour kinda unobvious. Especially for those who aren't familiar with the "subtleties" of C#

So I want to suggest introducing compiler warnings for those cases where the left side of an assignment contains a variable, the object inside of which changes somewhere in the right side of the expression, since it wouldn't be very cool to replace a = b; a.x = b; with a similar a.x = a = b; and spend a few sleepless nights in search of a bug :D

[maxkoshevoi]

Here is another code snippet that demonstrates this issue:

Its also worth mentioning that Visual Studio aware of the difference between first = second; first.Next = first; and first.Next = first = second;. In the TestOne method it states that initial value in first variable is never used, but it doesn't mention that in the TestTwo method.

[mavasani]

Analyzer implemented with https://github.com/dotnet/roslyn-analyzers/pull/2717

I am going to keep this issue open to track addition of a potential code fixer. I think we should offer following code fixes to the user to allow splitting their current statement into separate assignments for clarity. Let's assume original code on which the diagnostic fired: a.x = a = b;

1. Fix 1: Splits the code into 2 statements as follows, which is the most likely original intent:

a = b;
a.x = b;

1. Fix 2: Splits the code into 2 statements as follows, which is another possible intent:

a = b;
a.x = a;

1. Fix 3 (may be?): Splits the code into statements as per the actual IL generated for original code:

var temp = a;
a = b;
temp.x = b;