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.

Sign up for GitHub

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

ArgumentOutOfRangeException: use corresponding default message for all overloads #89846

Closed
wants to merge 3 commits into from
Closed

ArgumentOutOfRangeException: use corresponding default message for all overloads #89846

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7f0c322
ArgumentOutOfRangeException: use corresponding default message for al…
OwnageIsMagic Aug 2, 2023
3f624ff
fix ws
OwnageIsMagic Aug 2, 2023
e404ef9
address review
OwnageIsMagic Aug 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 5 additions & 14 deletions src/libraries/System.Private.CoreLib/src/System/ArgumentOutOfRangeException.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,31 +22,22 @@ public class ArgumentOutOfRangeException : ArgumentException
// Creates a new ArgumentOutOfRangeException with its message
// string set to a default message explaining an argument was out of range.
public ArgumentOutOfRangeException()
: base(SR.Arg_ArgumentOutOfRangeException)
{
HResult = HResults.COR_E_ARGUMENTOUTOFRANGE;
}
: this(null, null, null) {}

public ArgumentOutOfRangeException(string? paramName)
: base(SR.Arg_ArgumentOutOfRangeException, paramName)
{
HResult = HResults.COR_E_ARGUMENTOUTOFRANGE;
}
: this(paramName, null, null) {}

public ArgumentOutOfRangeException(string? paramName, string? message)
: base(message, paramName)
{
HResult = HResults.COR_E_ARGUMENTOUTOFRANGE;
}
: this(paramName, null, message) {}

public ArgumentOutOfRangeException(string? message, Exception? innerException)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is missing the same treatment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is consistent with other *Exception classes. I think this should be changed in another PR targeting all Exception derived classes
https://github.com/OwnageIsMagic/runtime/blob/3f624ff6fc8e347efde954744879c075efb1411b/src/libraries/System.Private.CoreLib/src/System/ArgumentException.cs#L44
https://github.com/OwnageIsMagic/runtime/blob/3f624ff6fc8e347efde954744879c075efb1411b/src/libraries/System.Private.CoreLib/src/System/MissingMethodException.cs#L30

Copy link
Member

@stephentoub stephentoub Aug 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But it's inconsistent with itself. There's no reason a null message to the different ctors should use a different message. Either we change them all on this type or we don't change any on this type.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about other classes (string, Exception) ctor? Another PR?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there are other types where this would make sense, we can use this PR to do so.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So there is no point constraining this to CoreLib.
This will affect all exception classes CoreLib (ls -1 **/*Exception.cs | wc -l # -> 100) and some in other libs
i.e.

runtime/src/libraries/System.Data.Common/src/System/Data/DataException.cs

Lines 26 to 31 in 3b21b90

public DataException(string? s) : base(s)
{
HResult = HResults.Data;
}
public DataException(string? s, Exception? innerException) : base(s, innerException) { }

runtime/src/libraries/System.Security.Principal.Windows/src/System/Security/Principal/IdentityNotMappedException.cs

Lines 20 to 31 in 3b21b90

public IdentityNotMappedException(string? message)
: base(message)
{
}
public IdentityNotMappedException(string? message, Exception? inner)
: base(message, inner)
{
}
internal IdentityNotMappedException(string? message, IdentityReferenceCollection? unmappedIdentities)
: this(message)

117 classes, but not every Exception out here have custom message

Are you ok with proposed pattern with constructor delegation?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you ok with proposed pattern with constructor delegation?

I'd prefer we just add ?? SR.Whatever where each message is passed along to the base.

Delegating from ctor to ctor unnecessarily keeps all larger constructors rooted and prevents them from being trimmed away when the smaller one is being used. It's also a lot more churn.

: base(message, innerException)
: base(message ?? SR.Arg_ArgumentOutOfRangeException, innerException)
{
HResult = HResults.COR_E_ARGUMENTOUTOFRANGE;
}

public ArgumentOutOfRangeException(string? paramName, object? actualValue, string? message)
: base(message, paramName)
: base(message ?? SR.Arg_ArgumentOutOfRangeException, paramName)
{
_actualValue = actualValue;
HResult = HResults.COR_E_ARGUMENTOUTOFRANGE;
Expand Down