opencover icon indicating copy to clipboard operation
opencover copied to clipboard

Type declarations in F# are shown as not covered

Open frankshearar opened this issue 8 years ago • 13 comments

Please provide the following information when submitting an issue, where appropriate replace the [ ] with a [X]

My Framework

  • [ ] .NET 2
  • [ ] .NET 3.5
  • [ ] .NET 4
  • [ ] .NET 4.5
  • [ ] .NET 4.6
  • [ ] .NET 4.6.1
  • [X] .NET 4.6.2
  • [ ] .NET Core 1.0.0
  • [ ] Something else

My Environment

  • [ ] Windows 7 or below (not truly supported due to EOL)
  • [ ] Windows 8
  • [ ] Windows 8.1
  • [X] Windows 10
  • [ ] Windows 10 IoT Core
  • [ ] Windows Server 2012
  • [ ] Windows Server 2012 R2
  • [ ] Windows Server 2016

I have already...

  • [X] repeated the problem using the latest stable release of OpenCover.
  • [X] reviewed the usage guide and usage document.
  • [X] have looked at the opencover output xml file in an attempt to resolve the issue.
  • [X] reviewed the current issues to check that the issue isn't already known.

My issue is related to (check only those which apply):

  • [X] no coverage being recorded
  • [ ] 32 or 64 bit support
  • [ ] feature request

Expected Behavior

Either plain vanilla record type declarations show up "white" because they're entirely uninteresting (much like C# class declarations, or empty-bodied constructors that just call base), or green.

Actual Behavior

Record type declarations show up as being uncovered, in F#, even when all getters and constructors are invoked through tests.

Steps to reproduce the problem:

type Thing = { Thing: string }
let makeThing s = { Thing = s }

[<Test>]
let testMakeThing() =
    Assert.AreEqual("s", (makeThing "s").Thing)

which dotPeek will show through "Decompiled sources" as equivalent to the C#

[CompilationMapping(SourceConstructFlags.RecordType)]
[Serializable]
public sealed class Thing : IEquatable<Reader.Thing>, IStructuralEquatable, IComparable<Reader.Thing>, IComparable, IStructuralComparable
{
    [DebuggerBrowsable(DebuggerBrowsableState.Never)]
    internal string Thing\u0040;

    [CompilationMapping(SourceConstructFlags.Field, 0)]
    public string Thing
    {
        get
        {
          return this.Thing\u0040;
        }
    }

    public Thing(string thing)
    {
        this.Thing\u0040 = thing;
    }
}

public static Reader.Thing makeThing(string s)
{
  return new Reader.Thing(s);
}

(The \u0040 renders as an @ in dotPeek.)

The test instantiates a Thing, and runs makeThing(), and accesses the Thing's Thing property. However in the coverage.xml I see that visited="false" for the makeThing function. Ditto for Thing::get_Thing and Thing::.ctor

frankshearar avatar Jul 29 '16 05:07 frankshearar

Thanks, we'll investigate...

sawilde avatar Jul 29 '16 22:07 sawilde

Discriminated Union are also concerned with the problem.

module DU
    type MyUnion =
        | Foo of int
        | Bar of string

    let returnFoo v = Foo v

    let returnBar v = Bar v
module Test
    open Xunit
    open Swensen.Unquote
    open DU

    [<Theory>]
    [<InlineData(10)>]
    [<InlineData(20)>]
    let ``Test Foo`` (v) =
        test <@ returnFoo v = Foo v @>

    [<Theory>]
    [<InlineData("Foo")>]
    [<InlineData("Bar")>]
    let ``Test Bar`` (v) =
        test <@ returnBar v = Bar v @>

Methods returnFoo and returnBar are marked as visited but discriminated union MyUnion is marked as not covered.

Julien-Pires avatar Sep 07 '16 10:09 Julien-Pires

Taking a look at this with OpenCover 4.6.519 and NUnit 2.6.4, using VS2017 v15.3.3, building

namespace N
open NUnit.Framework
module M =
  type Thing = { Thing: string }
  let makeThing s = { Thing = s }

  [<Test>]
  let testMakeThing() =
    Assert.AreEqual("s", (makeThing "s").Thing)

in debug as a library. Note that the dotPeek decompilation above omits a number of [CompilerGenerated] methods from the Thing type.

After a successful test run, testmakeThing and makeThing each have their one sequence point as visited; Thing::.ctor's MethodPoint is visited (no sequence or branch points) while Thing::get_Thing and Thing::ToString have just an unvisited MethodPoint. None of these method points has a code location.

The two of the three generated and unvisited overloads Thing::CompareTo have a sequence point matching typename Thing on line 4, as does one of the Thing::GetHashCode overloads.

Using pdb2xml, the functions shown with sequence points are as above. Using cvdump we get for the record type of interest the same three functions

** Module: "N.M.Thing" from "FE11DE8D"

Library1\Library1.fs (None), 0001:00000023-00000031
, line/addr pairs = 1

      4:8    -    4:13    00000023

Library1\Library1.fs (None), 0001:00000031-0000007D
, line/addr pairs = 7

      4:8    -    4:13    00000031  feefee:0                00000045
  feefee:0                00000054  feefee:0                00000068
  feefee:0                0000006A  feefee:0                00000079
  feefee:0                0000007B

Library1\Library1.fs (None), 0001:0000007D-0000008A
, line/addr pairs = 1

      4:8    -    4:13    0000007D

For the union case, there are a lot more generated functions and types marked [CompilerGenerated] (everything bar a couple of factory methods NewFoo, NewBar); but the same trio (two CompareTo and a GetHashCode) are the only ones with source location and sequence points.

With the DU tests being cast as standard NUnit calls, without quotation magic, as

    [<Test>]
    let testMakeUnion() =
        Assert.AreEqual(returnFoo 10, Foo 10)
        Assert.AreEqual(returnBar "s", Bar "s")

I get visits for MyUnion::.ctor, MyUnion::NewFoo, MyUnion::NewBar,both MyUnion::Equals overloads, MyUnion/Foo::.ctor and MyUnion/Bar::.ctor -- i.e. what one would expect, knowing how the type is implemented under the hood.

~~Just for interest's sake, I tried the old dot-net-coverage tool, and that didn't even show the Thing type (or the MyUnion type) at all.~~

Personally, I would regard the constructor(= write once setter) and getter as being exactly like automatic properties are in C#, for both records and union datatypes in F# -- things that we know are correctly implemented and which just clutter the (un-)coverage report -- ~~and vote for the dot-net-coverage result being the ideal~~ and vote for excluding all the so-generated functions from the report.


dot-net-coverage just failed to handle nested types, rather than there being any deliberate behaviour here. Fixing that issue, we get the same results as OpenCover for those methods that have sequence points, but none of the method-point-only methods show.

SteveGilham avatar Aug 31 '17 14:08 SteveGilham

FWIW -- member functions attached to types do get sensible coverage reported against them; it's only the compiler-generated ones, the ones that don't have any source location, that don't get handled consistently.

SteveGilham avatar Aug 31 '17 16:08 SteveGilham

@SteveGilham thanks - I am with you on if it is autogenerated then do we really care - I think we need to hear back from the OP about why is valuable for them to know from a developer point of view

sawilde avatar Sep 01 '17 00:09 sawilde

Looking back at the "Expected Behaviour" statement

Either plain vanilla record type declarations show up "white" because they're entirely uninteresting (much like C# class declarations, or empty-bodied constructors that just call base), or green.

simply omitting them would be an acceptable option. It's having methods that are exercised in the test then showing up with zero coverage in the report that's the problem.

SteveGilham avatar Sep 01 '17 16:09 SteveGilham

What Steve said: all I want is to not have my stats skewed because "my code is not covered" when actually it's "code the compiler generated is not covered". I don't care about the coverage of the latter, but I would like the coverage of the former to be as close to 100% as I can get it :)

frankshearar avatar Sep 01 '17 16:09 frankshearar

By the uncovered branches shown in the ReportGenerator output, it is likely that it is showing the uncovered state of the CompareTo method which has a code-located sequence point rather than the actual constructor or accessor which lack same.

SteveGilham avatar Sep 03 '17 07:09 SteveGilham

I find by experiment that my code in PR #758 only handles the case where the record or union types are public.

Long story short, in lines 387-8

                    return y[2].Equals(1)  // sum
                        || y[2].Equals(2); // record

and line 401

                        .Any(x => x.GetBlob()[2] == 4); // Field

the blob-derived value needs to be masked by & 31 i.e. & SourceConstructFlags.KindMask before comparison with the constant, to switch off the SourceConstructFlags.NonPublicRepresentation bit (==32) that is set on non-public types.

I'll put together a PR with tests etc. in due course.

SteveGilham avatar Oct 01 '17 17:10 SteveGilham

thanks

sawilde avatar Oct 14 '17 07:10 sawilde

@SteveGilham I could never work out if this issue was fixed by your PR and you raised two other issues to cover the other cases?

sawilde avatar Jan 29 '21 10:01 sawilde

I don't remember following up on this -- the last quarter of '17 was rather hectic for me IRL. The repro for the last comment would be simply changing the visibility of the the types, thus

module internal DU =
  type internal MyUnion =
        | Foo of int
        | Bar of string
  let internal returnFoo v = Foo v
  type internal Thing = { Thing: string }
  let internal makeThing s = { Thing = s }

module Tests =
  [<Test>]
  let testMakeThing() =
      Assert.AreEqual("s", (DU.makeThing "s").Thing)

  [<Test>]
  let testFoo() =
      Assert.AreEqual(Foo 23, DU.returnFoo 23)

which yields, decompiling, the following attribute values

[CompilationMapping((SourceConstructFlags)33)]
internal abstract class MyUnion : IEquatable<MyUnion>, IStructuralEquatable, IComparable<MyUnion>, IComparable, IStructuralComparable

and

[CompilationMapping((SourceConstructFlags)34)]
internal sealed class Thing : IEquatable<Thing>, IStructuralEquatable, IComparable<Thing>, IComparable, IStructuralComparable

rather than the values 1 and 2 respectively for public types

SteveGilham avatar Jan 29 '21 11:01 SteveGilham

Thanks for the update

the last quarter of '17 was rather hectic for me IRL.

I feel it - I've had some ups and downs myself which meant I had to park work on OpenCover for a while

sawilde avatar Jan 29 '21 20:01 sawilde