Skip to content

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

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

Merged
buyaa-n 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 all 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 @@ -131,7 +131,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

@stephentoub stephentoub Jun 29, 2020

Copy link
Copy Markdown
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?

@buyaa-n buyaa-n Jun 29, 2020

Copy link
Copy Markdown
Contributor 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
3 changes: 3 additions & 0 deletions src/libraries/System.Drawing.Common/src/System/Drawing/Bitmap.Windows.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@ public sealed partial class Bitmap
{
public Bitmap(Type type, string resource)
{
if (resource == null)
throw new ArgumentNullException(nameof(resource));

Stream? stream = type.Module.Assembly.GetManifestResourceStream(type, resource);
if (stream == 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 ArgumentNullException(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 ArgumentNullException(nameof(family));

IntPtr sformat = (format == null) ? IntPtr.Zero : format.nativeFormat;
// note: the NullReferenceException on s.Length is the expected (MS) exception
Expand Down
14 changes: 14 additions & 0 deletions src/libraries/System.Drawing.Common/src/System/Drawing/Drawing2D/GraphicsPath.Windows.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,6 +294,8 @@ public unsafe void AddLines(PointF[] points)
{
if (points == null)
throw new ArgumentNullException(nameof(points));
if (points.Length == 0)
throw new ArgumentException(null, nameof(points));

fixed (PointF* p = points)
{
Expand All @@ -312,6 +314,8 @@ public unsafe void AddLines(Point[] points)
{
if (points == null)
throw new ArgumentNullException(nameof(points));
if (points.Length == 0)
throw new ArgumentException(null, nameof(points));

fixed (Point* p = points)
{
Expand Down Expand Up @@ -526,6 +530,8 @@ public unsafe void AddRectangles(RectangleF[] rects)
{
if (rects == null)
throw new ArgumentNullException(nameof(rects));
if (rects.Length == 0)
throw new ArgumentException(null, nameof(rects));

fixed (RectangleF* r = rects)
{
Expand All @@ -545,6 +551,8 @@ public unsafe void AddRectangles(Rectangle[] rects)
{
if (rects == null)
throw new ArgumentNullException(nameof(rects));
if (rects.Length == 0)
throw new ArgumentException(null, nameof(rects));

fixed (Rectangle* r = rects)
{
Expand Down Expand Up @@ -639,6 +647,9 @@ public void AddString(string s, FontFamily family, int style, float emSize, Poin

public void AddString(string s, FontFamily family, int style, float emSize, RectangleF layoutRect, StringFormat? format)
{
if (family == null)
throw new ArgumentNullException(nameof(family));

Gdip.CheckStatus(Gdip.GdipAddPathString(
new HandleRef(this, _nativePath),
s,
Expand All @@ -652,6 +663,9 @@ public void AddString(string s, FontFamily family, int style, float emSize, Rect

public void AddString(string s, FontFamily family, int style, float emSize, Rectangle layoutRect, StringFormat? format)
{
if (family == null)
throw new ArgumentNullException(nameof(family));

Gdip.CheckStatus(Gdip.GdipAddPathStringI(
new HandleRef(this, _nativePath),
s,
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));

@buyaa-n buyaa-n Jun 30, 2020

Copy link
Copy Markdown
Contributor 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

@bartonjs bartonjs Jun 30, 2020

Copy link
Copy Markdown
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
14 changes: 13 additions & 1 deletion src/libraries/System.Drawing.Common/src/System/Drawing/Icon.Windows.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,9 @@ public Icon(Icon original, int width, int height) : this()

public Icon(Type type, string resource) : this()
{
if (resource == null)
throw new ArgumentNullException(nameof(resource));

Stream? stream = type.Module.Assembly.GetManifestResourceStream(type, resource);
if (stream == null)
{
Expand Down Expand Up @@ -167,6 +170,9 @@ void ISerializable.GetObjectData(SerializationInfo si, StreamingContext context)
throw new ArgumentNullException(nameof(filePath));
}

if (string.IsNullOrEmpty(filePath))
throw new ArgumentException(SR.NullOrEmptyPath, nameof(filePath));

filePath = Path.GetFullPath(filePath);
if (!File.Exists(filePath))
{
Expand Down Expand Up @@ -440,7 +446,13 @@ internal void DrawUnstretched(Graphics graphics, Rectangle targetRect)

~Icon() => Dispose(false);

public static Icon FromHandle(IntPtr handle) => new Icon(handle);
public static Icon FromHandle(IntPtr handle)
{
if (handle == IntPtr.Zero)
throw new ArgumentException(null, nameof(handle));

return new Icon(handle);
}

// Initializes this Image object. This is identical to calling the image's
// constructor with picture, but this allows non-constructor initialization,
Expand Down
7 changes: 6 additions & 1 deletion src/libraries/System.Drawing.Common/tests/BitmapTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,6 @@ public void Ctor_NullType_ThrowsNullReferenceException()

[ActiveIssue("https://github.com/dotnet/runtime/issues/22221", TestPlatforms.AnyUnix)]
[ConditionalTheory(Helpers.IsDrawingSupported)]
[InlineData(typeof(Bitmap), null)]
[InlineData(typeof(Bitmap), "")]
[InlineData(typeof(Bitmap), "bitmap_173x183_indexed_8bit.bmp")]
[InlineData(typeof(BitmapTests), "bitmap_173x183_INDEXED_8bit.bmp")]
Expand All @@ -121,6 +120,12 @@ public void Ctor_InvalidResource_ThrowsArgumentException(Type type, string resou
AssertExtensions.Throws<ArgumentException>(null, () => new Bitmap(type, resource));
}

[ConditionalFact(Helpers.IsDrawingSupported)]
public void Ctor_InvalidResource_ThrowsArgumentNullException()
{
AssertExtensions.Throws<ArgumentNullException, ArgumentException>("resource", null, () => new Bitmap(typeof(Bitmap), null));
}

[ConditionalTheory(Helpers.IsDrawingSupported)]
[MemberData(nameof(Ctor_FilePath_TestData))]
public void Ctor_Stream(string filename, int width, int height, PixelFormat pixelFormat, ImageFormat rawFormat)
Expand Down
8 changes: 4 additions & 4 deletions src/libraries/System.Drawing.Common/tests/Drawing2D/GraphicsPathTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -370,8 +370,8 @@ public void AddLines_PointsNull_ThrowsArgumentNullException()
[ConditionalFact(Helpers.IsDrawingSupported)]
public void AddLines_ZeroPoints_ThrowsArgumentException()
{
AssertExtensions.Throws<ArgumentException>(null, () => new GraphicsPath().AddLines(new Point[0]));
AssertExtensions.Throws<ArgumentException>(null, () => new GraphicsPath().AddLines(new PointF[0]));
AssertExtensions.Throws<ArgumentException>("points", null, () => new GraphicsPath().AddLines(new Point[0]));
AssertExtensions.Throws<ArgumentException>("points", null, () => new GraphicsPath().AddLines(new PointF[0]));
}

[ActiveIssue("https://github.com/dotnet/runtime/issues/22221", TestPlatforms.AnyUnix)]
Expand Down Expand Up @@ -1262,11 +1262,11 @@ public void AddString_StringNull_ThrowsNullReferenceException()
}

[ConditionalFact(Helpers.IsDrawingSupported)]
public void AddString_FontFamilyNull_ThrowsArgumentException()
public void AddString_FontFamilyNull_ThrowsArgumentNullException()
{
using (GraphicsPath gp = new GraphicsPath())
{
AssertExtensions.Throws<ArgumentException>(null, () =>
AssertExtensions.Throws<ArgumentNullException, ArgumentException>("family", null, () =>
new GraphicsPath().AddString("mono", null, 0, 10, new Point(10, 10), StringFormat.GenericDefault));
}
}
Expand Down
11 changes: 8 additions & 3 deletions src/libraries/System.Drawing.Common/tests/IconTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,7 +283,6 @@ public void Ctor_NullType_ThrowsNullReferenceException()
}

[ConditionalTheory(Helpers.IsDrawingSupported)]
[InlineData(typeof(Icon), null)]
[InlineData(typeof(Icon), "")]
[InlineData(typeof(Icon), "48x48_multiple_entries_4bit.ico")]
[InlineData(typeof(IconTests), "48x48_MULTIPLE_entries_4bit.ico")]
Expand All @@ -292,6 +291,12 @@ public void Ctor_InvalidResource_ThrowsArgumentException(Type type, string resou
AssertExtensions.Throws<ArgumentException>(null, () => new Icon(type, resource));
}

[ConditionalFact(Helpers.IsDrawingSupported)]
public void Ctor_InvalidResource_ThrowsArgumentNullException()
{
AssertExtensions.Throws<ArgumentNullException, ArgumentException>("resource", null, () => new Icon(typeof(Icon), null));
}

[ConditionalFact(Helpers.IsDrawingSupported)]
public void Clone_ConstructedIcon_Success()
{
Expand Down Expand Up @@ -437,7 +442,7 @@ public void ExtractAssociatedIcon_NullFilePath_ThrowsArgumentNullException()
[ConditionalFact(Helpers.IsDrawingSupported)]
public void ExtractAssociatedIcon_InvalidFilePath_ThrowsArgumentException()
{
AssertExtensions.Throws<ArgumentException>("path", null, () => Icon.ExtractAssociatedIcon(""));
AssertExtensions.Throws<ArgumentException>("filePath", null, () => Icon.ExtractAssociatedIcon(""));
}


Expand Down Expand Up @@ -759,7 +764,7 @@ public void FromHandle_BitmapHandleMultipleTime_Success()
[ConditionalFact(Helpers.IsDrawingSupported)]
public void FromHandle_Zero_ThrowsArgumentException()
{
AssertExtensions.Throws<ArgumentException>(null, () => Icon.FromHandle(IntPtr.Zero));
AssertExtensions.Throws<ArgumentException>("handle", null, () => Icon.FromHandle(IntPtr.Zero));
}

[ConditionalFact(Helpers.IsDrawingSupported)]
Expand Down
2 changes: 1 addition & 1 deletion src/libraries/System.Drawing.Common/tests/mono/System.Drawing/BitmapTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1304,7 +1304,7 @@ public void BitmapTypeStringCtor1()
[ConditionalFact(Helpers.IsDrawingSupported)]
public void BitmapTypeStringCtor2()
{
Assert.Throws<ArgumentException>(() => new Bitmap(typeof(Bitmap), null));
AssertExtensions.Throws<ArgumentNullException, ArgumentException>("resource", null, () => new Bitmap(typeof(Bitmap), null));
}

private void SetResolution(float x, float y)
Expand Down