Bug 14855 – -cov should ignore assert(0)

This code void main() { } void foo(int val) { assert(val >= 0 && val <= 5); switch(val) { case 0: /* do something*/ break; case 1: /* do something*/ break; case 2: /* do something*/ break; case 3: /* do something*/ break; case 4: /* do something*/ break; case 5: /* do something*/ break; default: assert(0); } } unittest { foreach(i; 0 .. 6) foo(i); } results in this coverage file: |void main() |{ |} | |void foo(int val) |{ 12| assert(val >= 0 && val <= 5); 6| switch(val) | { 2| case 0: /* do something*/ break; 2| case 1: /* do something*/ break; 2| case 2: /* do something*/ break; 2| case 3: /* do something*/ break; 2| case 4: /* do something*/ break; 2| case 5: /* do something*/ break; 0000000| default: assert(0); | } |} | |unittest |{ 21| foreach(i; 0 .. 6) 6| foo(i); |} q.d is 90% covered Clearly, dmd -cov does not consider this to have 100% coverage, because the line with assert(0) never ran. However, the line with assert(0) shouldn't even run. There's a serious bug in the program if it does, and it will kill the program (interestingly enough, that doesn't stop it from generating a coverage report, even with -release, and if the line does run, it'll show that in the coverage report). But it's code that should _not_ be covered and where by definition, there is a problem if it runs. This is one of the chief issues which prevents programs from being able to reach 100% code coverage even when they're properly tested, and it forces anyone with such code who wants to make sure that they've properly covered their code to manually verify that each uncovered line has an assert(0) on it (or one of the other few cases where a line is uncovered and shouldn't be covered). Whereas if assert(0) were treated as covered (or ignored completely by the coverage analyzer), then such a program would be at 100% code coverage, and no manual verification would be needed. So, the current behavior is a major obstacle in being able to automatically detect 100% code coverage with tools - or even for a programmer to quickly check it themselves. And if we want to be able to have stuff like the auto-tester verify 100% code coverage, then we need to make it so that the cases which should _not_ be covered are not taken into account when calculating the code coverage.

*** Issue 15327 has been marked as a duplicate of this issue. ***

I understand that 100% coverage is a great goal. But assert(0) is executable code and so it is correctly in the coverage report. Being concerned about assert(0)'s not being executed is placing too much emphasis on the metric.

(In reply to Walter Bright from comment #2) > Being concerned about assert(0)'s not being executed is placing too > much emphasis on the metric. Perhaps, but if you're trying to make sure that you tested every line that you're supposed to test, assert(0) is not on the list of lines that should ever be run. In fact, if the code is correct, it shouldn't even be testable, because it shouldn't be reachable. So, if the point of the code coverage metric is to show whether your tests actually run every reachable line of code, then including assert(0) in the total is incorrect and counterproductive. It's certainly not the end of the world, but I don't see why there is any value in counting assert(0) in the coverage, and there's definitely value in not counting it.

> I understand that 100% coverage is a great goal. But assert(0) is executable code and so it is correctly in the coverage report. Maybe we can add a compiler flag or API in Druntime to change the behavior based on a user's opinion?

> Maybe we can add a compiler flag or API in Druntime to change the behavior based on a user's opinion? Much as I would love to see assert(0) not counted towards the coverage, I think that we should avoid extra compiler flags for stuff like this, and Walter is generally opposed to adding compiler flags without a really good reason, so I'd be pretty shocked if he'd agree to one for this. If he were convinced that it mattered enough to consider it, he'd just agree to change the normal behavior. At this point, expect that the options are: 1. Just put up with how things are and fail to get 100% code coverage in a number of cases because of this issue. 2. Someone other than Walter implements it (which is often the case with compiler changes anyway). While he may not think that it's worth doing himself, he may not be opposed enough to the idea to block it if someone else did the work. If I worked on the compiler normally, I'd probably just make the change and see if I could get it through, but what little time I have for working on official D stuff goes towards the standard library. 3. Use a different code coverage tool. I don't know how possible that would be with dmd, but it should work with gdc or ldc. 4. Write a tool for tweaking the code coverage results from dmd to work around corner cases like this that Walter doesn't think should be fixed. It does seem kind of icky to massage the results like that, but it would provide a way to work around the problem.

(In reply to Jonathan M Davis from comment #5) > > Maybe we can add a compiler flag or API in Druntime to change the behavior based on a user's opinion? > > Much as I would love to see assert(0) not counted towards the coverage, I > think that we should avoid extra compiler flags for stuff like this, and > Walter is generally opposed to adding compiler flags without a really good > reason, so I'd be pretty shocked if he'd agree to one for this. If he were > convinced that it mattered enough to consider it, he'd just agree to change > the normal behavior. > > At this point, expect that the options are: > > 1. Just put up with how things are and fail to get 100% code coverage in a > number of cases because of this issue. > I'd say just put up with it. Code that we *know* is unreachable can only be considered good if it's never hit. :-) There are always ways to refactor code so that you get 100% hit without compromising code correctness. Such as using enforce + final switch. > 3. Use a different code coverage tool. I don't know how possible that would > be with dmd, but it should work with gdc or ldc. > Different coverage tools would still have the same outcome though.

To me it doesn't make sense that `assert(0);` should be counted towards coverage. I mean, `assert(0);` is basically the programmer telling the compiler "this is not supposed to happen" and "this is unreachable code", meaning that if execution ever gets to this point, the code is wrong and the program is in an invalid state. On the flip side, if everything else except assert(0) were reached during execution, then surely that means the code is 100% covered, since the assert(0)'s are not supposed to ever happen, from the language's POV.

This issue was already fixed at some point in time.

Yes fixed in https://github.com/dlang/dmd/pull/12264. https://issues.dlang.org/show_bug.cgi?id=21630 was a dup. *** This issue has been marked as a duplicate of issue 21630 ***