Updating NumberToStringFormat to not print the sign if there are no digits being returned

[tannergooding]

This ensures that the sign is only printed if we are returning one or more digits.

[tannergooding]

As an example:

var nfi = new NumberFormatInfo();

nfi.NaNSymbol = "NaN";
nfi.PositiveSign = "+";
nfi.NegativeSign = "-";
nfi.PerMilleSymbol = "x";
nfi.PositiveInfinitySymbol = "Infinity";
nfi.NegativeInfinitySymbol = "-Infinity";

nfi.NumberDecimalDigits = 5;
nfi.NumberDecimalSeparator = ".";
nfi.NumberGroupSeparator = ",";
nfi.NumberGroupSizes = new int[] { 3 };
nfi.NumberNegativePattern = 2;

nfi.CurrencyDecimalDigits = 2;
nfi.CurrencyDecimalSeparator = ".";
nfi.CurrencyGroupSeparator = ",";
nfi.CurrencyGroupSizes = new int[] { 3 };
nfi.CurrencyNegativePattern = 8;
nfi.CurrencyPositivePattern = 3;
nfi.CurrencySymbol = "$";

nfi.PercentDecimalDigits = 5;
nfi.PercentDecimalSeparator = ".";
nfi.PercentGroupSeparator = ",";
nfi.PercentGroupSizes = new int[] { 3 };
nfi.PercentNegativePattern = 0;
nfi.PercentPositivePattern = 0;
nfi.PercentSymbol = "%";

var str = (-0.0).ToString(nfi);
Console.WriteLine($"'{str}'");

str = (-0.0).ToString("0", nfi);
Console.WriteLine($"'{str}'");

str = (-0.0).ToString("0,", nfi);
Console.WriteLine($"'{str}'");

str = (-0.0).ToString("0,,", nfi);
Console.WriteLine($"'{str}'");

str = (-0.0).ToString("#", nfi);
Console.WriteLine($"'{str}'");

str = (-0.0).ToString("#,", nfi);
Console.WriteLine($"'{str}'");

str = (-0.0).ToString("#,,", nfi);
Console.WriteLine($"'{str}'");

str = (-1234.0).ToString(nfi);
Console.WriteLine($"'{str}'");

str = (-1234.0).ToString("0", nfi);
Console.WriteLine($"'{str}'");

str = (-1234.0).ToString("0,", nfi);
Console.WriteLine($"'{str}'");

str = (-1234.0).ToString("0,,", nfi);
Console.WriteLine($"'{str}'");

str = (-1234.0).ToString("#", nfi);
Console.WriteLine($"'{str}'");

str = (-1234.0).ToString("#,", nfi);
Console.WriteLine($"'{str}'");

str = (-1234.0).ToString("#,,", nfi);
Console.WriteLine($"'{str}'");

Prints the following in 2.1:

'0'
'0'
'0'
'0'
''
''
''
'-1234'
'-1234'
'-1'
'0'
'-1234'
'-1'
''

and will now print:

'-0'
'-0'
'-0'
'-0'
''
''
''
'-1234'
'-1234'
'-1'
'-0'
'-1234'
'-1'
''

[tannergooding]

@EgorBo pointed out the bug here: #20080 (comment)

The issue was introduced in #19775, which was fixing up the formatter to properly print -0, but which did not properly handle the case where the custom format string would cause no digits to be returned. This was causing all the results which should have returned '' to instead return '-'.

[tannergooding]

Working on adding some additional tests to CoreFX to cover this scenario.

[tannergooding]

CC. @jkotas, @danmosemsft

[tannergooding]

We could keep the original path and have the code down here only for the case where scale == 0.

For the scale == 0 case, I didn't see a clear way to check if a digit would be printed outside duplicating a number of the checks from the while loop.

[tannergooding]

Failing CoreFX test is with a custom format specifier ";", where it expects the result string to be "-", but the new code returns "".

Given the documentation on The ";" section separator, I wonder if the current behavior is a bug and the new behavior is correct.

Section separators ignore any preexisting formatting associated with a number when the final value is formatted. For example, negative values are always displayed without a minus sign when section separators are used. If you want the final formatted value to have a minus sign, you should explicitly include the minus sign as part of the custom format specifier.

I would think this means that the sign is meant to never be returned, including when there are no digits given.

Edit: I also wouldn't expect it to have a different behavior from the single section case, given the other comments there.

[danmoseley]

I agree that test sounds wrong

[tannergooding]

@danmosemsft, wondering if the "empty" section case is also a "bigger" bug. Currently, there seems to be a lot of "weird" behavior around the section separator and it doesn't seem to be consistent.

For example, from the docs for the "three section" case:

The second section can be left empty (by having nothing between the semicolons), in which case the first section applies to all nonzero values.

The "two section" case doesn't have any explicit wording on it, but I would imagine it would/should follow the same logic.

For "-1234.0" and various format specifiers, we currently get the following:

Format Specifier	Output
""	-1234
";"	-
"#;"	-1234
";#"	1234
"#;#"	1234
";;"	-
"#;;"	-1234
";#;"	1234
";;#"	-
"#;#;"	1234
";#;#"	1234
"#;;#"	-1234
"#;#;#"	1234
";;;"	-

I would expect:
the documentation to give an example of one vs two vs three section separators

• I interpreted, based on testing, no ; to be single-section and one ; to be a two-section
• different behavior for more than 3 sections (since that is the "max")
• the two or three section separator to always exclude the negative sign
  • I interpreted this to also apply when the second separator is empty and forwards to the first separator
  • I interpreted this to not apply to "single section" (no ; present)
• an empty section to be treated as "", less the negative sign ("" is currently treated as "G")

[tannergooding]

CC. @dotnet/dnceng and @adiaaida. The various perf tests keep failing with

12:44:22 Installing dotnet cli...
12:44:47 Restoring BuildTools version 2.2.0-preview1-03116-01...
12:45:14 Initializing BuildTools...
12:45:36 FATAL: command execution failed
12:45:36 java.nio.channels.ClosedChannelException
12:45:36 	at org.jenkinsci.remoting.protocol.NetworkLayer.onRecvClosed(NetworkLayer.java:154)
12:45:36 	at org.jenkinsci.remoting.protocol.impl.NIONetworkLayer.ready(NIONetworkLayer.java:179)
12:45:36 	at org.jenkinsci.remoting.protocol.IOHub$OnReady.run(IOHub.java:721)
12:45:36 	at jenkins.util.ContextResettingExecutorService$1.run(ContextResettingExecutorService.java:28)
12:45:36 	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
12:45:36 	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
12:45:36 	at java.lang.Thread.run(Thread.java:748)
12:45:36 Caused: java.io.IOException: Backing channel 'JNLP4-connect connection from 104.42.52.210/104.42.52.210:19392' is disconnected.
12:45:36 	at hudson.remoting.RemoteInvocationHandler.channelOrFail(RemoteInvocationHandler.java:212)
12:45:36 	at hudson.remoting.RemoteInvocationHandler.invoke(RemoteInvocationHandler.java:281)
12:45:36 	at com.sun.proxy.$Proxy187.isAlive(Unknown Source)
12:45:36 	at hudson.Launcher$RemoteLauncher$ProcImpl.isAlive(Launcher.java:1138)
12:45:36 	at hudson.Launcher$RemoteLauncher$ProcImpl.join(Launcher.java:1130)
12:45:36 	at hudson.tasks.CommandInterpreter.join(CommandInterpreter.java:155)
12:45:36 	at hudson.tasks.CommandInterpreter.perform(CommandInterpreter.java:109)
12:45:36 	at hudson.tasks.CommandInterpreter.perform(CommandInterpreter.java:66)
12:45:36 	at hudson.tasks.BuildStepMonitor$1.perform(BuildStepMonitor.java:20)
12:45:36 	at hudson.model.AbstractBuild$AbstractBuildExecution.perform(AbstractBuild.java:744)
12:45:36 	at hudson.model.Build$BuildExecution.build(Build.java:206)
12:45:36 	at hudson.model.Build$BuildExecution.doRun(Build.java:163)
12:45:36 	at hudson.model.AbstractBuild$AbstractBuildExecution.run(AbstractBuild.java:504)
12:45:36 	at hudson.model.Run.execute(Run.java:1724)
12:45:36 	at hudson.model.FreeStyleBuild.run(FreeStyleBuild.java:43)
12:45:36 	at hudson.model.ResourceController.execute(ResourceController.java:97)
12:45:36 	at hudson.model.Executor.run(Executor.java:429)
12:45:36 Build step 'Execute Windows batch command' marked build as failure

[MattGal]

@adiaaida if you don't mind, can you please take an initial look and if it's not specific to the perf lab, open up a core-eng First-Responder issue.

[michellemcdaniel]

This looks like the VMs are disconnecting. We'll open a core-eng issue.

[MattGal]

Created https://github.com/dotnet/core-eng/issues/4228. Based off the logs, either it's very unlucky or it seems installing buildtools might actually be causing something bad to happen.

[tannergooding]

CC. @stephentoub. It was indicated that you might have some useful input on the above (#20109 (comment))

[tannergooding]

@jkotas, any other feedback on this?

[jkotas]

This is fixing the break, but introducing a different one. I do not have a good idea about the custom formatting strings to tell whether it is the right thing to do.

Would it make sense to fix the original break, without introducing a new one?

[tannergooding]

Would it make sense to fix the original break, without introducing a new one?

I can do that, and then log a bug tracking the potential change required for the section modifiers.

[tannergooding]

Logged https://github.com/dotnet/corefx/issues/32464 for the remaining questions

[tannergooding]

@jkotas, any other feedback here?