Add/use ArgumentException.ThrowIfNullOrEmpty #64357
Conversation
|
Note regarding the This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change. |
|
Tagging subscribers to this area: @dotnet/area-meta Issue DetailsAdds ArgumentException.ThrowIfNullOrEmpty and then uses it in a bunch of places around the tree. Most of the replaced places used a resource string for the message, and I used it in places where it didn't feel like we were losing any meaningful information by centralizing on the single shared string, but I didn't use it in places where the resource string message used in the empty case conveyed something that seemed useful, e.g. a recommendation on what to use instead of an empty string. There are a few places where I allowed for order of exception throws to change in the case where there were multiple user errors and the validation for a single argument was split, e.g. I changed a few cases of: if (arg1 is null) throw new ArgumentNullException(...);
if (arg2 is null) throw new ArgumentNullException(...);
if (arg1.Length == 0) throw new ArgumentException(...);
if (arg2.Length == 0) throw new ArgumentException(...);to: ArgumentException.ThrowIfNullOrEmpty(arg1);
ArgumentException.ThrowIfNullOrEmpty(arg2);even though it'll produce a different exception for I wanted to get this in before we do the massive !! conversion, as I expect !! auto-fixes will make it a bit harder to roll this out. I explicitly did not do anything to roll out use of ArgumentNullException.ThrowIfNull, except in cases where I was modifying a method to use ArgumentException.ThrowIfNullOrEmpty, in which case for consistency I changed anything else in the function that could have been using ThrowIfNull to do so. Fixes #62628
|
|
Tagging subscribers to this area: @dotnet/area-system-runtime Issue DetailsAdds ArgumentException.ThrowIfNullOrEmpty and then uses it in a bunch of places around the tree. Most of the replaced places used a resource string for the message, and I used it in places where it didn't feel like we were losing any meaningful information by centralizing on the single shared string, but I didn't use it in places where the resource string message used in the empty case conveyed something that seemed useful, e.g. a recommendation on what to use instead of an empty string. There are a few places where I allowed for order of exception throws to change in the case where there were multiple user errors and the validation for a single argument was split, e.g. I changed a few cases of: if (arg1 is null) throw new ArgumentNullException(...);
if (arg2 is null) throw new ArgumentNullException(...);
if (arg1.Length == 0) throw new ArgumentException(...);
if (arg2.Length == 0) throw new ArgumentException(...);to: ArgumentException.ThrowIfNullOrEmpty(arg1);
ArgumentException.ThrowIfNullOrEmpty(arg2);even though it'll produce a different exception for I wanted to get this in before we do the massive !! conversion, as I expect !! auto-fixes will make it a bit harder to roll this out. I explicitly did not do anything to roll out use of ArgumentNullException.ThrowIfNull, except in cases where I was modifying a method to use ArgumentException.ThrowIfNullOrEmpty, in which case for consistency I changed anything else in the function that could have been using ThrowIfNull to do so. Fixes #62628
|
|
Occurs to me reading this that eg |
|
Thanks, yeah, looks like I need to put back a resource or two I'd removed. |
stephentoub
force-pushed
the
nulloremptyexception
branch
from
e57ce23
to
b5eb805
Compare
There was a problem hiding this comment.
Technically a breaking change, but given this is a case of multiple usage errors and all the exceptions are ArgumentException-based, I think it's acceptable.
I wholeheartedly agree. 👍
These changes look so good that I am surprised that we haven't come up with this idea sooner. Kudos to @datvm for the proposal!
src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/MethodBuilder.cs
Show resolved
Hide resolved
src/libraries/System.Runtime/tests/System/ArgumentExceptionTests.cs
Outdated
Show resolved
Hide resolved
...curity.Cryptography.X509Certificates/tests/CertificateCreation/CertificateRequestApiTests.cs
Show resolved
Hide resolved
.../System.Security.Cryptography/src/System/Security/Cryptography/Rfc2898DeriveBytes.OneShot.cs
Show resolved
Hide resolved
stephentoub
force-pushed
the
nulloremptyexception
branch
2 times, most recently
from
d7d3ae0
to
80c975c
Compare
|
We were just thinking a That would align with |
Adds ArgumentException.ThrowIfNullOrEmpty and then uses it in a bunch of places around the tree. Most of the replaced places used a resource string for the message, and I used it in places where it didn't feel like we were losing any meaningful information by centralizing on the single shared string, but I didn't use it in places where the resource string message used in the empty case conveyed something that seemed useful, e.g. a recommendation on what to use instead of an empty string.
There are a few places where I allowed for order of exception throws to change in the case where there were multiple user errors and the validation for a single argument was split, e.g. I changed a few cases of:
```C#
if (arg1 is null) throw new ArgumentNullException(...);
if (arg2 is null) throw new ArgumentNullException(...);
if (arg1.Length == 0) throw new ArgumentException(...);
if (arg2.Length == 0) throw new ArgumentException(...);
```
to:
```C#
ArgumentException.ThrowIfNullOrEmpty(arg1);
ArgumentException.ThrowIfNullOrEmpty(arg2);
```
even though it'll produce a different exception for `M("", null)` than it would previously. Technically a breaking change, but given this is a case of multiple usage errors and all the exceptions are ArgumentException-based, I think it's acceptable.
I wanted to get this in before we do the massive !! conversion, as I expect !! auto-fixes will make it a bit harder to roll this out. I explicitly did not do anything to roll out use of ArgumentNullException.ThrowIfNull, except in cases where I was modifying a method to use ArgumentException.ThrowIfNullOrEmpty, in which case for consistency I changed anything else in the function that could have been using ThrowIfNull to do so.
stephentoub
force-pushed
the
nulloremptyexception
branch
from
80c975c
to
003e43b
Compare
No, this PR is about the approved ThrowIfNullOrEmpty API. APIs need to be reviewed and approved following https://github.com/dotnet/runtime/blob/main/docs/project/api-review-process.md before PRs for them are accepted. That process starts with someone opening an issue proposing the APIs in question; please feel free to do so. |
|
All Improvements on windows-arm64 are related dotnet/perf-autofiling-issues#3288 |
|
Improvements on ubuntu-arm64: dotnet/perf-autofiling-issues#3294 |
|
@EgorBo, while I'd love to take credit for such improvements, I'm skeptical they're because of this... e.g. I don't think Int64.Parse touches anything that overlaps with this PR. Why do we think it's related? |
|
@stephentoub this change significantly affected inliner's decisions, e.g. https://sharplab.io/#v2:C4LghgzgtgPgAgJgIwFgBQ64BYAEBZACgHsAjAKwFMBjYHIgShwF4A+HYACwCciB3HAHYV+AQS4BzAK5QKA4ADlJAGyUBRAB5UKAB2ABLIgIICwMogDNi9egG4gA for this throw we have to emit a lot of codegen and 4 |
Yes... but I don't think any of those changes are anywhere on these code paths, are they? |
|
Range of commits 01fabd0...c7adf55 #64405 is also a candidate, but do any of the parsing functions use StringBuilder? Also, I assume you know, but don't look at the graphs in the issue - they don't reflect most recent improvements sometimes like today |
ghost
locked as resolved and limited conversation to collaborators
Adds ArgumentException.ThrowIfNullOrEmpty and then uses it in a bunch of places around the tree. Most of the replaced places used a resource string for the message, and I used it in places where it didn't feel like we were losing any meaningful information by centralizing on the single shared string, but I didn't use it in places where the resource string message used in the empty case conveyed something that seemed useful, e.g. a recommendation on what to use instead of an empty string.
There are a few places where I allowed for order of exception throws to change in the case where there were multiple user errors and the validation for a single argument was split, e.g. I changed a few cases of:
to:
even though it'll produce a different exception for
M("", null)than it would previously. Technically a breaking change, but given this is a case of multiple usage errors and all the exceptions are ArgumentException-based, I think it's acceptable.I wanted to get this in before we do the massive !! conversion, as I expect !! auto-fixes will make it a bit harder to roll this out. I explicitly did not do anything to roll out use of ArgumentNullException.ThrowIfNull, except in cases where I was modifying a method to use ArgumentException.ThrowIfNullOrEmpty, in which case for consistency I changed anything else in the function that could have been using ThrowIfNull to do so.
Fixes #62628