SteveGilham / altcover

Cross-platform coverage gathering and processing tool set for dotnet/.Net Framework and Mono
MIT License
498 stars 18 forks source link

BranchPoints differ between .NET Framework and .NET 7 when using interpolated strings #216

Closed Tim-Pohlmann closed 5 months ago

Tim-Pohlmann commented 5 months ago

I have the following sample code:

public class Sample
{
    public string Method(int i)
    {
        if (i > 0)
        {
            return $"Value: '{i}'";
        }
        else
            return "Negative";
    }
}

The sample project and the test project both target net48 and net7.0:

  <PropertyGroup>
    <TargetFrameworks>net48;net7.0</TargetFrameworks>
  </PropertyGroup>

Running dotnet test /p:AltCover=true will create two coverage files. The content, however, differs for the relevant code:

coverage.net48.xml:

              <BranchPoints>
                <BranchPoint vc="0" uspid="0" ordinal="0" offset="7" sl="5" path="0" offsetend="9" fileid="48" />
                <BranchPoint vc="0" uspid="1" ordinal="1" offset="7" sl="5" path="1" offsetend="29" fileid="48" />
              </BranchPoints>

coverage.net7.0.xml:

              <BranchPoints>
                <BranchPoint vc="0" uspid="0" ordinal="0" offset="7" sl="5" path="0" offsetend="9" fileid="63" />
                <BranchPoint vc="0" uspid="1" ordinal="1" offset="7" sl="5" path="1" offsetend="65" fileid="63" />
              </BranchPoints>

You can see that offsetend is higher for net7.0. I assume the reason is that .Net 7 is using DefaultInterpolatedStringHandler.

I do not fully understand what offset and offsetend represent, so I'm unsure whether this is a bug or intended.

Why is this relevant to me? We are aggregating the coverage reports to get a complete picture using some custom logic. It matches the BranchPoints based on its properties, and if they differ for the same piece of code, the algorithm doesn't work as intended. Is there a better way to aggregate the reports?

SteveGilham commented 5 months ago

The BranchPoint record is one of the more kitchen-sink parts of the OpenCover format, which I have faithfully reproduced in AltCover. The offset is the location of the branch instruction, relative to the start of the method; offsetend is the instruction where that particular option from the branch leads, possibly via intermediate unconditional branch instructions (noted in the offsetchain property, if that is set).

Building the sample code and decompiling, we see for the net48 code Screenshot 2024-04-15 110549 that the interpolation is a simple String.Format call using a compile-time generated format string, but for the net7.0 code Screenshot 2024-04-15 110846 there is a lot more code involved, where the output strung is stitched together in-line.

For this not-really-the-same-assembly merging, the better way to match up branch points would be just to compare sl - the source line at which the branch begins - and path - which option was chosen. That would be robust against most cases of different compiler-inlined logic, which might also include branches.

Tim-Pohlmann commented 5 months ago

Thanks for the answer; very insightful!

The problem with looking at path only is that, in some cases, the path is duplicated in the same report (particularly if you have two separate if statements on the same line).

Right now, I'm looking at uspid. While it is not perfect either, it seems more robust than relying on offset and offsetend.

SteveGilham commented 5 months ago

Certainly, whatever you end up doing here is going to involve heuristics of some sort, since they aren't quite the same assembly due to such compiler differences.

Of the other attributes, uspid is a count (from zero) of the branch points in that assembly; ordinal is a similar per-method count. Those would not be robust against a compiler inserted extra branch, should such a thing arise (e.g. potentially different IL from compiling a switch statement, especially one switching on string values).

In the general case, things are of course complicated by possible #if NET70 or #if NET48 blocks. These sorts of issues are why the Merge-OpenCover cmdlet and API match on assembly hash first.

SteveGilham commented 5 months ago

Taking no news as good news, and closing.

Tim-Pohlmann commented 5 months ago

Ah, sorry, I forgot to reply. Indeed, your in-depth posts helped us find an acceptable solution. Thanks a lot!