Turn on argument exception analyzer on runtime, fix warnings found #38578
Conversation
| throw new ArgumentException(SR.Arg_ArrayLengthsDiffer, "namedProperties, propertyValues"); | ||
| if (namedFields.Length != fieldValues.Length) | ||
| throw new ArgumentException(SR.Arg_ArrayLengthsDiffer, "namedFields, fieldValues"); | ||
| #pragma warning restore CA2208 |
There was a problem hiding this comment.
Just to confirm, we're happy with these argument exception names?
There was a problem hiding this comment.
combination of arguments found 2-3 times in previous PR, and we have suppressed it. I would say we are OK with it instead of happy 😄. We could change the analyzer to not warn in this case but not sure the amount of work for calculating that worth for this rare case scenario
| { | ||
| if (resource == null) | ||
| throw new ArgumentException(nameof(resource)); | ||
| throw new ArgumentNullException(nameof(resource)); |
There was a problem hiding this comment.
AssertExtensions.Throws<ArgumentException>(null, () => new Icon(typeof(Icon), null)); test is failing as the Windows implementation of this method is not checking this condition and not throwing, instead directly calling type.GetTypeInfo().Assembly.GetManifestResourceStream(type, resource) which is called for Unix implementation after these checks in row 249.
This kind of pattern happening for most of the Unix implementation updates in this PR and causing test failures. I think parameterName update for ArgumentExceptions is fine, i can fix tests by checking the OS, but is it OK to throw different exceptions (ArgumentNullException instead ArgumentException) for the same method but different OS?
CC @bartonjs @stephentoub
There was a problem hiding this comment.
Absent other data, it sounds like we should change the Windows implementation to throw the appropriate ArgumentNullException.
|
@safern Any objection to the exception normalization across OSes here? It seems like an improvement to me. |
Related to #33763
Most warnings fixed with #35717 but the analyzer wasn't turned with the PR as it needs updated analyzer. Now the runtime repo is using updated analyzers so we can turn it on