Fix argument exception warnings on runtime (#35717)

* Fix argument exception warnings on runtime * Apply feedback * Addressing feedback and test fix * Fix test failures * Fix tests for NetFx run * Fix suppressed warning for socket.SendAsyn(e) and fix corresponding tests * Applied feedback
dotnet · May 15, 2020 · ee0aadc · ee0aadc
1 parent ca0fd16
commit ee0aadc
Show file tree

Hide file tree

Showing 99 changed files with 257 additions and 236 deletions.
diff --git a/.editorconfig b/.editorconfig
@@ -155,6 +155,7 @@ csharp_space_between_square_brackets = false
 
 # Analyzers
 dotnet_code_quality.ca1802.api_surface = private, internal
+dotnet_code_quality.CA2208.api_surface = public
 
 # C++ Files
 [*.{cpp,h,in}]

diff --git a/eng/CodeAnalysis.ruleset b/eng/CodeAnalysis.ruleset
@@ -106,7 +106,7 @@
       <Rule Id="CA2200" Action="Warning" />          <!-- Rethrow to preserve stack details. -->
       <Rule Id="CA2201" Action="None" />             <!-- Do not raise reserved exception types -->
       <Rule Id="CA2207" Action="Warning" />          <!-- Initialize value type static fields inline -->
-      <Rule Id="CA2208" Action="None" />             <!-- Instantiate argument exceptions correctly -->
+      <Rule Id="CA2208" Action="Info" />             <!-- Instantiate argument exceptions correctly -->
       <Rule Id="CA2211" Action="None" />             <!-- Non-constant fields should not be visible -->
       <Rule Id="CA2213" Action="None" />             <!-- Disposable fields should be disposed -->
       <Rule Id="CA2214" Action="None" />             <!-- Do not call overridable methods in constructors -->

diff --git a/src/coreclr/src/System.Private.CoreLib/src/Internal/Runtime/InteropServices/ComActivator.cs b/src/coreclr/src/System.Private.CoreLib/src/Internal/Runtime/InteropServices/ComActivator.cs
@@ -124,7 +124,7 @@ public static object GetClassFactoryForType(ComActivationContext cxt)
 
             if (!Path.IsPathRooted(cxt.AssemblyPath))
             {
-                throw new ArgumentException();
+                throw new ArgumentException(null, nameof(cxt));
             }
 
             Type classType = FindClassType(cxt.ClassId, cxt.AssemblyPath, cxt.AssemblyName, cxt.TypeName);
@@ -156,7 +156,7 @@ public static void ClassRegistrationScenarioForType(ComActivationContext cxt, bo
 
             if (!Path.IsPathRooted(cxt.AssemblyPath))
             {
-                throw new ArgumentException();
+                throw new ArgumentException(null, nameof(cxt));
             }
 
             Type classType = FindClassType(cxt.ClassId, cxt.AssemblyPath, cxt.AssemblyName, cxt.TypeName);
@@ -288,7 +288,7 @@ public static unsafe int RegisterClassForTypeInternal(ComActivationContextIntern
             if (cxtInt.InterfaceId != Guid.Empty
                 || cxtInt.ClassFactoryDest != IntPtr.Zero)
             {
-                throw new ArgumentException();
+                throw new ArgumentException(null, nameof(pCxtInt));
             }
 
             try
@@ -328,7 +328,7 @@ public static unsafe int UnregisterClassForTypeInternal(ComActivationContextInte
             if (cxtInt.InterfaceId != Guid.Empty
                 || cxtInt.ClassFactoryDest != IntPtr.Zero)
             {
-                throw new ArgumentException();
+                throw new ArgumentException(null, nameof(pCxtInt));
             }
 
             try

diff --git a/src/coreclr/src/System.Private.CoreLib/src/System/ArgIterator.cs b/src/coreclr/src/System.Private.CoreLib/src/System/ArgIterator.cs
@@ -85,7 +85,9 @@ public TypedReference GetNextArg(RuntimeTypeHandle rth)
                 // malicious caller to increment the pointer to an arbitrary
                 // location in memory and read the contents.
                 if (ArgPtr == IntPtr.Zero)
+#pragma warning disable CA2208 // Instantiate argument exceptions correctly, the argument not applicable
                     throw new ArgumentNullException();
+#pragma warning restore CA2208
 
                 TypedReference result = default;
                 // reference to TypedReference is banned, so have to pass result as pointer

diff --git a/src/coreclr/src/System.Private.CoreLib/src/System/GC.cs b/src/coreclr/src/System.Private.CoreLib/src/System/GC.cs
@@ -135,7 +135,7 @@ public static void AddMemoryPressure(long bytesAllocated)
 
             if ((4 == IntPtr.Size) && (bytesAllocated > int.MaxValue))
             {
-                throw new ArgumentOutOfRangeException("pressure",
+                throw new ArgumentOutOfRangeException(nameof(bytesAllocated),
                     SR.ArgumentOutOfRange_MustBeNonNegInt32);
             }
 

diff --git a/src/coreclr/src/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.cs b/src/coreclr/src/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.cs
@@ -136,7 +136,7 @@ public IntPtr GetOrCreateComInterfaceForObject(object instance, CreateComInterfa
         {
             IntPtr ptr;
             if (!TryGetOrCreateComInterfaceForObjectInternal(this, instance, flags, out ptr))
-                throw new ArgumentException();
+                throw new ArgumentException(null, nameof(instance));
 
             return ptr;
         }
@@ -215,7 +215,7 @@ public object GetOrCreateObjectForComInstance(IntPtr externalComObject, CreateOb
         {
             object? obj;
             if (!TryGetOrCreateObjectForComInstanceInternal(this, externalComObject, flags, null, out obj))
-                throw new ArgumentNullException();
+                throw new ArgumentNullException(nameof(externalComObject));
 
             return obj!;
         }
@@ -271,7 +271,7 @@ public object GetOrRegisterObjectForComInstance(IntPtr externalComObject, Create
 
             object? obj;
             if (!TryGetOrCreateObjectForComInstanceInternal(this, externalComObject, flags, wrapper, out obj))
-                throw new ArgumentNullException();
+                throw new ArgumentNullException(nameof(externalComObject));
 
             return obj!;
         }

diff --git a/src/coreclr/src/System.Private.CoreLib/src/System/RuntimeType.CoreCLR.cs b/src/coreclr/src/System.Private.CoreLib/src/System/RuntimeType.CoreCLR.cs
@@ -2755,7 +2755,7 @@ public override InterfaceMapping GetInterfaceMap(Type ifaceType)
         protected override PropertyInfo? GetPropertyImpl(
             string name, BindingFlags bindingAttr, Binder? binder, Type? returnType, Type[]? types, ParameterModifier[]? modifiers)
         {
-            if (name == null) throw new ArgumentNullException();
+            if (name == null) throw new ArgumentNullException(nameof(name));
 
             ListBuilder<PropertyInfo> candidates = GetPropertyCandidates(name, bindingAttr, types, false);
 
@@ -2791,7 +2791,7 @@ public override InterfaceMapping GetInterfaceMap(Type ifaceType)
 
         public override EventInfo? GetEvent(string name, BindingFlags bindingAttr)
         {
-            if (name is null) throw new ArgumentNullException();
+            if (name is null) throw new ArgumentNullException(nameof(name));
 
             FilterHelper(bindingAttr, ref name, out _, out MemberListType listType);
 
@@ -2854,7 +2854,7 @@ public override InterfaceMapping GetInterfaceMap(Type ifaceType)
 
         public override Type? GetInterface(string fullname, bool ignoreCase)
         {
-            if (fullname is null) throw new ArgumentNullException();
+            if (fullname is null) throw new ArgumentNullException(nameof(fullname));
 
             BindingFlags bindingAttr = BindingFlags.Public | BindingFlags.NonPublic;
 
@@ -2888,7 +2888,7 @@ public override InterfaceMapping GetInterfaceMap(Type ifaceType)
 
         public override Type? GetNestedType(string fullname, BindingFlags bindingAttr)
         {
-            if (fullname is null) throw new ArgumentNullException();
+            if (fullname is null) throw new ArgumentNullException(nameof(fullname));
 
             bindingAttr &= ~BindingFlags.Static;
             string name, ns;
@@ -2916,7 +2916,7 @@ public override InterfaceMapping GetInterfaceMap(Type ifaceType)
 
         public override MemberInfo[] GetMember(string name, MemberTypes type, BindingFlags bindingAttr)
         {
-            if (name is null) throw new ArgumentNullException();
+            if (name is null) throw new ArgumentNullException(nameof(name));
 
             ListBuilder<MethodInfo> methods = default;
             ListBuilder<ConstructorInfo> constructors = default;

diff --git a/src/coreclr/src/System.Private.CoreLib/src/System/Threading/ThreadPool.CoreCLR.cs b/src/coreclr/src/System.Private.CoreLib/src/System/Threading/ThreadPool.CoreCLR.cs
@@ -260,7 +260,7 @@ private static extern long PendingUnmanagedWorkItemCount
             get;
         }
 
-        private static RegisteredWaitHandle RegisterWaitForSingleObject(  // throws RegisterWaitException
+        private static RegisteredWaitHandle RegisterWaitForSingleObject(
              WaitHandle waitObject,
              WaitOrTimerCallback callBack,
              object? state,

diff --git a/src/libraries/Common/src/System/Buffers/ArrayBufferWriter.cs b/src/libraries/Common/src/System/Buffers/ArrayBufferWriter.cs
@@ -42,7 +42,7 @@ public ArrayBufferWriter()
         public ArrayBufferWriter(int initialCapacity)
         {
             if (initialCapacity <= 0)
-                throw new ArgumentException(nameof(initialCapacity));
+                throw new ArgumentException(null, nameof(initialCapacity));
 
             _buffer = new T[initialCapacity];
             _index = 0;
@@ -101,7 +101,7 @@ public void Clear()
         public void Advance(int count)
         {
             if (count < 0)
-                throw new ArgumentException(nameof(count));
+                throw new ArgumentException(null, nameof(count));
 
             if (_index > _buffer.Length - count)
                 ThrowInvalidOperationException_AdvancedTooFar(_buffer.Length);

diff --git a/src/libraries/Common/src/System/Threading/Tasks/TaskToApm.cs b/src/libraries/Common/src/System/Threading/Tasks/TaskToApm.cs
@@ -43,7 +43,7 @@ public static void End(IAsyncResult asyncResult)
                 return;
             }
 
-            throw new ArgumentNullException();
+            throw new ArgumentNullException(nameof(asyncResult));
         }
 
         /// <summary>Processes an IAsyncResult returned by Begin.</summary>
@@ -55,7 +55,7 @@ public static TResult End<TResult>(IAsyncResult asyncResult)
                 return task.GetAwaiter().GetResult();
             }
 
-            throw new ArgumentNullException();
+            throw new ArgumentNullException(nameof(asyncResult));
         }
 
         /// <summary>Provides a simple IAsyncResult that wraps a Task.</summary>

diff --git a/src/libraries/Common/tests/TestUtilities/System/AssertExtensions.cs b/src/libraries/Common/tests/TestUtilities/System/AssertExtensions.cs
@@ -27,22 +27,23 @@ public static void ThrowsContains<T>(Action action, string expectedMessageConten
             Assert.Contains(expectedMessageContent, Assert.Throws<T>(action).Message);
         }
 
-        public static void Throws<T>(string netCoreParamName, string netFxParamName, Action action)
+        public static T Throws<T>(string netCoreParamName, string netFxParamName, Action action)
             where T : ArgumentException
         {
             T exception = Assert.Throws<T>(action);
 
             if (netFxParamName == null && IsNetFramework)
             {
                 // Param name varies between .NET Framework versions -- skip checking it
-                return;
+                return exception;
             }
 
             string expectedParamName =
                 IsNetFramework ?
                 netFxParamName : netCoreParamName;
 
             Assert.Equal(expectedParamName, exception.ParamName);
+            return exception;
         }
 
         public static void Throws<T>(string netCoreParamName, string netFxParamName, Func<object> testCode)

diff --git a/src/libraries/Microsoft.Extensions.Configuration/src/ChainedConfigurationProvider.cs b/src/libraries/Microsoft.Extensions.Configuration/src/ChainedConfigurationProvider.cs
@@ -29,7 +29,7 @@ public ChainedConfigurationProvider(ChainedConfigurationSource source)
             }
             if (source.Configuration == null)
             {
-                throw new ArgumentNullException(nameof(source.Configuration));
+                throw new ArgumentException(SR.Format(SR.InvalidNullArgument, "source.Configuration"), nameof(source));
             }
 
             _config = source.Configuration;

diff --git a/src/libraries/Microsoft.Extensions.Configuration/src/Resources/Strings.resx b/src/libraries/Microsoft.Extensions.Configuration/src/Resources/Strings.resx
@@ -120,4 +120,7 @@
   <data name="Error_NoSources" xml:space="preserve">
     <value>A configuration source is not registered. Please register one before setting a value.</value>
   </data>
+  <data name="InvalidNullArgument" xml:space="preserve">
+    <value>Null is not a valid value for '{0}'.</value>
+  </data>
 </root>
diff --git a/src/libraries/Microsoft.Extensions.DependencyModel/src/Dependency.cs b/src/libraries/Microsoft.Extensions.DependencyModel/src/Dependency.cs
@@ -13,11 +13,11 @@ public Dependency(string name, string version)
         {
             if (string.IsNullOrEmpty(name))
             {
-                throw new ArgumentException(nameof(name));
+                throw new ArgumentException(null, nameof(name));
             }
             if (string.IsNullOrEmpty(version))
             {
-                throw new ArgumentException(nameof(version));
+                throw new ArgumentException(null, nameof(version));
             }
             Name = name;
             Version = version;

diff --git a/src/libraries/Microsoft.Extensions.DependencyModel/src/Library.cs b/src/libraries/Microsoft.Extensions.DependencyModel/src/Library.cs
@@ -43,15 +43,15 @@ public Library(string type,
         {
             if (string.IsNullOrEmpty(type))
             {
-                throw new ArgumentException(nameof(type));
+                throw new ArgumentException(null, nameof(type));
             }
             if (string.IsNullOrEmpty(name))
             {
-                throw new ArgumentException(nameof(name));
+                throw new ArgumentException(null, nameof(name));
             }
             if (string.IsNullOrEmpty(version))
             {
-                throw new ArgumentException(nameof(version));
+                throw new ArgumentException(null, nameof(version));
             }
             if (dependencies == null)
             {

diff --git a/src/libraries/Microsoft.Extensions.DependencyModel/src/ResourceAssembly.cs b/src/libraries/Microsoft.Extensions.DependencyModel/src/ResourceAssembly.cs
@@ -12,11 +12,11 @@ public ResourceAssembly(string path, string locale)
         {
             if (string.IsNullOrEmpty(path))
             {
-                throw new ArgumentException(nameof(path));
+                throw new ArgumentException(null, nameof(path));
             }
             if (string.IsNullOrEmpty(locale))
             {
-                throw new ArgumentException(nameof(locale));
+                throw new ArgumentException(null, nameof(locale));
             }
             Locale = locale;
             Path = path;
@@ -27,4 +27,4 @@ public ResourceAssembly(string path, string locale)
         public string Path { get; set; }
 
     }
-}
+}
diff --git a/src/libraries/Microsoft.Extensions.DependencyModel/src/RuntimeAssembly.cs b/src/libraries/Microsoft.Extensions.DependencyModel/src/RuntimeAssembly.cs
@@ -16,11 +16,11 @@ public RuntimeAssembly(string assemblyName, string path)
         {
             if (string.IsNullOrEmpty(assemblyName))
             {
-                throw new ArgumentException(nameof(assemblyName));
+                throw new ArgumentException(null, nameof(assemblyName));
             }
             if (string.IsNullOrEmpty(path))
             {
-                throw new ArgumentException(nameof(path));
+                throw new ArgumentException(null, nameof(path));
             }
             _assemblyName = assemblyName;
             Path = path;
@@ -45,4 +45,4 @@ public static RuntimeAssembly Create(string path)
             return new RuntimeAssembly(assemblyName, path);
         }
     }
-}
+}
diff --git a/src/libraries/Microsoft.Extensions.DependencyModel/src/RuntimeFallbacks.cs b/src/libraries/Microsoft.Extensions.DependencyModel/src/RuntimeFallbacks.cs
@@ -18,7 +18,7 @@ public RuntimeFallbacks(string runtime, IEnumerable<string> fallbacks)
         {
             if (string.IsNullOrEmpty(runtime))
             {
-                throw new ArgumentException(nameof(runtime));
+                throw new ArgumentException(null, nameof(runtime));
             }
             if (fallbacks == null)
             {
@@ -28,4 +28,4 @@ public RuntimeFallbacks(string runtime, IEnumerable<string> fallbacks)
             Fallbacks = fallbacks.ToArray();
         }
     }
-}
+}
diff --git a/src/libraries/Microsoft.Extensions.DependencyModel/src/RuntimeFile.cs b/src/libraries/Microsoft.Extensions.DependencyModel/src/RuntimeFile.cs
@@ -12,7 +12,7 @@ public RuntimeFile(string path, string assemblyVersion, string fileVersion)
         {
             if (string.IsNullOrEmpty(path))
             {
-                throw new ArgumentException(nameof(path));
+                throw new ArgumentException(null, nameof(path));
             }
 
             Path = path;

diff --git a/src/libraries/Microsoft.Extensions.DependencyModel/src/TargetInfo.cs b/src/libraries/Microsoft.Extensions.DependencyModel/src/TargetInfo.cs
@@ -14,7 +14,7 @@ public TargetInfo(string framework,
         {
             if (string.IsNullOrEmpty(framework))
             {
-                throw new ArgumentException(nameof(framework));
+                throw new ArgumentException(null, nameof(framework));
             }
 
             Framework = framework;
@@ -32,4 +32,4 @@ public TargetInfo(string framework,
         public bool IsPortable { get; }
 
     }
-}
+}