Runtime: Don't directly throw Exception

[svick]

When looking at https://github.com/Microsoft/dotnet/issues/471, I noticed that the thrown exception is Exception, not a type derived from Exception.

I think this should not be done and the Framework Design Guidelines agree. Instead, some derived type should be thrown.

This issue seems to happen in a few places in CoreFX:

$ git grep -n 'new Exception(' | grep -v tests | cut -d: -f1,2
Common/src/System/Drawing/ColorConverterCommon.cs:131
System.ComponentModel.TypeConverter/src/System/ComponentModel/BaseNumberConverter.cs:48
System.Configuration.ConfigurationManager/src/System/Configuration/GenericEnumConverter.cs:35
System.Configuration.ConfigurationManager/src/System/Configuration/GenericEnumConverter.cs:42
System.Configuration.ConfigurationManager/src/System/Configuration/GenericEnumConverter.cs:47
System.Configuration.ConfigurationManager/src/System/Configuration/ProtectedConfigurationSection.cs:58
System.Configuration.ConfigurationManager/src/System/Configuration/ProtectedConfigurationSection.cs:88
System.Data.SqlClient/src/System/Data/SqlClient/SNI/SNIProxy.cs:162
System.Data.SqlClient/src/System/Data/SqlClient/SNI/SNIProxy.cs:166
System.Data.SqlClient/src/System/Data/SqlClient/SqlUtil.cs:191
System.Drawing.Common/src/System/Drawing/Graphics.cs:216
System.IO.FileSystem.AccessControl/src/System/Security/AccessControl/DirectoryObjectSecurity.cs:352
System.IO.FileSystem.AccessControl/src/System/Security/AccessControl/DirectoryObjectSecurity.cs:398
System.IO.FileSystem.AccessControl/src/System/Security/AccessControl/DirectoryObjectSecurity.cs:417
System.IO.FileSystem.AccessControl/src/System/Security/AccessControl/DirectoryObjectSecurity.cs:493
System.Net.Http/src/System/Net/Http/Unix/CurlHandler.CurlResponseMessage.cs:61
System.Net.Mail/src/System/Net/Mail/MailHeaderInfo.cs:79
System.Net.WebSockets.Client/src/System/Net/WebSockets/WebSocketHandle.WinRT.cs:49
System.Private.DataContractSerialization/src/System/Runtime/Serialization/DiagnosticUtility.cs:83
System.Private.DataContractSerialization/src/System/Runtime/Serialization/Json/XmlJsonReader.cs:357
System.Private.Xml/src/System/Xml/BinaryXml/XmlBinaryReader.cs:3217
System.Private.Xml/src/System/Xml/BinaryXml/XmlBinaryReader.cs:3219
System.Private.Xml/src/System/Xml/Core/XmlTextReaderImplAsync.cs:3609
System.Private.Xml/src/System/Xml/Serialization/XmlSerializationWriter.cs:1925
System.Private.Xml/src/System/Xml/Serialization/Xmlcustomformatter.cs:80
System.Private.Xml/src/System/Xml/Serialization/Xmlcustomformatter.cs:246
System.Private.Xml/src/System/Xml/Xsl/Xslt/XPathPatternBuilder.cs:331
System.Runtime.WindowsRuntime/src/System/Runtime/InteropServices/WindowsRuntime/WindowsRuntimeBuffer.cs:94
System.Runtime.WindowsRuntime/src/System/Threading/Tasks/TaskToAsyncInfoAdapter.cs:983
System.Security.AccessControl/src/System/Security/AccessControl/SecurityDescriptor.cs:675
System.Threading.Tasks.Dataflow/src/Base/DataflowBlock.cs:1356

There are also many cases where CoreFX tests throw Exception, but I think that's okay.

[karelz]

I think it would be best to discuss it per area - I would start with 1 or 2 and see how it goes. There may be compatibility concerns, but hopefully not.

[satano]

I would take ConfigurationManager. It looks really easy. In GenericEnumConverter new Exception is used to control flow and it can be fully avoided. In ProtectedConfigurationSection it is just to decide which exception to throw. I would use ArgumentException.

[karelz]

@satano please submit a PR and we can have detailed discussion there ... thanks!

[JonHanna]

That particular case is one where the Exceptionis immediately caught, so the actual public method does not throw Exception as far as observable by users of the API. This might be the case for some of the rest of those above, in which case they are fine.

[danmosemsft]

@satano do yo ufeel like doing more of these..?

[satano]

@danmosemsft Yes, I will look at some other.

[MarcoRossignoli]

@danmosemsft @satano can i help on this issue or i stop?Maybe there are some throw Exception() on Drawing(Graphics.FromImage(image) for instance). I'm newbie i don't know if we can help together on issue.

[satano]

@MarcoRossignoli Yes, you can if you want. It is good to separate it by project. System.Configuration.ConfigurationManager is already done and waiting for merge. So you can take a look at the System.Drawing if you want. I will - later - check the others and do something.

[MarcoRossignoli]

@satano i'm waiting for System.Drawing, now i take System.IO.FileSystem.AccessControl.

[MarcoRossignoli]

after "$ git grep -n 'new Exception(' | grep -v tests | cut -d: -f1,2"

pull closed 25698 no action on this namespace after discussion

src/System.Data.SqlClient/src/System/Data/Sql/SqlNorm.cs:104
src/System.Data.SqlClient/src/System/Data/Sql/SqlNorm.cs:246
src/System.Data.SqlClient/src/System/Data/Sql/SqlSer.cs:199
src/System.Data.SqlClient/src/System/Data/SqlClient/SNI/SSRP.cs:37
src/System.Data.SqlClient/src/System/Data/SqlClient/SqlUtil.cs:197

No action

src/Microsoft.Diagnostics.Tracing.EventSource.Redist/shared/StubEnvironment.cs:69 --> Assert code #IF ES_BUILD_AGAINST_DOTNET_V35
src/System.Drawing.Common/src/System/Drawing/Font.Serializable.cs:83 --> excluded from compilation
src/System.Drawing.Common/src/System/Drawing/Icon.Unix.cs:556 --> internal no references found
src/System.Management/src/System/Management/WMIGenerator.cs:566 --> comment out
src/System.Net.Http/src/System/Net/Http/Unix/CurlHandler.CurlResponseMessage.cs:58  --> sentinel object
src/System.Net.Mail/src/System/Net/Mail/MailHeaderInfo.cs:79 --> #if DEBUG
src/System.Net.WebSockets.Client/src/System/Net/WebSockets/WebSocketHandle.WinRT.cs:49 --> rethrow WebSocketException()
src/System.Private.DataContractSerialization/src/System/Runtime/Serialization/DiagnosticUtility.cs:78  --> it seems like never bubble out to user, no useful test found, maybe on race condition bug or OutOfMemoryException, StackOverflowException
src/System.Private.Xml/src/System/Xml/BinaryXml/XmlBinaryReader.cs:3217 --> #if DEBUG 
src/System.Private.Xml/src/System/Xml/BinaryXml/XmlBinaryReader.cs:3219 --> #if DEBUG
src/System.Private.Xml/src/System/Xml/Serialization/XmlSerializationWriter.cs:1934 --> comment out
src/System.Runtime.Caching/src/System/Runtime/Caching/Dbg.cs:867 --> #IF DEBUG 
src/System.Runtime.WindowsRuntime/src/System/Runtime/InteropServices/WindowsRuntime/WindowsRuntimeBuffer.cs:93 --> rethrow NotImplementedException()
src/System.Runtime.WindowsRuntime/src/System/Threading/Tasks/TaskToAsyncInfoAdapter.cs:982 --> "instead of failing with an inexplicable NullReferenceException"
src/System.Threading.Channels/src/System/Threading/Channels/ChannelUtilities.cs:15 --> Sentinel object used to indicate being done writing.

@karelz @danmosemsft i think we can close this issue.
Maybe the guideline should be:
"X DO NOT throw System.Exception or System.SystemException if exception can reach user code." or something like that.
In my journey i found some place where the exception is throw and catch internally.
In other place throw generic exception is used because there isn't a good one already present and no custom class is used.

[karelz]

Thanks for working on it and for wrapping up the issue @MarcoRossignoli and @satano!

IMO every rule has an exception, so I am not sure if we need to update the official guidelines.

[danmosemsft]

I think even when throwing and catching internally it's nice to avoid using the base class if only because it doesn't show up in grepping like this...

BTW, I'm fine that we did this, but we may have a situation in future where .NET Core is more popular and folks write code for it first -- such code could then break when they try to run it on .NET Framework. We would have to port the changes then.

[MarcoRossignoli]

@danmosemsft with netstandard lib developed/tested on core host right?

[danmosemsft]

@MarcoRossignoli yep. It's not just exceptions - for example we have taken changes on Core that make a method more lenient in the inputs it expects. We think the value is worth it, but it's something that may come up in future.