-
Notifications
You must be signed in to change notification settings - Fork 5.5k
Fix warnings found by analyzer "Instantiate argument exceptions correctly" for CoreClr #35147
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1896,7 +1896,7 @@ private static void ThrowIfTypeNeverValidGenericArgument(RuntimeType type) | |
| internal static void SanityCheckGenericArguments(RuntimeType[] genericArguments, RuntimeType[] genericParamters) | ||
| { | ||
| if (genericArguments == null) | ||
| throw new ArgumentNullException(); | ||
| throw new ArgumentNullException(nameof(genericArguments)); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
|
||
| for (int i = 0; i < genericArguments.Length; i++) | ||
| { | ||
|
|
@@ -2752,7 +2752,7 @@ public override InterfaceMapping GetInterfaceMap(Type ifaceType) | |
| protected override PropertyInfo? GetPropertyImpl( | ||
| string name, BindingFlags bindingAttr, Binder? binder, Type? returnType, Type[]? types, ParameterModifier[]? modifiers) | ||
| { | ||
| if (name == null) throw new ArgumentNullException(); | ||
| if (name == null) throw new ArgumentNullException(nameof(name)); | ||
|
|
||
| ListBuilder<PropertyInfo> candidates = GetPropertyCandidates(name, bindingAttr, types, false); | ||
|
|
||
|
|
@@ -2788,7 +2788,7 @@ public override InterfaceMapping GetInterfaceMap(Type ifaceType) | |
|
|
||
| public override EventInfo? GetEvent(string name, BindingFlags bindingAttr) | ||
| { | ||
| if (name is null) throw new ArgumentNullException(); | ||
| if (name is null) throw new ArgumentNullException(nameof(name)); | ||
|
|
||
| FilterHelper(bindingAttr, ref name, out _, out MemberListType listType); | ||
|
|
||
|
|
@@ -2814,7 +2814,7 @@ public override InterfaceMapping GetInterfaceMap(Type ifaceType) | |
|
|
||
| public override FieldInfo? GetField(string name, BindingFlags bindingAttr) | ||
| { | ||
| if (name is null) throw new ArgumentNullException(); | ||
| if (name is null) throw new ArgumentNullException(nameof(name)); | ||
|
|
||
| FilterHelper(bindingAttr, ref name, out _, out MemberListType listType); | ||
|
|
||
|
|
@@ -2851,7 +2851,7 @@ public override InterfaceMapping GetInterfaceMap(Type ifaceType) | |
|
|
||
| public override Type? GetInterface(string fullname, bool ignoreCase) | ||
| { | ||
| if (fullname is null) throw new ArgumentNullException(); | ||
| if (fullname is null) throw new ArgumentNullException(nameof(fullname)); | ||
|
|
||
| BindingFlags bindingAttr = BindingFlags.Public | BindingFlags.NonPublic; | ||
|
|
||
|
|
@@ -2885,7 +2885,7 @@ public override InterfaceMapping GetInterfaceMap(Type ifaceType) | |
|
|
||
| public override Type? GetNestedType(string fullname, BindingFlags bindingAttr) | ||
| { | ||
| if (fullname is null) throw new ArgumentNullException(); | ||
| if (fullname is null) throw new ArgumentNullException(nameof(fullname)); | ||
|
|
||
| bindingAttr &= ~BindingFlags.Static; | ||
| string name, ns; | ||
|
|
@@ -2913,7 +2913,7 @@ public override InterfaceMapping GetInterfaceMap(Type ifaceType) | |
|
|
||
| public override MemberInfo[] GetMember(string name, MemberTypes type, BindingFlags bindingAttr) | ||
| { | ||
| if (name is null) throw new ArgumentNullException(); | ||
| if (name is null) throw new ArgumentNullException(nameof(name)); | ||
|
|
||
| ListBuilder<MethodInfo> methods = default; | ||
| ListBuilder<ConstructorInfo> constructors = default; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -186,8 +186,8 @@ public void OffsetOf_NullType_ThrowsArgumentNullException() | |
| [Fact] | ||
| public void OffsetOf_NullFieldName_ThrowsArgumentNullException() | ||
| { | ||
| AssertExtensions.Throws<ArgumentNullException>(null, () => Marshal.OffsetOf(new object().GetType(), null)); | ||
| AssertExtensions.Throws<ArgumentNullException>(null, () => Marshal.OffsetOf<object>(null)); | ||
| AssertExtensions.Throws<ArgumentNullException>("name", () => Marshal.OffsetOf(new object().GetType(), null)); | ||
| AssertExtensions.Throws<ArgumentNullException>("name", () => Marshal.OffsetOf<object>(null)); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If Marshal.OffsetOf were a more entry-level method, I'd advocate taking a change for it to validate the field name directly so it could throw the ANE with the correct parameter name ("fieldName"). But I'm guessing our position right now is "it's an advanced enough API that the caller is expected to realize the parameter sharing between OffsetOf's fieldName and GetField's name." Writing this down in case someone else is iffy about it and wants to have the discussion. But currently sighing and not recommending further action. |
||
| } | ||
|
|
||
| [Fact] | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would have thought we wanted Warnings here (with everything fixed or suppressed by this PR)
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have discussed to upgrade severity level from hidden to info, so I thought I should make it same here, but yes maybe we want to make it warning to get full benefit, we might need lots of suppress though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Talked with @bartonjs we will make it warning after fixing/suppressing all warnings. For now, I will leave this PR on hold until applying his suggestions on improving analyzer:
Then we might have much less warning need to fix/suppress, and i might update this PR with all repo fix (coreclr+libs+mono)