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

Jump to bottom

Turn on argument exception analyzer on runtime, fix warnings found #38578

Merged
merged 5 commits into from
Jul 2, 2020
Merged

Turn on argument exception analyzer on runtime, fix warnings found #38578

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
900e3ab
Turn on argument exception analyzer on runtime, fix failures found
buyaa-n Jun 29, 2020
ec7228b
Fix warnings on linux build
buyaa-n Jun 29, 2020
d2bd508
Merge branch 'master' of https://github.com/dotnet/runtime into turn_…
buyaa-n Jul 1, 2020
61c8827
Throw same exceptions in Windows as in unix, fix unit tests
buyaa-n Jul 1, 2020
21c3cf6
Fix netFx tests
buyaa-n Jul 1, 2020
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
2 changes: 1 addition & 1 deletion .editorconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ csharp_space_between_square_brackets = false

# Analyzers
dotnet_code_quality.ca1802.api_surface = private, internal
dotnet_code_quality.CA2208.api_surface = public
dotnet_code_quality.ca2208.api_surface = public

# C++ Files
[*.{cpp,h,in}]
Expand Down
2 changes: 1 addition & 1 deletion eng/CodeAnalysis.ruleset
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@
<Rule Id="CA2200" Action="Warning" /> <!-- Rethrow to preserve stack details. -->
<Rule Id="CA2201" Action="None" /> <!-- Do not raise reserved exception types -->
<Rule Id="CA2207" Action="Warning" /> <!-- Initialize value type static fields inline -->
<Rule Id="CA2208" Action="Info" /> <!-- Instantiate argument exceptions correctly -->
<Rule Id="CA2208" Action="Warning" /> <!-- Instantiate argument exceptions correctly -->
<Rule Id="CA2211" Action="None" /> <!-- Non-constant fields should not be visible -->
<Rule Id="CA2213" Action="None" /> <!-- Disposable fields should be disposed -->
<Rule Id="CA2214" Action="None" /> <!-- Do not call overridable methods in constructors -->
Expand Down
2 changes: 2 additions & 0 deletions src/coreclr/src/System.Private.CoreLib/src/System/Reflection/Emit/CustomAttributeBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,10 +63,12 @@ public CustomAttributeBuilder(ConstructorInfo con, object?[] constructorArgs, Pr
throw new ArgumentNullException(nameof(namedFields));
if (fieldValues == null)
throw new ArgumentNullException(nameof(fieldValues));
#pragma warning disable CA2208 // Instantiate argument exceptions correctly, combination of arguments used
if (namedProperties.Length != propertyValues.Length)
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
Copy link
Member

Choose a reason for hiding this comment

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

Just to confirm, we're happy with these argument exception names?

Copy link
Member Author

Choose a reason for hiding this comment

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

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 ((con.Attributes & MethodAttributes.Static) == MethodAttributes.Static ||
(con.Attributes & MethodAttributes.MemberAccessMask) == MethodAttributes.Private)
Expand Down
2 changes: 1 addition & 1 deletion src/libraries/System.Drawing.Common/src/System/Drawing/Bitmap.Unix.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ public Bitmap(Stream stream, bool useIcm)
public Bitmap(Type type, string resource)
{
if (resource == null)
throw new ArgumentException(nameof(resource));
throw new ArgumentNullException(nameof(resource));

// For compatibility with the .NET Framework
if (type == null)
Expand Down
12 changes: 6 additions & 6 deletions src/libraries/System.Drawing.Common/src/System/Drawing/Drawing2D/GraphicsPath.Unix.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -381,7 +381,7 @@ public void AddLines(Point[] points)
if (points == null)
throw new ArgumentNullException(nameof(points));
if (points.Length == 0)
throw new ArgumentException(nameof(points));
throw new ArgumentException(null, nameof(points));

int status = Gdip.GdipAddPathLine2I(_nativePath, points, points.Length);
Gdip.CheckStatus(status);
Expand All @@ -392,7 +392,7 @@ public void AddLines(PointF[] points)
if (points == null)
throw new ArgumentNullException(nameof(points));
if (points.Length == 0)
throw new ArgumentException(nameof(points));
throw new ArgumentException(null, nameof(points));

int status = Gdip.GdipAddPathLine2(_nativePath, points, points.Length);
Gdip.CheckStatus(status);
Expand Down Expand Up @@ -464,7 +464,7 @@ public void AddRectangles(Rectangle[] rects)
if (rects == null)
throw new ArgumentNullException(nameof(rects));
if (rects.Length == 0)
throw new ArgumentException(nameof(rects));
throw new ArgumentException(null, nameof(rects));

int status = Gdip.GdipAddPathRectanglesI(_nativePath, rects, rects.Length);
Gdip.CheckStatus(status);
Expand All @@ -475,7 +475,7 @@ public void AddRectangles(RectangleF[] rects)
if (rects == null)
throw new ArgumentNullException(nameof(rects));
if (rects.Length == 0)
throw new ArgumentException(nameof(rects));
throw new ArgumentException(null, nameof(rects));

int status = Gdip.GdipAddPathRectangles(_nativePath, rects, rects.Length);
Gdip.CheckStatus(status);
Expand Down Expand Up @@ -642,7 +642,7 @@ public void AddString(string s, FontFamily family, int style, float emSize, Poin
public void AddString(string s, FontFamily family, int style, float emSize, Rectangle layoutRect, StringFormat? format)
{
if (family == null)
throw new ArgumentException(nameof(family));
throw new ArgumentException(null, nameof(family));

IntPtr sformat = (format == null) ? IntPtr.Zero : format.nativeFormat;
// note: the NullReferenceException on s.Length is the expected (MS) exception
Expand All @@ -653,7 +653,7 @@ public void AddString(string s, FontFamily family, int style, float emSize, Rect
public void AddString(string s, FontFamily family, int style, float emSize, RectangleF layoutRect, StringFormat? format)
{
if (family == null)
throw new ArgumentException(nameof(family));
throw new ArgumentException(null, nameof(family));

IntPtr sformat = (format == null) ? IntPtr.Zero : format.nativeFormat;
// note: the NullReferenceException on s.Length is the expected (MS) exception
Expand Down
8 changes: 4 additions & 4 deletions src/libraries/System.Drawing.Common/src/System/Drawing/Icon.Unix.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,7 @@ public Icon(Icon original, Size size)
}

if (id == ushort.MaxValue)
throw new ArgumentException(SR.NoValidIconImageFound, "Icon");
throw new ArgumentException(SR.NoValidIconImageFound, nameof(original));

iconSize.Height = iconDir.idEntries[id].height;
iconSize.Width = iconDir.idEntries[id].width;
Expand Down Expand Up @@ -240,7 +240,7 @@ public Icon(string fileName)
public Icon(Type type, string resource)
{
if (resource == null)
throw new ArgumentException(nameof(resource));
throw new ArgumentNullException(nameof(resource));
Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Absent other data, it sounds like we should change the Windows implementation to throw the appropriate ArgumentNullException.


// For compatibility with the .NET Framework
if (type == null)
Expand Down Expand Up @@ -313,7 +313,7 @@ public static Icon ExtractAssociatedIcon(string filePath)
if (filePath == null)
throw new ArgumentNullException(nameof(filePath));
if (string.IsNullOrEmpty(filePath))
throw new ArgumentException(SR.NullOrEmptyPath, "path");
throw new ArgumentException(SR.NullOrEmptyPath, nameof(filePath));
if (!File.Exists(filePath))
throw new FileNotFoundException(SR.CouldntFindSpecifiedFile, filePath);

Expand Down Expand Up @@ -346,7 +346,7 @@ public object Clone()
public static Icon FromHandle(IntPtr handle)
{
if (handle == IntPtr.Zero)
throw new ArgumentException(nameof(handle));
throw new ArgumentException(null, nameof(handle));

return new Icon(handle);
}
Expand Down