[BUG] HasValue of Nullable<T> is not taken into account

[mu88]

Describe the bug
After ensuring that an instance of Nullable<T> is not null via .HasValue, coverlet still assumes the instance to be nullable and therefore the branch coverage indicates a missing test branch.

To Reproduce

public static class ServiceExtensions
{
    public static bool IsInPast(this IService service)
    {
        Nullable<TimeOnly> timeFromService = service.GetTime();
        if (!timeFromService.HasValue) return true;

        bool isInPast = timeFromService < TimeOnly.FromDateTime(DateTime.UtcNow);

        return isInPast;
    }
}

Here you find a complete repro case.

Expected behavior

Branch coverage is 100% for the line var isInPast = timeFromService < TimeOnly.FromDateTime(DateTime.UtcNow);.

Actual behavior

Coverlet complains that only 1 out of 2 branches is tested as it assumes that timeFromService can still be null:

Configuration (please complete the following information):
Please provide more information on your .NET configuration:

• Which coverlet package and version was used? → coverlet.msbuild v6.0.2
• Which version of .NET is the code running on? → .NET 8
• What OS and version, and what distro if applicable? → Windows 10, 22H2
• What is the architecture (x64, x86, ARM, ARM64)? → x64
• Do you know whether it is specific to that configuration? → it is not specific to that configuration

Additional context

Using timeFromService.Value rather than timeFromService can be used as a workaround.

❗ Please also read Known Issues

[Thomas-Shephard]

I don't think this is a bug in Coverlet. By not using the .Value property when reading the value of timeFromService, the C# generated during the lowering process adds an additional check to ensure that it has a value when it is used. As you have already checked that it has a value, the branch that exists where it does not have a value at this later point is never covered.

In other words, your code

public static bool IsInPast(this IService service)
{
    var timeFromService = service.GetTime();
    if (!timeFromService.HasValue) return true;

    var isInPast = timeFromService < TimeOnly.FromDateTime(DateTime.UtcNow);

    return isInPast;
}

becomes

public static bool IsInPast(this IService service)
{
    TimeOnly? timeFromService = service.GetTime();
    if (!timeFromService.HasValue)
        return true;
    TimeOnly? nullable = timeFromService;
    TimeOnly timeOnly = TimeOnly.FromDateTime(DateTime.UtcNow);
    return nullable.HasValue && nullable.GetValueOrDefault() < timeOnly;
}

As you can see the additional .HasValue call in the last line of the lowered version is the culprit of this unexpected lack of coverage.

You could rectify this lack of branch coverage by using the .Value property and simply editing your code to be as follows.

public static bool IsInPast(this IService service)
{
    var timeFromService = service.GetTime();
    if (!timeFromService.HasValue) return true;

    var isInPast = timeFromService.Value < TimeOnly.FromDateTime(DateTime.UtcNow);

    return isInPast;
}

[mu88]

@Thomas-Shephard thank you for your reply 🤜🏻🤛🏻

Using timeFromService.Value rather than timeFromService can be used as a workaround.

Am I missing something or what is the difference between your suggested workaround and what I already wrote in the question? 🤔

I don't think this is a bug in Coverlet.

One may have different opinions about that, but I do understand that as a maintainer, one tends to not see it as a bug, whereas as a user I perceive it as a problem. Maybe we can agree that the null check has already been done before, so calling up .Value is actually unnecessary and we call it an unpleasantness? Maybe a Roslyn check/fix provided by coverlet would be an option here to check for such patterns?