ArgumentOutOfRangeException throw helpers#69767

hrrrrustic · 2022-05-25T01:08:18Z

Converting usages to possible new helpers (#69590) without /tests directories
It's possible that this PR contains a few wrong conversions/casts/bugs, but I'll fix them in smaller PRs after approving API. This one is just for collecting statistic

Did not find any meaningful custom messages and dropped them all for now. But can be revisited more carefully in smaller separate PRs. ~~ThrowIf method should contains additional message argument that I will add in separate PRs (Just need to regenerate all this diff and don't drop messages from old throwing)~~ It can't have it as lazy. ThrowIf will be removed it separate PRs

The main problem is that all tests with Assert.Throws( () => {}) verifies paramName (that's ok). But if some cast was used while passing argument to helper it will also appear in paramName and fail the test (e.g (uint) index)

Remaining manual throwing:

else if (...) throw OutOfRange else
Custom ThrowHelpers
Something between if and throw (Debug.Fail or logging)
switch-case with default
Usages under net4.x/netstandard targets

Main (search for /libraries):

This PR:

ghost · 2022-05-25T01:08:31Z

Note regarding the new-api-needs-documentation label:

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.

ghost · 2022-05-25T01:08:38Z

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

Issue Details

Converting usages to possible new helpers (#69590) without /tests directories
Changed a little bit generic constraints in suggested methods due to some generic math interfaces were moved
Did not find any meaningful custom messages and dropped them all for now

Compilation errors:

A little bit cases with pointers (I'll remove them manually)
Some usages with TimeSpan, Decimal, DateTime, etc. The IComparisonOperators<TSelf, TSelf> interface removed via Determine recommended behavior of checked/unchecked operators for DateTime and similar types #67744. But I actually don't understand how exactly IComparisonOperators<TSelf, TSelf> related to overflowing and checked / unchecked. @tannergooding
A lot of usages with enums for IsNotBetween. I tried to mark them all with "ENUMUSAGE" message argument
I also added (or removed) manual casting to uint a few times

Remaining manual throwing:

More complex conditions that could be converted to the proposed API manually
A lot of custom conditions, maybe it would make sense to add ThrowIf(bool condition... overload
arg < someValue || arg >= someValue. This is really close to IsNotBetween, but not enough 😄
else if () throw OutOfRange or if () throw OutOfRange; else. Could be converted manually by removing if/else at all
Some GreaterOrEqualsThan and LessOrEqualsThan cases
Custom ThrowHelpers
Something between if and throw (Debug.Fail or logging)
Throwing in switch-case

Main (search for /libraries):

This PR:

Author:	hrrrrustic
Assignees:	-
Labels:	`area-Meta`, `new-api-needs-documentation`, `community-contribution`
Milestone:	-

jeffhandley · 2022-05-29T06:44:32Z

@hrrrrustic I assigned myself to ensure you get a review on this once the build errors are fixed.

hrrrrustic · 2022-06-06T10:23:01Z

I'll try to finish work this week. Converting to draft for now

hrrrrustic · 2022-06-27T01:41:13Z

Jobs are still failing, some errors can be fixed by reverting changes in old libraries, others related to ParamName (described it in first comment)
Since this PR should be splitted in smaller parts anyway, I hope this is enough at least for initial review in #69590

danmoseley · 2022-06-27T04:26:38Z

src/libraries/Common/src/System/IO/ReadOnlyMemoryStream.cs

-                {
-                    throw new ArgumentOutOfRangeException(nameof(value));
-                }
+                ArgumentOutOfRangeException.ThrowIfNotBetween(value, 0, int.MaxValue);


ThrowIfNotBetween suggests an exclusive range, but the code here treated the ends of the range as valid. Should it be something like ThrowIfOutsideRange?

Initially discussed here
#69590 (comment)

But can be easily changed to ThrowIfOutsideRange. Added this to questions section in issue

ThrowIfNotBetween is perhaps a good name, but i think it would not be the method you use here. Anyway, as you say, naming can be discussed at review.

danmoseley · 2022-06-27T04:28:02Z

src/libraries/Common/src/System/Net/RangeValidationHelpers.cs

-            {
-                throw new ArgumentOutOfRangeException(nameof(segment));
-            }
+            ArgumentOutOfRangeException.ThrowIf(segment.Offset < 0 || segment.Count < 0 || segment.Count > (segment.Array.Length - segment.Offset));


This no longer includes the argument name.

Ah, I see you mentioned that, but it would be good to fix to see what it looks like.

Yep, I'll add message (or paramName) arg to ThrowIf after regenerating diff. Just not sure should it be simple string or interpolation handler. Mentioned that in linked issue

danmoseley · 2022-06-27T04:29:32Z

src/libraries/Common/src/System/Text/DBCSDecoder.cs

+
+            ArgumentOutOfRangeException.ThrowIfLessThan(bytes.Length - byteIndex, byteCount);
+
+            ArgumentOutOfRangeException.ThrowIfLessThan(chars.Length - charIndex, charCount);


Some relatively simple checks here turned into a lot of method calls - maybe inlined - is the generated code larger?

I didn't check that, what's the easiest way to do that?
Also current implementation should use same private helper as ArgumentNullException for better inlining I guess

runtime/src/libraries/System.Private.CoreLib/src/System/ArgumentNullException.cs

Lines 90 to 92 in c9f80fd

[DoesNotReturn]

internal static void Throw(string? paramName) =>

throw new ArgumentNullException(paramName);

eerhardt · 2022-07-25T14:44:55Z

Closing per https://github.com/dotnet/runtime/blob/main/docs/project/api-review-process.md#pull-requests. We can reopen this PR once the API has been approved.

hrrrrustic · 2022-07-26T08:25:03Z

Closing per https://github.com/dotnet/runtime/blob/main/docs/project/api-review-process.md#pull-requests. We can reopen this PR once the API has been approved.

This PR was created to address #69590 (comment), but I think it's ok to close it until api review process

stephentoub · 2022-07-26T11:22:50Z

This PR was created to address #69590 (comment),

Thanks. We don't actually need a PR for that, since we can't "pull" anything until the APIs are approved. The goal of doing the work is to be able to tweak the APIs into they're as successful as possible, which can be done just with local changes and a commit linked from the API issue. I appreciate your efforts here.

add helpers

4258f64

ghost added community-contribution Indicates that the PR has been added by a community member area-Meta new-api-needs-documentation labels May 25, 2022

jeffhandley self-assigned this May 29, 2022

hrrrrustic marked this pull request as draft June 6, 2022 10:23

hrrrrustic added 13 commits June 13, 2022 00:50

Merge branch 'main' into outOfRange

2da4ff6

Simplify if-else

8db21c2

Simplify double check

b036371

ThrowIfZero

7e054ee

ThrowIfNegative

78539d0

ThrowIfNegativeOrZero

a059b20

ThrowIfGreaterThan

3b5e61e

ThrowIfLessThan

a39f676

ThrowIfLessThanOrEqual

a1d3120

ThrowIfGreaterThanOrEqual

5b02a7c

ThrowIfNotBetween

94d77d6

ThrowIf

5a58f3a

ThrowIfEnum

d578c7f

hrrrrustic force-pushed the outOfRange branch from f2e1867 to d578c7f Compare June 14, 2022 21:46

hrrrrustic added 6 commits June 15, 2022 01:03

fix methods

edaadc5

ThrowIfEnum -> ThrowIf

e69d1e6

build fixes

05cee19

Merge remote-tracking branch 'origin/main' into outOfRange

e4dfdae

remove useless(?) check

328a89d

revert old targets

1f998cd

hrrrrustic added 5 commits June 22, 2022 02:50

remove whitespace

4261f81

more old targets reverts

f432b62

fix

f71399c

fixes

4d11482

fixes

bb1b8f1

hrrrrustic marked this pull request as ready for review June 27, 2022 01:43

hrrrrustic mentioned this pull request Jun 27, 2022

[API Proposal]: ArgumentOutOfRangeException helper methods #69590

Closed

danmoseley reviewed Jun 27, 2022

View reviewed changes

eerhardt closed this Jul 25, 2022

ghost locked as resolved and limited conversation to collaborators Aug 25, 2022


		ArgumentOutOfRangeException.ThrowIfLessThan(bytes.Length - byteIndex, byteCount);

		ArgumentOutOfRangeException.ThrowIfLessThan(chars.Length - charIndex, charCount);

	[DoesNotReturn]
	internal static void Throw(string? paramName) =>
	throw new ArgumentNullException(paramName);

Comments

Conversation

hrrrrustic commented May 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented May 25, 2022

Uh oh!

ghost commented May 25, 2022

Uh oh!

jeffhandley commented May 29, 2022

Uh oh!

hrrrrustic commented Jun 6, 2022

Uh oh!

hrrrrustic commented Jun 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eerhardt commented Jul 25, 2022

Uh oh!

hrrrrustic commented Jul 26, 2022

Uh oh!

stephentoub commented Jul 26, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

hrrrrustic commented May 25, 2022 •

edited

Loading

hrrrrustic commented Jun 27, 2022 •

edited

Loading