Use corresponding exception message, if null passed #90505
Conversation
ghost
added
the
community-contribution
dotnet-issue-labeler
bot
added
the
needs-area-label
This comment was marked as resolved.
This comment was marked as resolved.
|
Do I read correctly that only those 4 test failing |
As of the latest run there appear to be a number of other failure from other CI legs as well, like
|
|
@stephentoub I had some thoughts on this: I think this should be reverted for some types that are well-known base types (mainly |
marek-safar
added
area-System.Runtime
and removed
needs-area-label
|
Tagging subscribers to this area: @dotnet/area-system-runtime Issue DetailsCloses #89783 Notes:
Missing custom message
|
There was a problem hiding this comment.
Thanks for the PR @OwnageIsMagic. I'm curious how did you find all the exceptions in src/libraries?
"Exception of type 'SomeSpecificException' was thrown." looks more preferable than unintelligible "System error." if user logs only Message property without ToString().
I actually like more the consistency between the parameterless ctor and the other ones.
src/libraries/System.Private.CoreLib/src/System/InsufficientMemoryException.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/OutOfMemoryException.cs
Outdated
Show resolved
Hide resolved
...tem.Text.RegularExpressions/src/System/Text/RegularExpressions/RegexMatchTimeoutException.cs
Outdated
Show resolved
Hide resolved
...tem.Text.RegularExpressions/src/System/Text/RegularExpressions/RegexMatchTimeoutException.cs
Outdated
Show resolved
Hide resolved
|
@jozkee some .Net conventions + shell hackery and then manual diff review. $files = ls -Filter *Exception.cs -Recurse -File |% {[string]$_}
foreach ($f in $files) {
$c=gc $f -Raw
if ($c -cmatch '(?:base)\((SR\.(?!Format\(|GetString)[A-Za-z0-9_]+)') {
if ($Matches.Count -gt 2) { echo 'Count' $f $Matches }
Set-Content -LiteralPath $f -Value ($c -creplace 'base\(message' , "base(message ?? $($Matches[1])") -NoNewline
}
else { echo $f }
}What about changing those types default message to also include specific type if As a native English speaker, what do you think about my nametag? Do you find it inappropriate/aggressive or something like that? |
|
This PR cuts on "technical" aspects (aot size, microbenchmarks values) and brings in "aesthetics" (nice error messages in en culture, god save you from localized ones). This fits perfectly with .NET Framework culture, but I'm not sure about more rough and edgy Core. Maybe you can /cc some platform visionaries? |
|
@OwnageIsMagic how does this affects MicroBenchmarks? Does it affect non-throwing scenarios? |
This comment was marked as off-topic.
This comment was marked as off-topic.
I do not see a problem with this change wrt AOT app size impact. |
|
@jozkee sorry for confusion. The impact is around 10-60 bytes per exception type (resource is no longer trimmable) per locale (Core doesn't have any beside EN, but I'm not sure about Mono, etc). For benchmarks, I was referring to exception object creation (without user msg) now most of the time will go through ResourceManager with bunch of thread local variables. Yeah, usually it's a throwing scenario. |
I think we usually don't worry for the perf of throwing scenarios and I think this wouldn't have a significant impact. |
| // \p{Pi} any kind of opening quote https://www.compart.com/en/unicode/category/Pi | ||
| // \p{Pf} any kind of closing quote https://www.compart.com/en/unicode/category/Pf | ||
| // \p{Po} any kind of punctuation character that is not a dash, bracket, quote or connector https://www.compart.com/en/unicode/category/Po | ||
| Assert.Matches(@"[\p{Pi}\p{Po}]" + Regex.Escape(typeof(ConfigurationErrorsException).FullName) + @"[\p{Pf}\p{Po}]", cee.BareMessage); |
There was a problem hiding this comment.
I quite impressed by people spending their free time writing exception constructor unit tests.
OwnageIsMagic
force-pushed
the
patch-1
branch
from
65e25d0 to
48e8625
Compare
|
@jozkee there is problematic pattern with exceptions thrown by runtime ( var e = new ArgumentException(null);
e.HResult = 0;
Console.WriteLine(e);
// System.ArgumentException: Exception of type 'System.ArgumentException' was thrown.
e.HResult = -2147024809; // COR_E_ARGUMENT
Console.WriteLine(e); // message changed
// System.ArgumentException: Value does not fall within the expected range.
e.HResult = 0;
Console.WriteLine(e); // message didn't change back: ToString mutates state -- this is not nice
// System.ArgumentException: Value does not fall within the expected range.This can be worked around by introducing |
|
If |
|
@jkotas the problem is that they derive from another exception which sets message to non-null. Look at this call chain: |
|
Is there any reason for that |
There was a problem hiding this comment.
@OwnageIsMagic From what I can tell, we're stalled on this pull request because it has failing CI legs that illustrate the behavioral changes.
To move this forward efficiently, could you review the failing tests and author a comment that summarizes where the behavioral changes are being observed, showing the previous and updated exception messages?
If we agree to those changes, then we'd need updates sent to the PR that update the affected tests.
ghost
added
the
needs-author-action
OwnageIsMagic
force-pushed
the
patch-1
branch
from
754e28d to
53988f1
Compare
Missing timeout? |
|
Is there anything I can do beside closing and reopening PR to get CI green? |
|
Sorry, I was out for the holidays. I pinged @dotnet/mono-team hoping they could give a better insight on #90505 (comment). Will reopen the PR to see if we are still getting errors. |
|
@jozkee should I report issues with infrastructure? Pipeline didn't timed out for 9 days. Btw, all green, thanks for your magic touch. |
|
@jozkee the mono issue seems to be resolved now, right? |
|
@marek-safar yes, it went away on its own. |
* use corresponding exception message if null passed * fix * review fixes * fix tests * do not use default message if null passed for SystemException, ApplicationException, IOException * change COMException default message --------- Co-authored-by: David Cantú <[email protected]>
Closes #89783
Notes:
Some new code already using pattern like this.
runtime/src/libraries/System.Formats.Cbor/src/System/Formats/Cbor/CborContentException.cs
Line 18 in 09af161
override string MessageviaSetMessageField(), can't find where it defined.runtime/src/libraries/System.Private.CoreLib/src/System/TypeLoadException.cs
Lines 32 to 39 in 09af161
Missing custom message
@stephentoub