Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Breaking change]: Limiting the precision in numeric format strings from Int32.MaxValue to 999,999,999 #30352

Closed
1 of 2 tasks
dakersnar opened this issue Jul 26, 2022 · 4 comments · Fixed by #31002
Closed
1 of 2 tasks
Assignees
Labels
binary incompatible Existing binaries may encounter a breaking change in behavior. breaking-change Indicates a .NET Core breaking change 🏁 Release: .NET 7 Work items for the .NET 7 release doc-idea Indicates issues that are suggestions for new topics [org][type][category] Pri1 High priority, do before Pri2 and Pri3

Comments

@dakersnar
Copy link

dakersnar commented Jul 26, 2022

Description

The fix will likely be merged into .NET 7 RC1 with dotnet/runtime#72647.

in .NET 6, we updated the maximum precision for numeric format strings to be Int32.MaxValue, as documented here https://docs.microsoft.com/en-us/dotnet/standard/base-types/standard-numeric-format-strings and here https://docs.microsoft.com/en-us/dotnet/core/compatibility/core-libraries/6.0/numeric-format-parsing-handles-higher-precision. Unfortunately, the implementation was buggy, and the easiest fix requires we pick a slightly smaller limit of 999,999,999. The original expansion to Int32.MaxValue was pretty arbitrary as far as I can tell, and 999,999,999 is more than big enough. This comment (dotnet/runtime#67891 (comment)) goes into a bit more detail on the logic of this choice.

Version

.NET 7 RC1

Previous behavior

The previous behavior intended to throw a FormatException for any precision larger than Int.MaxValue. It did not throw that exception for many of such inputs due to the mentioned bug. The intended behavior was:

double d = 123.0;

d.ToString("E" + int.MaxValue.ToString()); // doesn't throw

long intMaxPlus1 = (long)int.MaxValue + 1;
string intMaxPlus1String = intMaxPlus1.ToString();
Assert.Throws<FormatException>(() => d.ToString("E" + intMaxPlus1String)); // throws

New behavior

Now, we correctly throw a FormatException for any precision larger than 999,999,999.

double d = 123.0;
Assert.Throws<FormatException>(() => d.ToString("E" + int.MaxValue.ToString())); // throws

long intMaxPlus1 = (long)int.MaxValue + 1;
string intMaxPlus1String = intMaxPlus1.ToString();
Assert.Throws<FormatException>(() => d.ToString("E" + intMaxPlus1String)); // throws

d.ToString("E999999999"); // doesn't throw

d.ToString("E00000999999999"); // doesn't throw

Type of breaking change

  • Binary incompatible: Existing binaries may encounter a breaking change in behavior, such as failure to load/execute or different run-time behavior.
  • Source incompatible: Source code may encounter a breaking change in behavior when targeting the new runtime/component/SDK, such as compile errors or different run-time behavior.

Reason for change

Described above.

Recommended action

This change should not have any substantial effect, as I don't think many are using precisions this large. Simply updating the documentation should be sufficient.

Feature area

Core .NET libraries

Affected APIs

Should be the same APIs as the previous change https://docs.microsoft.com/en-us/dotnet/core/compatibility/core-libraries/6.0/numeric-format-parsing-handles-higher-precision

@dakersnar dakersnar added doc-idea Indicates issues that are suggestions for new topics [org][type][category] breaking-change Indicates a .NET Core breaking change Pri1 High priority, do before Pri2 and Pri3 labels Jul 26, 2022
@dotnet-bot dotnet-bot added ⌚ Not Triaged Not triaged 🏁 Release: .NET 7 Work items for the .NET 7 release labels Jul 26, 2022
@dakersnar
Copy link
Author

To reiterate, this is a pretty minor change that should have no significant impact. Let me know if you need any more information.

@tannergooding
Copy link
Member

LGTM.

Just as an aside for the template (CC. @jeffhandley), I'm not personally a fan of:

Binary incompatible: Existing binaries may encounter a breaking change in behavior, such as failure to load/execute or different run-time behavior.

To me, it would be better if we had this as:

  • Behavioral break: Existing binaries may result in a different behavior at runtime
  • Source break: Existing sources may fail to compile
  • Binary break: Existing binaries may fail to load or execute

A binary break is typically referred to as a very serious change and one we almost never make.

Source breaks do happen, but we try to mitigate them as much as possible and are typically the result of things like new overloads or the like.

Behavioral breaks are by far the most common and can happen for any number of reasons, sometimes even due to things outside our control such as the underlying C Runtime implementation being versioned. In this case, the binary will load/execute and the sources will continue compiling, its just that someone who between .NET 6 and 7 decided that printing a 1-2GB string was a good idea may run into a break if they requested 1 billion or more digits.

@jeffhandley
Copy link
Member

Thanks, @tannergooding. You make a good point that there are different degrees for breaking changes. While this one lands in the "different run-time behavior" subcategory, it's a subtle, corner case binary breaking change. It won't cause the program to fail to load or execute, but the application could yield different results in extremely limited scenarios.

I'll leave it to @gewarren to consider if we should have subcategories on these issues to indicate which type(s) of binary/source-breaking change apply.

@gewarren gewarren removed the ⌚ Not Triaged Not triaged label Jul 28, 2022
@dotnet-bot dotnet-bot added the binary incompatible Existing binaries may encounter a breaking change in behavior. label Jul 28, 2022
@dakersnar
Copy link
Author

Side note, in a similar fashion we are also limiting the maximum exponent allowed when parsing a BigInteger from a string to 999,999,999. See dotnet/runtime#73643 (comment) for more details.

@ghost ghost added the in-pr This issue will be closed (fixed) by an active pull request. label Sep 3, 2022
@ghost ghost removed the in-pr This issue will be closed (fixed) by an active pull request. label Sep 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
binary incompatible Existing binaries may encounter a breaking change in behavior. breaking-change Indicates a .NET Core breaking change 🏁 Release: .NET 7 Work items for the .NET 7 release doc-idea Indicates issues that are suggestions for new topics [org][type][category] Pri1 High priority, do before Pri2 and Pri3
Projects
No open projects
Status: Done
Development

Successfully merging a pull request may close this issue.

5 participants