Lowering of value is ConstantA or ConstantB

[dellamonica]

In my code I had line similar to

Assert(value is ConstA or ConstB);

This was lowered to

bool condition = ((num == 1 || num == 4) ? true : false);
Assert(condition, ...);

I understand that the JIT will most likely simplify this to avoid the ternary. However, when the code-coverage tool analyzes the code, it will consider that the falsy branch is never taken (and it should not be in this case, hence the Assert). Since I actually take branch covering very seriously, the false positives due to the lowering are annoying.

So, why is this code lowered this way?

[HaloFour]

Feels like this should be a Roslyn question? I don't think the language spec says anything about how exactly that code should be lowered as long as the result is the same.

[dellamonica]

Indeed, if the language spec does not require it (if anyone could confirm that) then I could move the discussion to the compiler.

[jnm2]

@333fred are you able to move discussion threads between repos?

[colejohnson66]

Code coverage tools should be working on the high-level representation of code, not the lowered version.

[dellamonica]

I disagree, the tool should report branches not taken in the actual code that gets executed.

That's up to the tool. Feel free to report such an issue to the tool you're using. That's not Roslyn itself.

There is no issue with the tool in this case, maybe it is ILSpy or whatever the tool is using that reports the branch to the tool.

[CyrusNajmabadi]

You said above:

However, when the code-coverage tool analyzes the code, it will consider that the falsy branch is never taken

That's an issue with the code coverage tool. It considers a branch not taken when it would be taken.

[dellamonica]

You said above:

However, when the code-coverage tool analyzes the code, it will consider that the falsy branch is never taken

That's an issue with the code coverage tool. It considers a branch not taken when it would be taken.

The branches that should be covered are "value == 1" and "value == 4", anything other than those two cases is an assertion failure that corresponds to a bug in my code. What the tool reports is that the buggy path is not covered because it sees a ternary that looks like boolean_value ? true : false.

[CyrusNajmabadi]

What the tool reports is that the buggy path is not covered because it sees a ternary that looks like boolean_value ? true : false.

That's a problem with the tool. As far as the language, compiler, and runtime are concerned, all that code is reachable.

[dellamonica]

What the tool reports is that the buggy path is not covered because it sees a ternary that looks like boolean_value ? true : false.

That's a problem with the tool. As far as the language, compiler, and runtime are concerned, all that code is reachable.

Right, but then one line that is always covered becomes several which are equivalent, but not always covered. When I started the discussion, I did not have a clear idea where to put the "blame". At this point I think it is probably on the decompilation, not the coverage tool, not the lang, compiler or runtime.

[omariom]

It's lowered to this IL.
I don't think it can be lowered to anything more efficient.

What you see is C# code "lifted" by decompiler from IL.

[dellamonica]

The point, from the beginning was a superfluous ternary

If it is superfluous, then it isn't efficient.

It doesn't matter if it gets simplified at the time it matters, it was never my point of contention, so I don't quite understand why insist on it.

especially of a system or machine) achieving maximum productivity with minimum wasted effort or expense.

leading the code coverage tool to consider an additional branch in its analysis.

This is an issue with your tool. The false branches exist and are reachable.

The branch is an artifact of compilation, it is not there on the original code. In fact, by rewriting the code slightly, I got rid of the branch in the decompilation and the branch coverage is satisfied.

[CyrusNajmabadi]

The branch is an artifact of compilation, it is not there on the original code

It is there in the original code. Pattern matching has a true branch and a false branch. Because patterns are a test that can fail and produce false. That's what a pattern is.

[CyrusNajmabadi]

and the branch coverage is satisfied.

That's an issue with your coverage tool. Patterns have false branches. If you are not exercising then with your tests, that's lacking coverage.

it was never my point of contention, so I don't quite understand why insist on it.

Because we asked about what you wanted from IL, and you said 'efficiency' and then made several claims that things were not efficient. For example, that there was a "superfluous ternary" or that "It is not needed". I'm making this point because you continue making misstatements about what the compiler emits. Given the confusion there seems to be about what the roles of each tool is, and what is actually going on, it's relevant to provide information clearing up statements you're making that indicate confusion.

[dellamonica]

You are way off the mark. The code coverage tool does not consider "value is 1" or "value == 1" to be branches. The same way that it does not consider all possible integer values that can be passed to a function as branches.

The fact that it sees b?true:false is why it reports on a additional superfluous branch. If it is efficient or not was never a point I raised or cared.

Note that inside the Assert, which is not what I care about, there is indeed a false branch that is reachable.

[CyrusNajmabadi]

You are way off the mark. The code coverage tool

I'm not talking about, nor do i care at all about, the code coverage tool. :)

I'm explaining what a pattern is, which is something the language and compiler care about.

The same way that it does not consider all possible integer values that can be passed to a function as branches.

Same as above. The behavior of the coverage tool is not relevant to anything i'm saying.

The fact that it sees b?true:false is why it reports on a additional superfluous branch

It's not superfluous. The branches are reachable if you provide an appropriate test case that will hit the true or false branches. It sounds like you're missing a test that exercises the false branch, and htat's what the coverage tool is telling you.

All your concerns here relate entirely to that tool. :)

All my points relate entirely to the language and compiler the topic of this repo. These are different topics and the stuff you keep bringing up has no relevance currently to this repo.

[CyrusNajmabadi]

Closing as all discussion indicates that the user is complaining about external tools (a particular decompiler, and a particular code coverage tool.

No claim against Roslyn had been levied, and the user themselves indicates that the IL emitted by Roslyn is efficient and that they are not asking for a change there.

As such, this is not actionable by Roslyn (as this was not actionable by the language design team).

@dellamonica please report your issues with these particular tools to their respective owners. Just because you are working with c# does not mean the issue lies with the language or our compiler.

--

Locking as well as this is going in circles and there doesn't seem to be any willingness to understand the distinction/responsibilities of all the pieces at play here.