Code review feedback. Making the new MetadataUpdater.GetCapabilities() internal.

Fixed the argument checking in coreclr's MetadataUpdater.ApplyUpdate().

Author: Mike McLaughlin

src/coreclr/System.Private.CoreLib/src/ILLink/ILLink.Substitutions.xml

@@ -6,8 +6,5 @@
     <type fullname="Internal.Runtime.InteropServices.ComponentActivator" feature="System.Runtime.InteropServices.EnableConsumingManagedCodeFromNativeHosting" featurevalue="false">
       <method signature="System.Boolean get_IsSupported()" body="stub" value="false" />
     </type>
-    <type fullname="System.Reflection.Metadata.MetadataUpdater" feature="System.Reflection.Metadata.MetadataUpdater.IsSupported" featurevalue="false">
-      <method signature="System.Boolean get_IsSupported()" body="stub" value="false" />
-    </type>
   </assembly>
 </linker>

src/coreclr/System.Private.CoreLib/src/System/Reflection/Metadata/MetadataUpdater.cs

@@ -28,18 +28,15 @@ public static class MetadataUpdater
         /// <param name="metadataDelta">The metadata changes to be applied.</param>
         /// <param name="ilDelta">The IL changes to be applied.</param>
         /// <param name="pdbDelta">The PDB changes to be applied.</param>
+        /// <exception cref="ArgumentException">The assembly argument is not a runtime assembly.</exception>
         /// <exception cref="ArgumentNullException">The assembly argument is null.</exception>
+        /// <exception cref="InvalidOperationException">The assembly is not editable.</exception>
         /// <exception cref="NotSupportedException">The update could not be applied.</exception>
         public static void ApplyUpdate(Assembly assembly, ReadOnlySpan<byte> metadataDelta, ReadOnlySpan<byte> ilDelta, ReadOnlySpan<byte> pdbDelta)
         {
-            if (assembly == null)
-            {
-                throw new ArgumentNullException(nameof(assembly));
-            }
-
-            RuntimeAssembly? runtimeAssembly = assembly as RuntimeAssembly;
-            if (runtimeAssembly == null)
+            if (assembly is not RuntimeAssembly runtimeAssembly)
             {
+                if (assembly is null) throw new ArgumentNullException(nameof(assembly));
                 throw new ArgumentException(SR.Argument_MustBeRuntimeAssembly);
             }
 
@@ -56,7 +53,7 @@ public static void ApplyUpdate(Assembly assembly, ReadOnlySpan<byte> metadataDel
         /// <summary>
         /// Returns the metadata update capabilities.
         /// </summary>
-        public static string GetCapabilities() => "Baseline AddMethodToExistingType AddStaticFieldToExistingType AddInstanceFieldToExistingType NewTypeDefinition ChangeCustomAttributes";
+        internal static string GetCapabilities() => "Baseline AddMethodToExistingType AddStaticFieldToExistingType AddInstanceFieldToExistingType NewTypeDefinition ChangeCustomAttributes";
 
         /// <summary>
         /// Returns true if the apply assembly update is enabled and available.

src/libraries/System.Private.CoreLib/src/ILLink/ILLink.Substitutions.Shared.xml

@@ -27,5 +27,8 @@
     <type fullname="System.Resources.ResourceReader" feature="System.Resources.ResourceManager.AllowCustomResourceTypes" featurevalue="false">
       <method signature="System.Boolean get_AllowCustomResourceTypes()" body="stub" value="false" />
     </type>
+    <type fullname="System.Reflection.Metadata.MetadataUpdater" feature="System.Reflection.Metadata.MetadataUpdater.IsSupported" featurevalue="false">
+      <method signature="System.Boolean get_IsSupported()" body="stub" value="false" />
+    </type>
   </assembly>
 </linker>

src/libraries/System.Runtime.Loader/ref/System.Runtime.Loader.cs

@@ -15,7 +15,6 @@ public static partial class AssemblyExtensions
     public static partial class MetadataUpdater
     {
         public static void ApplyUpdate(Assembly assembly, ReadOnlySpan<byte> metadataDelta, ReadOnlySpan<byte> ilDelta, ReadOnlySpan<byte> pdbDelta) { throw null; }
-        public static string GetCapabilities() { throw null; }
         public static bool IsSupported { get { throw null; } }
     }
     [System.AttributeUsage(System.AttributeTargets.Assembly, AllowMultiple = true)]

src/libraries/System.Runtime.Loader/tests/ApplyUpdateTest.cs

@@ -111,11 +111,19 @@ public static void ApplyUpdateInvalidParameters()
         [Fact]
         public static void GetCapabilities()
         {
-            string result = MetadataUpdater.GetCapabilities();
+            var ty = typeof(System.Reflection.Metadata.MetadataUpdater);
+            var mi = ty.GetMethod("GetCapabilities", BindingFlags.NonPublic | BindingFlags.Static, Array.Empty<Type>());
+
+            Assert.NotNull(mi);
+
+            var result = mi.Invoke(null, null);
+
             Assert.NotNull(result);
+            Assert.Equal(typeof(string), result.GetType());
         }
 
         [Fact]
+        [ActiveIssue("Returns true on mono", TestRuntimes.Mono)]
         public static void IsSupported()
         {
             bool result = MetadataUpdater.IsSupported;

src/libraries/System.Runtime.Loader/tests/ApplyUpdateUtil.cs

@@ -48,10 +48,16 @@ internal static bool CheckSupportedMonoConfiguration()
 
         internal static bool HasApplyUpdateCapabilities()
         {
-            string caps = MetadataUpdater.GetCapabilities();
+            var ty = typeof(MetadataUpdater);
+            var mi = ty.GetMethod("GetCapabilities", BindingFlags.NonPublic | BindingFlags.Static, Array.Empty<Type>());
+
+            if (mi == null)
+                return false;
+
+            var caps = mi.Invoke(null, null);
 
             // any non-empty string, assumed to be at least "baseline"
-            return caps.Length > 0;
+            return caps is string {Length: > 0};
         }
 
         private static System.Collections.Generic.Dictionary<Assembly, int> assembly_count = new();

src/mono/System.Private.CoreLib/src/ILLink/ILLink.Substitutions.xml

@@ -1,5 +1,2 @@
 <linker>
-    <type fullname="System.Reflection.Metadata.MetadataUpdater" feature="System.Reflection.Metadata.MetadataUpdater.IsSupported" featurevalue="false">
-      <method signature="System.Boolean get_IsSupported()" body="stub" value="false" />
-    </type>
 </linker>

src/mono/System.Private.CoreLib/src/System/Reflection/Metadata/MetadataUpdater.cs

@@ -21,7 +21,9 @@ public static class MetadataUpdater
         /// <param name="metadataDelta">The metadata changes to be applied.</param>
         /// <param name="ilDelta">The IL changes to be applied.</param>
         /// <param name="pdbDelta">The PDB changes to be applied.</param>
+        /// <exception cref="ArgumentException">The assembly argument is not a runtime assembly.</exception>
         /// <exception cref="ArgumentNullException">The assembly argument is null.</exception>
+        /// <exception cref="InvalidOperationException">The assembly is not editable.</exception>
         /// <exception cref="NotSupportedException">The update could not be applied.</exception>
         public static void ApplyUpdate(Assembly assembly, ReadOnlySpan<byte> metadataDelta, ReadOnlySpan<byte> ilDelta, ReadOnlySpan<byte> pdbDelta)
         {
@@ -45,7 +47,7 @@ public static void ApplyUpdate(Assembly assembly, ReadOnlySpan<byte> metadataDel
             }
         }
 
-        public static string GetCapabilities() => s_ApplyUpdateCapabilities.Value;
+        internal static string GetCapabilities() => s_ApplyUpdateCapabilities.Value;
 
         public static bool IsSupported { get; } = ApplyUpdateEnabled() != 0;