Roslyn: Using elivs operator lowers code coverage

[BrainSlugs83]

Version Used:
1.3.1.60616

Steps to Reproduce:

Given the following set of classes:

public class A { public B B { get; } = new B(); }
public class B { public C C { get; } = new C(); }
public class C { public bool value { get; } = true; }

Create two unit tests as follows that are equivalent -- one uses the Elvis operator introduced in C# 6, and one manually checks for null, the old fashioned way.

[Test]
public static void TestWithManualNullChecks()
{
    // Pre-C# 6.0 code that gets 100% coverage in both Debug and Release modes.
    var a = new A();
    if (a != null)
    {
        if (a.B != null)
        {
            if (a.B.C != null)
            {
                if (a.B.C.value)
                {
                    // do something here
                    Trace.WriteLine("Hello World!");
                }
            }
        }
    }
}

[Test]
public static void TestWithElvisOperator()
{
    // C# 6.0 Code that uses Elvis operator, only gets 78.95% Coverage in Debug mode.
    // C# 6.0 Code that uses Elvis operator, only gets 77.78% Coverage in Release mode.
    var a = new A();
    if (a?.B?.C?.value == true)
    {
        // do something here
        Trace.WriteLine("Hello World!");
    }
}

Run the two unit tests via "Analyze Code Coverage for All Tests" in the Visual Studio Test Runner.

Expected Behavior:
Since all branches of the source code are reached, code coverage should be 100% (as it is when we do the exact same thing without using the Elvis operator).

Actual Behavior:
The Elvis operator is inserting invisible branches into the code that can't be reached causing the code coverage results to be lowered. -- Even though there are only two meaningful branches to be tested here, we would have to provide every combination of null / not null objects to get back to 100% code coverage. -- This completely negates the code savings introduced by the Elvis operator reducing it's usefulness.

Notes:
Let me know if there's any further detail I can add, or anything I can do to help. I really love the new features you've been adding to C#, including the Elvis operator, but this issue is really killing me.

[HaloFour]

Out of curiosity what is the coverage if you change the condition to the following?

[Test]
public static void TestWithElvisOperator()
{
    var a = new A();
    if (a?.B?.C?.value ?? true)
    {
        // do something here
        Trace.WriteLine("Hello World!");
    }
}

[BrainSlugs83]

Looks like coalesce scores the exact same as the equality check:
Debug: 78.95%
Release: 77.78%

I ran it several times to be sure.

[BrainSlugs83]

Likewise, if I add an equality check to "TestWithManualNullChecks" (e.g. if (a.B.C.value == true)), it stays at 100%.

[HaloFour]

Just curious if the implicit conversion from bool? mattered at all.

The two code blocks aren't really equivalent. The pre-C# 6.0 form matching the null-propagation operator would be more akin to the following:

[Test]
public static void TestLikeElvisOperator()
{
    var a = new A();
    bool? $temp;
    if (a == null) {
        $temp = null;
    }
    else {
        var b = a.B;
        if (b == null) {
            $temp = null;
        }
        else {
            var c = b.C;
            if (c == null) {
                $temp = null;
            }
            else {
                $temp = c.value;
            }
        }
    }
    if ($temp == true)
    {
        // do something here
        Trace.WriteLine("Hello World!");
    }
}

The big difference between that the property accessors are cached (via il dup instruction) and that the expression results in a bool? which would always be evaluated in the final condition.

Not arguing one way or another, just describing the semantic differences.

[BrainSlugs83]

@HaloFour -- I'm not sure what you mean by "property caching" here -- but the difference -- from a coverage percentage -- are the extra code branches that get inserted and are never executed -- like when you have several completely separate branches for "$temp = null". -- Basically the compiler is inserting invisible branches here, which should really be a no-no, as it also drives up the measurable cyclomatic complexity.

i.e. if the code you showed above could be changed to the following code (and it's 100% functionally equivalent), those extra branches could be eliminated:

[Test]
public static void TestLikeElvisOperator()
{
    var a = new A();
    bool? $temp = null; // assign null once, instead of several times in invisible branches.
    if (a != null)
    {
        var b = a.B;
        if (b != null)
        {
            var c = b.C;
            if (c != null)
            {
                $temp = c.value;
            }
        }
    }
    if ($temp == true)
    {
        // do something here
        Trace.WriteLine("Hello World!");
    }
}

^ this method achieves ~95% coverage (up from 82% in your example) -- which is a huge improvement IMO -- according to Visual Studio the 5% hit comes from the "$temp == true" check (which is apparently introducing an extra hidden branch as well. ~sigh).

[HaloFour]

@BrainSlugs83

I'm not sure what you mean by "property caching" here

Only that ?. ensures that the target property is only evaluated once whereas in your original pre-C# 6.0 example you were calling the property getter accessor methods multiple times. I don't think that has an impact on the coverage results, though.

I believe that the compiler is emitting the initialization in each branch in order to only initialize the null result in the case that the expression is null. That initialization involves two IL opcodes that it would otherwise not have to execute. Although considering that the end result is null and that the .locals have the init flag that does seem kind of redundant; the value should be null by default regardless of whether it is a reference type or a Nullable<T>.

My code example is only really an approximation. The compiler emits IL that can't directly be represented by C#, notably the dup operation on the expression result which avoids the need to declare a local.

Anywho, what the compiler emits is pretty raw. It's not like it's converting the ?. operator into C# using if/then/else blocks. I assume that the correlation between IL and the original source is determined by the debugger symbols file which perhaps treats each ?. as its own block. It may have been a conscious decision that each use qualify as a separate branch. I'd could certainly understand the argument that your coverage isn't 100% as you're not testing how the method would behave if any of those properties were to return null.

[BrainSlugs83]

Only that ?. ensures that the target property is only evaluated once whereas in your original pre-C# 6.0 example you were calling the property getter accessor methods multiple times. I don't think that has an impact on the coverage results, though.

Ahh, that makes sense. I concur, the dup operation should not be affecting the code coverage. -- Only instructions that can branch should affect it.

I believe that the compiler is emitting the initialization in each branch in order to only initialize the null result in the case that the expression is null. That initialization involves two IL opcodes that it would otherwise not have to execute. Although considering that the end result is null and that the .locals have the init flag that does seem kind of redundant; the value should be null by default regardless of whether it is a reference type or a Nullable.

That's exactly what I'm getting at. -- (And I understand that the compiler emits IL and not C# -- the code above was just for high level illustration) -- as you mention, those extra initialization branches it is adding are actually not needed, which is what I'm suggesting that we remove.

I could certainly understand the argument that your coverage isn't 100% as you're not testing how the method would behave if any of those properties were to return null.

1. 100% just means that every single _block_ of code was executed, not that it got to every block via every possible path/behavior (notice that in the 100% example I posted, I never executed a code path where value == false either). -- 100% coverage is not the same as 100% tested -- it's just a good starting point. :-)
2. For that matter, if this was the motivation behind the decision then it would also make sense for every single C# if statement to be compiled the same as an if / else statement (with a nop instruction inside of the block that the else branch got rendered to). -- Granted, the code that was added in this case is not literally a nop instruction, it is still guaranteed to have about the same effect.
3. All that being said, I'm not sure I fully grasp the point of testing compiler generated code that is literally guaranteed to have zero affect. ;-)
4. And testing aside, there's at least the performance issue to consider (I mean, we're inserting extra instructions and branches of code into the final executable image that are just plain not required / and are guaranteed to have no effect -- seems to me it would be better if we just stopped doing that... XD).

[peteraritchie]

This seems more like a change to the code coverage logic, not to the compiler logic. Is Code Coverage part of the roslyn source code?

[BrainSlugs83]

No. The code coverage logic is fine. -- Literally, the IL code that's generated regarding the elvis operator has a higher cyclomatic complexity than the IL generated by the advertised equivalent; it's literally doing extra work that it doesn't need to be doing, and generating extra code that is redundant and serves no purpose.

i.e. the following code:

object result = null;
result = SomeExistingVar?.Something();
return result;

Is generating IL with a higher cyclomatic complexity than the advertised equivalent:

object result = null;
var copy = SomeExistingVar;
if (copy != null)
{
  result = copy.Something();
}
return result;

Again, the latter snippet, generates IL code with FEWER branches than the former.

This is absolutely a bug in the compiler.

[CyrusNajmabadi]

it's literally doing extra work that it doesn't need to be doing

What extra work is it doing that it doesn't need to be doing?

[CyrusNajmabadi]

has a higher cyclomatic complexity than the IL generated by the advertised equivalent

The advertised equivalent of:

c# object result = null; result = SomeExistingVar?.Something(); return result;

is not the code you specified. It's:

```c#
object result = null;
var copy = SomeExistingVar;
result = copy == null ? null : copy.Something();
return result;

[CyrusNajmabadi]

but the difference -- from a coverage percentage -- are the extra code branches that get inserted and are never executed

If you do care about code coverage (i'm not going to argue if that's something to actually care about or not), then why not write tests that actually hit all these other branches? i.e. tests that will results in null for all those potential cases?

[CyrusNajmabadi]

This seems more like a change to the code coverage logic, not to the compiler logic. Is Code Coverage part of the roslyn source code?

No, it is not. :)

[CyrusNajmabadi]

See spec on how this actually is defined to be rewritten:

https://github.com/dotnet/csharplang/blob/master/spec/expressions.md#null-conditional-operator

[sharwell]

The original code claims that the test TestWithManualNullChecks is fully covered, but it is not. The bug appears to be a misreporting of the original, not the one that uses ?..

To be more specific, I would expect any conditional null check in code to only be covered if the condition is reached both for a null input and a non-null input. The code above only checks the non-null case, leading to the use of ?. correctly reporting that the code is not fully covered.