icsharpcode / ILSpy

.NET Decompiler with support for PDB generation, ReadyToRun, Metadata (&more) - cross-platform!
21.45k stars 3.35k forks source link

The operand of an increment or decrement operator must be a variable, property or indexer #1552

Closed greenozon closed 5 years ago

greenozon commented 5 years ago

ILSpy version 5.0.0.4844-preview2

ILSpy generates wrong ++ operator usage, eg:

                    decimal d = ++(result * (1m - result3 / 100m) / 100m);
                    decimal d2 = ++(result2 * (1m - result3 / 100m) / 100m);

vs:

image

Here is how reflector takes care of this case:


decimal? nullable2 = new decimal?((num4 * ((((result * (1M - (num3 / 100M))) / 100M) + 1) / (((num2 * (1M - (num3 / 100M))) / 100M) + 1))) - num4);
--
tamlin-mike commented 5 years ago

Reflector is obviously also not too good when it comes to nullables, since it new's a nullable decimal.

Have you checked to see what dotPeek generates? I suspect that might be ... less bad.

greenozon commented 5 years ago

dotPeek generated 3rd option:

image

is it valid? :)

tamlin-mike commented 5 years ago

I'm torn between admiration, even awe, for the almost total lack of optimality, while trying to not spray the monitor with coffee from laughing. I tip my hat for dotPeek. Thanks @greenozon for making this day a little brighter.

siegfriedpammer commented 5 years ago
public void Test()
{
    decimal d = 4;
    decimal.op_Increment(d);
}

leads to CS0571 'decimal.operator ++(decimal)': cannot explicitly call operator or accessor.

So, no dotPeek is producing total bogus here. They're not even trying to translate it.

@greenozon Could you provide the assembly for further investigation? Thank you!

greenozon commented 5 years ago

yes, sure will do it later on, today

greenozon commented 5 years ago

CcyRateCalcer class private void Calculate() decimals.zip

greenozon commented 5 years ago

This is output from Telerik JustDecompile engine

 decimal num5 = ((num * (new decimal(1) - (num2 / new decimal(100)))) / new decimal(100))++;
                                                decimal num6 = ((num1 * (new decimal(1) - (num2 / new decimal(100)))) / new decimal(100))++;
     nullable3 = new decimal?((num3 * (num5 / num6)) - num3);
     decimal num7 = num3;
     decimal num8 = num4;
     decimal? nullable4 = nullable3;
     if (nullable4.HasValue)
     {
       nullable = new decimal?(num8 * nullable4.GetValueOrDefault());
    }
    else
   {
         nullable = null;
   }
dgrunwald commented 5 years ago

The problem is that C# only allows: result = ++lvalue, which compiles to result = (lvalue = op_Increment(lvalue)). However, for local variables, Roslyn may optimize out the assignment to lvalue if it can tell that this is a dead store. The first use of lvalue can then also be optimized out it the store was just in the prior statement:

So:

void Test(decimal a, decimal b) {
   decimal lvalue = a + b; 
   Console.WriteLine(++lvalue);
}

gets optimized to:

void Test(decimal a, decimal b) {
   Console.WriteLine(decimal.op_Increment(a + b));
}

ILSpy currently renders this as Console.WriteLine(++(a + b));, the dotPeek output is a bit more accurate here.

To produce valid C#, a decompiler would have to re-introduce the variable that got deleted by the compiler.

dgrunwald commented 5 years ago

While my previous comment applies to general user-defined types, for decimal in particular some C# compiler versions also optimize d + 1m to decimal.op_Increment(d). So ILSpy will now reverse this optimization to ensure the code is valid.

The original example now gets decompiled as:

                decimal d = result * (1m - result3 / 100m) / 100m + 1m;
                decimal d2 = result2 * (1m - result3 / 100m) / 100m + 1m;
                num = result4 * (d / d2) - result4;
github-actions[bot] commented 4 years ago

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.