Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Exclude lines & branches that come after calls to methods with [DoesNotReturn] #898

Closed
kevin-montrose opened this issue Jul 12, 2020 · 6 comments
Labels
enhancement General enhancement request tenet-coverage Issue related to possible incorrect coverage

Comments

@kevin-montrose
Copy link
Contributor

kevin-montrose commented Jul 12, 2020

It'd be nice to be able to exclude lines, like return;, that follow methods that contractually never return from coverage. The BCL already ships with the DoesNotReturnAttribute that could be used to indicate this.

I stumble across this a lot when using the "throw helper"-pattern. An example from one my GitHub repos.

PoisonableBase<T> has a method like so:

internal void AssertNotPoisoned<T>(IBoundConfiguration<T> self)
{
    if (Poison != null)
    {
        switch (Poison.Value)
        {
            case PoisonType.Cancelled: 
                Throw.InvalidOperationException<object>("Object is in an invalid state, a previous operation was canceled"); 
                return;
            case PoisonType.Exception: 
                Throw.InvalidOperationException<object>("Object is in an invalid state, a previous operation raised an exception"); 
                return;
            default:
                Throw.ImpossibleException<object, T>($"Unexpected {nameof(PoisonType)}: {Poison}", self);
                return;
        }
    }
}

Each method on Throw never returns.

Even though I do have tests for each PoisonType, my reports will always include each return statement as uncovered:

  • image

For non-void methods I can hack around this restriction by pretending these methods return an object, but for void returning methods you're out of luck.


I suspect this could be hacked into Instrumentor.InstrumentIL, but maybe there's a pass elsewhere that would be a better place.

I think the logic to support this is something like:

  1. For every method body...
    a. Break the IL into runs based on branch targets
    b. In each run, any instructions following a call or callvirt to a method with [DoesNotReturn] is unreachable
    c. Record unreachable instructions somewhere
  2. When instrumenting instructions and branches, lookup reach-ability and skip unreachable instructions

I considered just looking for ret and throw and so on, but I've convinced myself fall-through will break that.

@MarcoRossignoli MarcoRossignoli added enhancement General enhancement request tenet-coverage Issue related to possible incorrect coverage labels Jul 13, 2020
@MarcoRossignoli
Copy link
Collaborator

MarcoRossignoli commented Jul 13, 2020

Thank's for reporting this, we have to understand if makes sense from coverage perspective, I mean that IL is emitted and not covered.
Also need to understand if make sense add a C# only feature, coverlet can instrument every IL code(we test/debug and develop using C# btw, so actually we're used to fix roslyn related compiler tricks cached delegates, async state machine etc...).

@kevin-montrose
Copy link
Contributor Author

kevin-montrose commented Jul 14, 2020

I think of it as analogous to [ExcludeFromCoverage], it's something code opts-in to because there's some additional information that can't (or shouldn't) be inferred. I think it'd be unreasonable to expect coverlet to automatically detect that called methods never return, unless some future version of .NET adds a never type or something.

I'm tempted to take a swing at PR'ing it, even if all I get out of it is my own edification.

@MarcoRossignoli
Copy link
Collaborator

MarcoRossignoli commented Jul 15, 2020

I think you're right and I agree, actually that members are inside System.Diagnostics.CodeAnalysis and not Microsoft.CodeAnalysis.CSharp so it's not language related, I don't know how complex is at the momet, but we could stop to instrument all sequence/branches after a call with method decorated like that.
It's not high priority but I keep this open for future implementation.
Thank's Kevin!
If you want to take a trip on code(not obligated at all) this is the point where we instrument sequence/branches https://github.com/coverlet-coverage/coverlet/blob/master/src/coverlet.core/Instrumentation/Instrumenter.cs#L504
Feel free to ask if you need help on testing/debugging

@tonerdo
Copy link
Collaborator

tonerdo commented Jul 19, 2020

If it's just to add special handling of the DoesNotReturnAttribute, I think that's something we should do transparently and document. Adding support for the "throw helper"-pattern is as @MarcoRossignoli a C#-ism and is out of scope of this project

@kevin-montrose
Copy link
Contributor Author

kevin-montrose commented Jul 20, 2020

Agreed, anyone using the throw helper pattern should just use the attribute explicitly.

I took a whack at an implementation in #904, though there are a fair number of open questions. I haven't yet run it on my original project due to lack of time (I'm hoping to do that in the near future), but there are some simple test methods.

@MarcoRossignoli
Copy link
Collaborator

MarcoRossignoli commented Sep 5, 2020

Closed in #904

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement General enhancement request tenet-coverage Issue related to possible incorrect coverage
Projects
None yet
Development

No branches or pull requests

3 participants