Double(Single).Parse(string) and Double(Single).Parse(ReadOnlySpan<char) throws different exception for null

[TrayanZapryanov]

    please file an issue on that with repro, that sounds like a bug

Originally posted by @krwq in #75452 (comment)

This makes migration from string to ReadOnlySpan a bit tricky.
Repro:

//Parse with null

string test = null;
		ReadOnlySpan<char> ros = test.AsSpan().Trim();
		try
		{
			double dVal = double.Parse(ros, NumberStyles.AllowLeadingSign | NumberStyles.AllowDecimalPoint | NumberStyles.AllowExponent | NumberStyles.AllowLeadingWhite | NumberStyles.AllowTrailingWhite, NumberFormatInfo.InvariantInfo);
		}
		catch (FormatException){}

		try
		{
			double dVal = double.Parse(test, NumberStyles.AllowLeadingSign | NumberStyles.AllowDecimalPoint | NumberStyles.AllowExponent | NumberStyles.AllowLeadingWhite | NumberStyles.AllowTrailingWhite, NumberFormatInfo.InvariantInfo);
		}
		catch (ArgumentNullException) { }

		//Parse with empty
		string testEmpty = "";
		ReadOnlySpan<char> rosEmpty = testEmpty.AsSpan().Trim();
		try
		{
			double dVal = double.Parse(rosEmpty, NumberStyles.AllowLeadingSign | NumberStyles.AllowDecimalPoint | NumberStyles.AllowExponent | NumberStyles.AllowLeadingWhite | NumberStyles.AllowTrailingWhite, NumberFormatInfo.InvariantInfo);
		}
		catch (FormatException) { }

		try
		{
			double dVal = double.Parse(testEmpty, NumberStyles.AllowLeadingSign | NumberStyles.AllowDecimalPoint | NumberStyles.AllowExponent | NumberStyles.AllowLeadingWhite | NumberStyles.AllowTrailingWhite, NumberFormatInfo.InvariantInfo);
		}
		catch (FormatException) { }

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

[Joe4evr]

Well, ReadOnlySpan is a struct, so it can't have a true null value; instead a null string is normalized to an empty span. After that, you lose the information of whether or not to throw ArgumentNullException. (Yes, you could compare the actual address to default(ROS<char>), but that's somewhat relying on implementation details and might be adding too much complexity.)

According to the doc, making existing code throw less exceptions is in "Bucket 2: Reasonable Grey Area", which sounds like it could be acceptable if the impact is deemed low enough.

[TrayanZapryanov]

If this is normal - how should migration looks like in dotnet repo ?
Should we add check for null in caller method(if possible) to mimic existing behavior, or it would be fine to live with FormatException instead of ArgumentNullException?
Or for every case we should check documentation and if it is written that some method throws ArgumentNull, only then to mimic old code ?

[Clockwork-Muse]

ArgumentException is indication of programmer error, so anyone relying on it/hitting it is already relying on broken code....

That said, probably any of our callers should be checking/enforcing against null args anyways, if they then delegate to a Span implementation. For clarity reasons if nothing else.

Tagging subscribers to this area: @dotnet/area-system-runtime
See info in area-owners.md if you want to be subscribed.

Issue Details

---

    please file an issue on that with repro, that sounds like a bug

Originally posted by @krwq in #75452 (comment)

This makes migration from string to ReadOnlySpan a bit tricky.
Repro:

//Parse with null

string test = null;
		ReadOnlySpan<char> ros = test.AsSpan().Trim();
		try
		{
			double dVal = double.Parse(ros, NumberStyles.AllowLeadingSign | NumberStyles.AllowDecimalPoint | NumberStyles.AllowExponent | NumberStyles.AllowLeadingWhite | NumberStyles.AllowTrailingWhite, NumberFormatInfo.InvariantInfo);
		}
		catch (FormatException){}

		try
		{
			double dVal = double.Parse(test, NumberStyles.AllowLeadingSign | NumberStyles.AllowDecimalPoint | NumberStyles.AllowExponent | NumberStyles.AllowLeadingWhite | NumberStyles.AllowTrailingWhite, NumberFormatInfo.InvariantInfo);
		}
		catch (ArgumentNullException) { }

		//Parse with empty
		string testEmpty = "";
		ReadOnlySpan<char> rosEmpty = testEmpty.AsSpan().Trim();
		try
		{
			double dVal = double.Parse(rosEmpty, NumberStyles.AllowLeadingSign | NumberStyles.AllowDecimalPoint | NumberStyles.AllowExponent | NumberStyles.AllowLeadingWhite | NumberStyles.AllowTrailingWhite, NumberFormatInfo.InvariantInfo);
		}
		catch (FormatException) { }

		try
		{
			double dVal = double.Parse(testEmpty, NumberStyles.AllowLeadingSign | NumberStyles.AllowDecimalPoint | NumberStyles.AllowExponent | NumberStyles.AllowLeadingWhite | NumberStyles.AllowTrailingWhite, NumberFormatInfo.InvariantInfo);
		}
		catch (FormatException) { }

Author:	TrayanZapryanov
Assignees:	-
Labels:	area-System.Runtime, untriaged
Milestone:	-

[tannergooding]

This difference is by design and has been this way for since .NET Core 2.1 when Span was first introduced.

As indicated, Span/ROSpan doesn't really have a way to expose "null" and as a value type there is no forced null checking required to get the length. Because of this, we can simply check the length directly and treat the input as "too short".

In general Span/ROSpan are similar to, but not the same as strings or arrays. Most of the functionality is identical and most of the time the string/array version forwards to some Span/ROSpan version after the relevant null check, but not always. These subtle differences are generally just around the null check and aren't something most devs need to consider.

---

Could you please clarify why you need to catch the exception here, let alone the exact exception?

Most exception types are indicative of errors and shouldn't be caught in regular circumstances (such as for control flow). In the case of something like the Parse APIs where you might be handling user input then TryParse is the correct alternativeas it returns bool and out T result instead that way you can determine success or failure without the cost or complexity of exceptions.

This issue has been marked needs-author-action and may be missing some important information.

[TrayanZapryanov]

@tannergooding Thank you for the detailed explanation. I also think in this direction, just wanted to be sure that will not introduce any breaking change when replacing Double(Single).Parse with their counter Span/ROSpan version.
Closing this as answered.