ILSpy makes mistakes in logical conditions using bitwise operators instead of logical one

[greenozon]

ILSpy version 5.0.0.4840-preview2

here is a self-descriptive picture
left pane - from dnSpy (OK), right- not OK (ILSpy)

as you might see - if there is a negative "!" operator before bool var - && (=OK) is used, otherwise a mistake

[dgrunwald]

I think this is a C# compiler optimization -- the C# compiler detects that there are no side effects on the right-hand-side of &&, and emits a bitwise & instead.
ILSpy then correctly shows that the IL code performs a bitwise &.
dnSpy maybe has some code that reverses the C# compiler optimization?

In any case, this is not a bug: both decompiled results would compile to the same IL, so are indistinguishable for a decompiler.

[greenozon]

here is an excerpt of IL with C# for bit operand

	// if ((resolution == "1920x1080" && (num == 29.97 || num == 29.9701)) & isInterlaced)
	IL_01b9: ldarg.1
	IL_01ba: ldstr "1920x1080"
	IL_01bf: call bool [mscorlib]System.String::op_Equality(string, string)
	IL_01c4: brfalse.s IL_01e3

	IL_01c6: ldloc.0
	IL_01c7: ldc.r8 29.97
	IL_01d0: beq.s IL_01e0

	IL_01d2: ldloc.0
	IL_01d3: ldc.r8 29.9701
	IL_01dc: ceq
	// (no C# code)
	IL_01de: br.s IL_01e1

	IL_01e0: ldc.i4.1

	IL_01e1: br.s IL_01e4

	IL_01e3: ldc.i4.0

	IL_01e4: ldarg.2
	IL_01e5: and
	IL_01e6: stloc.s 10
	IL_01e8: ldloc.s 10
	IL_01ea: brfalse.s IL_01f8

	IL_01ec: nop
	// return "Hi59";
	IL_01ed: ldstr "Hi59"
	IL_01f2: stloc.2
	// (no C# code)
	IL_01f3: br IL_0862

	// if (resolution == "1920x1080" && num == 30.0 && !isInterlaced)

[tamlin-mike]

@dgrunwald wrote "I think this is a C# compiler optimization"
Yup, it is.

Granted, most source code is using && and ||, but while some might find it not-easy-on-the-eyes it's not an error to replace them with bitwise op's.

That said, ILSpy should probably emit logical && and ||, and defer the optimization to the compiler later using the generated src, for the cases they are equivalent.

Another optimization - that at least csc is completely omitting - is
if ((a && !b) || (!a && b)) => if (a ^ b) (or even if (a != b)),
that is also impossible to guess what the original src was. csc is gracefully completely non-optimizing, so these issues are rare.

[dgrunwald]

Did this get implemented somewhere without referencing this issue?
We now have a bug report where the opposite behavior is desired: #1924

[tamlin-mike]

be28469 ? :-)

[dgrunwald]

OK digging through the history, there's a bit back-and-forth...

Originally this was a contribution: fe6a85f

Then ef86604 touched that code; here this issue started to interact with the nullable lifting transform (for the compiler-generated a.HasValue & b.HasValue)

Then there turned out to be problems with the &= transform, so I removed the de-optimization again: 93806b4
But that broke that build because we still had a bunch of tests expecting &&, so I added the feature back (but at a later stage in the decompiler pipeline) in the commit you found, be28469.

I'm still not sure this feature is a good idea. When both options are semantically identical and equally readable, we essentially have free choice; so do we consider "&& is more usual" or "& is closer to the IL" to be more important? I think we should go for "closer to the IL" in such cases.

[tamlin-mike]

Hahaha, this problem seems to have walked a path as straight as a sailor hitting land after six months at sea, and a barrel of beer after landfall.

If there's any risk of mis-decompilation, it should be removed.

Probably a test should also be written for #1924. To not depend on exact syntax of output (as it seems it can be in flux), it could use a regexp to at least verify binary op where binary op is expected.

Once that test is in place, experimentation with making decompiled code look more like written by a human could resume.

My 0.02.

[dgrunwald]

Well #1924 is not a mis-decompilation, the program semantics are the same either way.
It's just that ILSpy is smarter than Roslyn here and recognized this as a candidate for the (de-)optimization.

There's several options here. All of them fulfill the primary goal "the decompiled code is correct and readable".
a) Keep the status quo: replace & on bools with && whenever possible.
b) Reduce the scope of the transform; e.g. instead of working with arbitrary effect-free expressions on the right-hand-side, just accept simple variables (inst.Right is LdLoc only). That covers most (maybe all?) of the cases I've seen Roslyn perform the optimization.
c) Remove the transform and adjust our test cases (e.g. by replacing local variables with method calls, so that we don't need to add a bunch of #if ROSLYN in the tests).

No matter which option we choose, it's going to end up surprising for someone who doesn't know about this optimization/de-optimization. Option b has the added advantage that it might be surprising for both camps simultaneously ;)

I think my pick would be c), stay as close to the IL as possible.
It's a real optimization that happens, why should ILSpy hide it from the user?

[tamlin-mike]

While my knee-jerk reaction would probably be to vote a), I do have in fresh memory the surprise I got by switching view from AST to "C#, No Transforms", yet this very transform had already been applied and I simply couldn't get ilspy to display C# without it. I found that frustrating.

One could argue "well that's because it's not a Transform per se", but such an argument would be met with a cold stare from me. :-) Given the options as-are, I'd have to agree that c) seems best.

Should things change, to the point the transform becomes an actual Transform, letting you view the "raw" result of decompilation, then switch to run "Logical Binary op's deoptimization" (or whatever it'd be called), I'd have to reevaluate that position.

Hmm, this uncertainty could suggest that "transformation" could someday be worth of a decompiler option (not Tranformation step, but a checkbox option). Maybe it'd even be better suited as an AST transform (giving both C# and VB output the same chance)? Maybe. See, I'm torn all over the place on this. :-)
(I blame the coffee -- the lack of it)

My 0.001.