-
Notifications
You must be signed in to change notification settings - Fork 663
Description
Altough some work was already done addressing this issue in the general case in #2305, this is not enough to completely get rid of unwanted not-covered code in the F# case.
Detailed Description
#2305 solved the issue of having generated code counted in code coverage (obviously as not covered) by decorating the generated GitVersionInformation class with the ExcludeFromCodeCoverage attribute.
However, the F# compiler generates additional types when compiling a module. Here is what ILSpy gives us when examining an F# assembly:
This $GitVersionInformation type was generated by the compiler, and unfortunately it does not retain the attributes of its source type.
In order to not be polluted by these generated types during code coverage analysis, having the compiler not generate these types is one way to go. By having the F# module replaced by a class with static members, it appears we can achieve this goal. See the implementation section for details.
Possible Implementation
Here is what the generated F# class could look like:
namespace global
[<AbstractClass; Sealed>]
[<global.System.Runtime.CompilerServices.CompilerGenerated>]
[<global.System.Diagnostics.CodeAnalysis.ExcludeFromCodeCoverage>]
type GitVersionInformation =
static member Major
with [<global.System.Runtime.CompilerServices.CompilerGenerated; global.System.Diagnostics.DebuggerNonUserCode>] get() = "1"
static member Minor
with [<global.System.Runtime.CompilerServices.CompilerGenerated; global.System.Diagnostics.DebuggerNonUserCode>] get() = "2"
static member Patch
with [<global.System.Runtime.CompilerServices.CompilerGenerated; global.System.Diagnostics.DebuggerNonUserCode>] get() = "3"Notes:
- The
namespace globalis needed: F# does not like the class being in an not-explicitely set namespace. - The pair of attributes in front of each getter is very verbose, but I added them in order to have the code generated by the F# compiler as close as possible to the
module-based one. - Generating this implies both a modification of the template and of the logic that fills the template, but it seems not that difficult.
Compatibility concerns
Similarly to issue #2310 I wondered if that was going to be a breaking change. Well it seems it is not, at least from a C# consumer perspective (I'm not sure if there is special semantics in F# that would work with a module and not with a class):
Both the current module-based and the proposed templates generate the same result when decompiled with ILSpy; something like that:
// GitVersionInformation
using Microsoft.FSharp.Core;
using System.Diagnostics;
using System.Runtime.CompilerServices;
[AbstractClass]
[Sealed]
[CompilerGenerated]
[CompilationMapping(/*Could not decode attribute arguments.*/)]
public static class GitVersionInformation {
public static string Major {
[CompilerGenerated, DebuggerNonUserCode]
get { return "0"; }
}
public static string Minor {
[CompilerGenerated, DebuggerNonUserCode]
get { return "1"; }
}The only remaining difference is that the new template does not produce any $GitVersionInformation type. This type not existing anymore could be considered a compatibility difference if we are picky, but I honestly doubt anyone would try to access this compiler-generated thing by reflection...
A final issue
There is one last issue that I became aware of by examining all the versions of GitVersionInformation. In VB.NET and C#, the members of the class are static fields, but the F# version defines static readonly properties. Not consistent. But here again, choosing fields for the F# version would prove more breaking. Maybe we should live with different implementations of the GitVersionInformation type per source language until the next major version...
Generating nearly identical code at the IL level from the three languages may be possible (I just need to figure out how F# supports fields in classes), but is it desirable right now? And if we were to go down this path, I would favor using readonly properties rather than read/write fields.
