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 code that follows [DoesNotReturn] from code coverage (per #898) #904

Conversation

kevin-montrose
Copy link
Contributor

@kevin-montrose kevin-montrose commented Jul 20, 2020

Per #898

Implementing reachability analysis, which will allow instructions that follow calls to methods that never return to be excluded from coverage metrics.

Determines what methods will return with [DoesNotReturn], though that is configurable.
Adds a test that explores a few "odd" patterns.

Though this seems to work, there are a few open questions:

  • Preferred way to punch configuration of this attribute in?
    • The bits are there for setting in on Coverage, but the last mile (how to set it from the command line, build tasks, etc.) needs to be done
  • Test suite is really fragile, especially around multiple tests locking each other out of the same files - is there a better way to run them?
    • I've been using the contributing instructions and killing dotnet build-server shutdown (with copious direct killing of dotnet and MSBuild process that VS spins up) and there are still some tests (like Coverlet.Core.Instrumentation.Tests.InstrumenterTests.TestInstrumentCoreLib) that just will not run
  • Reachability does not distinguish between "unreachable because of method call" and "unreachable because the IL is actually unreachable"
    • The compiler is... disinclined to produce unreachable IL, but this is a real (potentially) significant change
  • Related, reachability is always calculated and used - should it be ignored if there are no calls to [DoesNotReturn] methods?
  • "Odder" unconditional branches are not handled (leave, leave_s, jmp, etc.) yet
  • Naming and code layout things
    • There are several new classes (mostly nested), where should these go?
    • There's yet-another-block-class, what should it be named? Are there existing classes that can be used instead?

Finally, there are performance concerns. Current approach doesn't go wild with allocations or an insane number of passes or anything, but could probably be improved.

…t follow calls to methods that never return to be excluded from coverage metrics.

Determines what methods will return with [DoesNotReturn], though that is configurable.
Adds a test that explores a few "odd" patterns.

There are a number of open questions:
 - Preferred way to punch configuration of this attribute in?
 - Test suite is really fragile, especially around multiple tests locking each other out of the same files - is there a better way to run them?
 - Reachability does not distinguish between "unreachable because of method call" and "unreachable because the IL is actually unreachable"
   * The compiler is... disinclined to produce unreachable IL, but this is a real (potentially) significant change
 - Related, reachability is always calculated and used - should it be ignored if there are no calls to [DoesNotReturn] methods?
 - "Odder" unconditional branches are not handled (leave, leave_s, jmp, etc.) yet
 - Naming and code layout things
   * There are several new classes (mostly nested), where should these go?
   * There's yet-another-block-class, what should it be named?  Are there existing classes that can be used instead?

Finally, there are performance concerns.  Current approach doesn't go wild with allocations or an insane number of passes or anything, but could probably be improved.
@MarcoRossignoli MarcoRossignoli added enhancement General enhancement request tenet-coverage Issue related to possible incorrect coverage labels Jul 20, 2020
@kevin-montrose
Copy link
Contributor Author

kevin-montrose commented Jul 20, 2020

So these failures are much easier to understand than what I was seeing locally. Appears that this Resolve is the where "file in use" is killing things.

@MarcoRossignoli maybe I should be treating corelib specially? Or is there something else I should consider.

@MarcoRossignoli
Copy link
Collaborator

MarcoRossignoli commented Jul 20, 2020

Thanks Kevin I'll take a deep look asap(bit busy moment) cc: @tonerdo @petli for double check

@kevin-montrose
Copy link
Contributor Author

kevin-montrose commented Jul 21, 2020

Poking around a little more, the root issue seems to be that I'm trying to resolve a type that is defined in a module that is being actively instrumented. I think if I instead build a list of [DoesNotReturn] methods in the current module and check against that list instead of Resolve()'ing when the referenced method is defined in the current module it'll go away.

Will see if I can get that hacked up in the next day or two.

… before we start instrumenting them. Trying to do it in the same pass inevitably leads to issues with Resolve.

Also adds tests for generic methods and generic types, which were not functional before.
Has been tested against https://github.com/kevin-montrose/Cesil , and confirmed to work.
…e that gracefully and log as a warning. Also log when methods are first found to not return
@kevin-montrose
Copy link
Contributor Author

kevin-montrose commented Jul 25, 2020

Ultimately ended up doing a read-only pass over a module before instrumentation starts to avoid any need to call .Resolve() during actual instrumentation. This removes concerns around trying to open an assembly file that's being modified, but does introduce another pass over the module.

There's still a chance that resolving fails (if, for example, the assembly referenced cannot be found). This seems to happen in some integrations tests, which might just be an error in their configuration (seen in these logs). I chose to assume those methods always return - whether that is correct is debatable. Relevant logic is here if it should change.

I expanded the new test case to include generic methods and methods on generic types, which did not work in my first pass at this.

I've also run this against the personal project that prompted #898, and confirmed this PR addresses my original issue:
image

@MarcoRossignoli
Copy link
Collaborator

MarcoRossignoli commented Jul 27, 2020

Thanks again Kevin I'm on vacation this weeks I'll take a look asap 🙇

@kevin-montrose kevin-montrose changed the title Exclude code that follows [DoesNotReturn] from code cover (per #898) Exclude code that follows [DoesNotReturn] from code coverage (per #898) Aug 9, 2020
@MarcoRossignoli
Copy link
Collaborator

MarcoRossignoli commented Aug 19, 2020

Preferred way to punch configuration of this attribute in?
The bits are there for setting in on Coverage, but the last mile (how to set it from the command line, build tasks, etc.) needs to be done

You mean a way to opt-out in case of bugs?At the moment we don't have settings for coverage engine...ops sorry we have new one added last PR only to skip autoprops(but it's related to needs of user to avoid a % increase decrease for autoprops, make sense). Btw could be better avoid too much flags, are you concerned about some ambiguity on some custom user DoesNotReturn attribute?

Test suite is really fragile, especially around multiple tests locking each other out of the same files - is there a better way to run them?

To test in local usually works well

git clean -fdx
dotnet restore
dotnet pack
dotnet test

Btw for testing you should add some tests like in file https://github.com/coverlet-coverage/coverlet/pull/904/files#diff-b0c2dc6aa3b37bd4720bfeea133be676 (I pushed a file sample to speedup)
In this way you can debug step-by-step inside core code, test/debug instrumentation is a bit complex due to invasive nature(rewrite/rollback at the end).
If you run test inside vs at the end you'll see live report generation for fast check.

Reachability does not distinguish between "unreachable because of method call" and "unreachable because the IL is actually unreachable"
The compiler is... disinclined to produce unreachable IL, but this is a real (potentially) significant change
Related, reachability is always calculated and used - should it be ignored if there are no calls to [DoesNotReturn] methods?

I think we should apply only in case of call to DoesNotReturn if doable with low possible noise.

"Odder" unconditional branches are not handled (leave, leave_s, jmp, etc.) yet

Ok

Naming and code layout things
There are several new classes (mostly nested), where should these go?
There's yet-another-block-class, what should it be named? Are there existing classes that can be used instead?

Create a new file under Instrumentation folder called for instance ReachabilityHelper.cs and add all classes you need under Coverlet.Core.Instrumentation.Reachability namespace. It's a complex feat feel free to add needed object for it.

Finally, there are performance concerns. Current approach doesn't go wild with allocations or an insane number of passes or anything, but could probably be improved.

I'm ok with it...we have other place with that problem and an open issue #836

…rather than mixing and matching) and remove _most_ LINQ (clarifies some code, and performs a little better).
…ssary to support Leave(_S) in the non-exception cases, since it's legal for the compiler to emit those in a try{} even if they don't "leave".
…s an unconditional branch (in the same way these other terms are branches, anyway).
…he TestReachabilityHelper test working in VS.
@kevin-montrose
Copy link
Contributor Author

kevin-montrose commented Aug 21, 2020

Took a little longer than I'd hoped @MarcoRossignoli , but I've caught this PR back up to master and made some changes.

Preferred way to punch configuration of this attribute in?
The bits are there for setting in on Coverage, but the last mile (how to set it from the command line, build tasks, etc.) needs to be done

You mean a way to opt-out in case of bugs?At the moment we don't have settings for coverage engine...ops sorry we have new one added last PR only to skip autoprops(but it's related to needs of user to avoid a % increase decrease for autoprops, make sense). Btw could be better avoid too much flags, are you concerned about some ambiguity on some custom user DoesNotReturn attribute?

I tried to follow the pattern I saw with taking the attribute names explicitly into Instrumenter.

I'll take a look at the SkipAutoProps commit and see if I can puzzle out the last mile. It seems reasonable to allow the same flexibility with these attributes as is allowed with the others.

Reachability does not distinguish between "unreachable because of method call" and "unreachable because the IL is actually unreachable"
The compiler is... disinclined to produce unreachable IL, but this is a real (potentially) significant change
Related, reachability is always calculated and used - should it be ignored if there are no calls to [DoesNotReturn] methods?

I think we should apply only in case of call to DoesNotReturn if doable with low possible noise.

I have implemented this behavior - I now bail out early if no calls to [DoesNotReturn] methods are found.

"Odder" unconditional branches are not handled (leave, leave_s, jmp, etc.) yet

Ok

In testing, I found it was actually really important to implement support for Leave and Leave_S. Without support everything around try{ }-blocks got really wonky, as unconditional branches get swapped out for leaves most of the time.

So I did implement them, and ended up implementing endfilter and endfinally as well to make reachability in catch { } and finally { } blocks work correctly. I added two tests cases for them.

The remaining "weird" instructions I can think of are jmp, throw, and rethrow. I suspect all three are actually fine, since they all terminate control flow, but I will spend a little more time thinking and testing to confirm.

Naming and code layout things
There are several new classes (mostly nested), where should these go?
There's yet-another-block-class, what should it be named? Are there existing classes that can be used instead?

Create a new file under Instrumentation folder called for instance ReachabilityHelper.cs and add all classes you need under Coverlet.Core.Instrumentation.Reachability namespace. It's a complex feat feel free to add needed object for it.

I have moved everything into the file \src\coverlet.core\Instrumentation\ReachabilityHelper.cs and the namespace Coverlet.Core.Instrumentation.Reachability.

Finally, there are performance concerns. Current approach doesn't go wild with allocations or an insane number of passes or anything, but could probably be improved.

I'm ok with it...we have other place with that problem and an open issue #836

Alright, good to know. I did a little bit of de-LINQ-ing and just general cleanup (mostly unifying on immutable collections, instead of mixing immutable and mutable collections), so hopefully I'm not making things too much worse.

@kevin-montrose
Copy link
Contributor Author

kevin-montrose commented Aug 21, 2020

Took a whack at the settings and documentation. Seems to work for me, but I am not very well versed in dotnet tools, msbuild, or customizing VS test runners so I'm not very confident in that.

Copy link
Collaborator

@MarcoRossignoli MarcoRossignoli left a comment

Some nits but here we go.
I need to take another deep review loop on reachability helper, do some integration test(for new parameters usually I did a manual test to be sure that values flow correctly, but in future we'll add also automatic integration) and after that we can merge.
Thanks a lot!

Copy link
Collaborator

@MarcoRossignoli MarcoRossignoli left a comment

Thanks and sorry for the delay 🙇 busy summer

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

Successfully merging this pull request may close these issues.

None yet

2 participants