Fix reflection correctness edge cases

ApiSurface/ApiMember.fs

@@ -68,7 +68,7 @@ module ApiMember =
             let typeString = fieldType |> Type.toFullName
 
             if fieldInfo.IsLiteral then
-                let value = m.DeclaringType.GetField(m.Name).GetValue null |> string<obj>
+                let value = fieldInfo.GetValue null |> string<obj>
 
                 // Don't print `= ` for empty strings, as many editors/Git hooks trim trailing whitespace
                 if value = "" then

ApiSurface/ApiSurface.fs

@@ -30,9 +30,7 @@ module ApiSurface =
 
     /// In the rare case that you have several different baselines depending on what framework you are running under,
     /// you can use a more specific name for your baseline files.
-    let private frameworkBaselineFile =
-        let desc = RuntimeInformation.FrameworkDescription
-
+    let internal frameworkBaselineFileFor (desc : string) =
         if desc.StartsWith (".NET Core", StringComparison.Ordinal) then
             "SurfaceBaseline-NetCore.txt"
         elif desc.StartsWith (".NET Framework", StringComparison.Ordinal) then
@@ -42,10 +40,13 @@ module ApiSurface =
             let frameworkNumber = Regex(@"^\.NET ([0-9]+)\.").Match desc
 
             if frameworkNumber.Success then
-                sprintf "SurfaceBaseline-Net%s.txt" frameworkNumber.Groups.[0].Value
+                sprintf "SurfaceBaseline-Net%s.txt" frameworkNumber.Groups.[1].Value
             else
                 failwithf "Unknown runtime framework: %s" desc
 
+    let private frameworkBaselineFile =
+        RuntimeInformation.FrameworkDescription |> frameworkBaselineFileFor
+
     let surfaces (assembly : Assembly) =
         assembly.GetManifestResourceNames ()
         |> Seq.choose Option.ofObj

ApiSurface/ApiSurface.fsi

@@ -16,6 +16,8 @@ type internal Version =
 [<RequireQualifiedAccess>]
 module ApiSurface =
 
+    val internal frameworkBaselineFileFor : string -> string
+
     /// Read the SurfaceBaseline.txt embedded resource from the given assembly.
     [<CompiledName "FromAssemblyBaseline">]
     val ofAssemblyBaseline : Assembly -> ApiSurface

ApiSurface/DocCoverage.fs

@@ -27,13 +27,21 @@ module DocCoverage =
     let isPublic (memberInfo : MemberInfo) : bool =
         (isNull memberInfo.DeclaringType || memberInfo.DeclaringType.IsVisible)
         && match memberInfo.MemberType with
-           | MemberTypes.Method
-           | MemberTypes.Constructor -> let i = memberInfo :?> MethodInfo in i.IsPublic
-           | MemberTypes.Event -> let i = memberInfo :?> EventInfo in i.AddMethod.IsPublic
+           | MemberTypes.Method -> let i = memberInfo :?> MethodInfo in i.IsPublic
+           | MemberTypes.Constructor -> let i = memberInfo :?> ConstructorInfo in i.IsPublic
+           | MemberTypes.Event ->
+               let i = memberInfo :?> EventInfo
+
+               (not (isNull i.AddMethod) && i.AddMethod.IsPublic)
+               || (not (isNull i.RemoveMethod) && i.RemoveMethod.IsPublic)
            | MemberTypes.Field -> let i = memberInfo :?> FieldInfo in i.IsPublic
            | MemberTypes.TypeInfo
-           | MemberTypes.NestedType -> let i = memberInfo :?> TypeInfo in i.IsPublic
-           | MemberTypes.Property -> let i = memberInfo :?> PropertyInfo in i.GetMethod.IsPublic
+           | MemberTypes.NestedType -> let i = memberInfo :?> Type in i.IsVisible
+           | MemberTypes.Property ->
+               let i = memberInfo :?> PropertyInfo
+
+               (not (isNull i.GetMethod) && i.GetMethod.IsPublic)
+               || (not (isNull i.SetMethod) && i.SetMethod.IsPublic)
            | memberType -> failwithf "Unrecognised MemberType: %O" memberType
 
     let paramInfoToString (pi : ParameterInfo) : string =
@@ -263,7 +271,10 @@ module DocCoverage =
         let publicMembers = publicMembers |> Seq.map (fun (n, c, _) -> n, c) |> Set.ofSeq
 
         let nonPublicMembers =
-            nonPublicMembers |> Seq.map (fun (n, c, _) -> n, c) |> Set.ofSeq
+            nonPublicMembers
+            |> Seq.map (fun (n, c, _) -> n, c)
+            |> Set.ofSeq
+            |> fun members -> Set.difference members publicMembers
 
         DocCoverage (Path.GetFileName assembly.Location, Members.MixedAccess (publicMembers, nonPublicMembers))
 

ApiSurface/DocCoverage.fsi

@@ -9,6 +9,8 @@ type DocCoverage
 [<RequireQualifiedAccess>]
 module DocCoverage =
 
+    val internal isPublic : MemberInfo -> bool
+
     /// Map the exposed types and members of an assembly into a list of
     /// member names expected to be present in the corresponding .XML
     /// documentation file.

ApiSurface/Test/ApiSurface.Test.fsproj

@@ -9,6 +9,7 @@
   <ItemGroup>
     <Compile Include="RedirectOutput.fs" />
     <Compile Include="Sample.fs" />
+    <Compile Include="TestReflectionCorrectness.fs" />
     <Compile Include="TestDocCoverage.fs" />
     <Compile Include="TestSemanticVersioning.fs" />
     <Compile Include="TestSurface.fs" />

ApiSurface/Test/TestApiSurface.fs

@@ -89,3 +89,9 @@ module TestApiSurface =
 
         let expected = Sample.publicSurface |> Set.add "Bar" |> Set.add "Foo"
         actual |> shouldEqual expected
+
+    [<TestCase(".NET Core 3.1.0", "SurfaceBaseline-NetCore.txt")>]
+    [<TestCase(".NET Framework 4.8.0", "SurfaceBaseline-NetFramework.txt")>]
+    [<TestCase(".NET 8.0.0", "SurfaceBaseline-Net8.txt")>]
+    let ``Test framework baseline filename`` desc expected =
+        desc |> ApiSurface.frameworkBaselineFileFor |> shouldEqual expected

ApiSurface/Test/TestReflectionCorrectness.fs

@@ -0,0 +1,109 @@
+namespace ApiSurface.Test
+
+open System
+open System.Reflection
+open System.Reflection.Emit
+open NUnit.Framework
+open FsUnitTyped
+open ApiSurface
+
+[<TestFixture>]
+module TestReflectionCorrectness =
+
+    let private reflectionFixture =
+        lazy
+            (let assemblyName = AssemblyName "ApiSurface.Test.ReflectionFixture"
+
+             let assembly =
+                 AssemblyBuilder.DefineDynamicAssembly (assemblyName, AssemblyBuilderAccess.Run)
+
+             let moduleBuilder = assembly.DefineDynamicModule assemblyName.Name
+
+             let typeBuilder =
+                 moduleBuilder.DefineType ("ReflectionFixture", TypeAttributes.Public)
+
+             typeBuilder.DefineDefaultConstructor MethodAttributes.Public |> ignore
+
+             let emitReturn (methodBuilder : MethodBuilder) =
+                 methodBuilder.GetILGenerator().Emit OpCodes.Ret
+
+             let propertySetter =
+                 typeBuilder.DefineMethod (
+                     "set_WriteOnly",
+                     MethodAttributes.Public
+                     ||| MethodAttributes.SpecialName
+                     ||| MethodAttributes.HideBySig,
+                     typeof<Void>,
+                     [| typeof<int> |]
+                 )
+
+             emitReturn propertySetter
+
+             let property =
+                 typeBuilder.DefineProperty ("WriteOnly", PropertyAttributes.None, typeof<int>, Type.EmptyTypes)
+
+             property.SetSetMethod propertySetter
+
+             let eventRemover =
+                 typeBuilder.DefineMethod (
+                     "remove_RemoveOnly",
+                     MethodAttributes.Public
+                     ||| MethodAttributes.SpecialName
+                     ||| MethodAttributes.HideBySig,
+                     typeof<Void>,
+                     [| typeof<EventHandler> |]
+                 )
+
+             emitReturn eventRemover
+
+             let event =
+                 typeBuilder.DefineEvent ("RemoveOnly", EventAttributes.None, typeof<EventHandler>)
+
+             event.SetRemoveOnMethod eventRemover
+
+             let literal =
+                 typeBuilder.DefineField (
+                     "HiddenLiteral",
+                     typeof<int>,
+                     FieldAttributes.Private ||| FieldAttributes.Static ||| FieldAttributes.Literal
+                 )
+
+             literal.SetConstant 42
+
+             let nestedType =
+                 typeBuilder.DefineNestedType ("Nested", TypeAttributes.NestedPublic)
+
+             nestedType.CreateTypeInfo () |> ignore
+
+             typeBuilder.CreateTypeInfo().AsType ())
+
+    [<Test>]
+    let ``isPublic handles constructors`` () =
+        reflectionFixture.Value.GetConstructors().[0]
+        |> DocCoverage.isPublic
+        |> shouldEqual true
+
+    [<Test>]
+    let ``isPublic handles setter-only properties`` () =
+        reflectionFixture.Value.GetProperty "WriteOnly"
+        |> DocCoverage.isPublic
+        |> shouldEqual true
+
+    [<Test>]
+    let ``isPublic handles remove-only events`` () =
+        reflectionFixture.Value.GetEvent "RemoveOnly"
+        |> DocCoverage.isPublic
+        |> shouldEqual true
+
+    [<Test>]
+    let ``isPublic handles nested public types`` () =
+        reflectionFixture.Value.GetNestedType "Nested"
+        |> DocCoverage.isPublic
+        |> shouldEqual true
+
+    [<Test>]
+    let ``print handles private literals`` () =
+        reflectionFixture.Value.GetField ("HiddenLiteral", BindingFlags.NonPublic ||| BindingFlags.Static)
+        |> ApiMember.ofMemberInfo
+        |> ApiMember.print
+        |> shouldEqual "ReflectionFixture.HiddenLiteral [static field]: int = 42"