Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
761aefb
Removed redundant suppressions from System.Private.CoreLib
jkurdek Aug 2, 2022
bbbc1e7
Removed redundant suppressions from System.Text.Json
jkurdek Aug 2, 2022
75abc6c
Removed redundant suppressions from System.Net.Http.Json
jkurdek Aug 2, 2022
060a478
Removed redundant suppressions from System.Private.DataContractSerial…
jkurdek Aug 2, 2022
0e6e96a
Removed redundant suppressions from System.Linq.Queryable
jkurdek Aug 2, 2022
8438461
Removed redundant suppressions from System.Reflection.DispatchProxy
jkurdek Aug 2, 2022
3d15fcc
Removed redundant suppressions from System.ComponentModel.TypeConverter
jkurdek Aug 2, 2022
e25790d
Removed redundant suppressions from System.Data.Common
jkurdek Aug 2, 2022
d6b594d
Removed redundant suppressions from Microsoft.CSharp
jkurdek Aug 2, 2022
32162d2
Removed redundant suppressions from Microsoft.VisualBasic.Core
jkurdek Aug 2, 2022
fc0e9d5
Removed redundant suppressions from System.Diagnostics.DiagnosticSource
jkurdek Aug 2, 2022
f3670c2
Apply suggestions from code review
jkurdek Aug 9, 2022
38b6f45
Suggestions from PR review
jkurdek Aug 9, 2022
5a6bb2e
removed omitted local function call
jkurdek Aug 9, 2022
e941d97
Enabled IL2121 in sfx
jkurdek Aug 12, 2022
bedf079
fix suppressions mono + X86
jkurdek Aug 18, 2022
393a6f2
Removed unnecessery suppressions around PInokes
jkurdek Aug 30, 2022
f8209e1
Fixed IL2121 warnings on Linux
jkurdek Aug 30, 2022
9cc221b
Apply suggestions from PR review
jkurdek Sep 6, 2022
d58df4f
Fix redundant suppression after dynamic code annotation
jkurdek Sep 6, 2022
9a0df91
Removed redundant suppressions NativeAot
jkurdek Sep 6, 2022
0c7165d
Reverted change in EventSource, put suppression in ifdef
jkurdek Sep 6, 2022
04dc831
Removed redundant suppressions found on mac/iOS
jkurdek Sep 7, 2022
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
Next Next commit
Removed redundant suppressions from System.Private.CoreLib
  • Loading branch information
jkurdek committed Aug 30, 2022
commit 761aefb17ce18a6b02ecb79b9e06804cec45751c
Original file line number Diff line number Diff line change
Expand Up @@ -119,8 +119,6 @@ public override Type MakeArrayType(int rank)

[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.Interfaces)]
[return: DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.Interfaces)]
[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2063:UnrecognizedReflectionPattern",
Justification = "Linker doesn't recognize always throwing method. https://github.com/mono/linker/issues/2025")]
Copy link
Member

Choose a reason for hiding this comment

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

We have references to this bug under src/coreclr/nativeaot as well - how was the redundant suppression analysis for NativeAOT done? I wonder why it didn't find those (they should be redundant because they were just a bug workaround).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The redundant warning suppression detection has not been ported to nativeaot yet (#75201). The suppression you mentioned should be identified after the functionality is ported and run witihin nativeaot.

Copy link
Member

Choose a reason for hiding this comment

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

This PR has a change in src/coreclr/nativeaot/System.Private.CoreLib/src/System/Reflection/Runtime/General/Assignability.cs - what mechanism was used to detect it?

(NativeAOT doesn't run on code that is unreferenced and doesn't have a "library mode" like illink does - so the ability to detect redundant suppressions is somewhat limited even if we were to port the feature - we would have to come up with an app that includes all the code in NativeAOT's libraries. It will only flag things that are used, making it easy for us to ship with an actual warning that only some customers will see - customers who happen to use that part of the library.)

Copy link
Member

Choose a reason for hiding this comment

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

It was detected because linker was used to "trim libraries for warnings" (The sfx project run of linker) and it was given the AOT corelib as input. Jeremi found out that this happens when you run build Clr.Aot+Libs -rc Debug -lc Release, in which case linker gets artifacts\bin\coreclr\windows.x64.Debug\aotsdk\System.Private.CoreLib.dll as one of the input assemblies.

As for the comment about NativeAOT not having libraries mode - that should not matter. The detection works in the "trim app" mode as well - it only reports redundant warnings on methods which were "marked".

Copy link
Member

Choose a reason for hiding this comment

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

It was detected because linker was used to "trim libraries for warnings" (The sfx project run of linker) and it was given the AOT corelib as input

I see, yes, there can only be one corelib project for the libs partition but we have 3 in this repo.

The corelib is selected here:

<CoreLibProject Condition="'$(RuntimeFlavor)' == 'CoreCLR'">$([MSBuild]::NormalizePath('$(CoreClrProjectRoot)', 'System.Private.CoreLib', 'System.Private.CoreLib.csproj'))</CoreLibProject>
<CoreLibProject Condition="'$(RuntimeFlavor)' == 'Mono'">$([MSBuild]::NormalizePath('$(MonoProjectRoot)', 'System.Private.CoreLib', 'System.Private.CoreLib.csproj'))</CoreLibProject>
<CoreLibProject Condition="'$(UseNativeAotCoreLib)' == 'true'">$([MSBuild]::NormalizePath('$(CoreClrProjectRoot)', 'nativeaot', 'System.Private.CoreLib', 'src', 'System.Private.CoreLib.csproj'))</CoreLibProject>

public override Type GetInterface(string name, bool ignoreCase) { throw new NotSupportedException(); }

[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.Interfaces)]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -398,8 +398,6 @@ public override FieldInfo[] GetFields(BindingFlags bindingAttr)

[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.Interfaces)]
[return: DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.Interfaces)]
[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2063:UnrecognizedReflectionPattern",
Justification = "Linker doesn't recognize always throwing method. https://github.com/mono/linker/issues/2025")]
public override Type GetInterface(string name, bool ignoreCase)
{
throw new NotSupportedException(SR.NotSupported_NonReflectedType);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -178,8 +178,6 @@ public override Type? BaseType

[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.Interfaces)]
[return: DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.Interfaces)]
[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2063:UnrecognizedReflectionPattern",
Justification = "Linker doesn't recognize always throwing method. https://github.com/mono/linker/issues/2025")]
public override Type GetInterface(string name, bool ignoreCase) { throw new NotSupportedException(); }

[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.Interfaces)]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -789,19 +789,12 @@ public static object BindToMoniker(string monikerName)
Release(bindctx);
}
}
// Revist after https://github.com/mono/linker/issues/1989 is fixed
[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2050:UnrecognizedReflectionPattern",
Justification = "The calling method is annotated with RequiresUnreferencedCode")]
[LibraryImport(Interop.Libraries.Ole32)]
private static partial int CreateBindCtx(uint reserved, out IntPtr ppbc);

[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2050:UnrecognizedReflectionPattern",
Justification = "The calling method is annotated with RequiresUnreferencedCode")]
[LibraryImport(Interop.Libraries.Ole32)]
private static partial int MkParseDisplayName(IntPtr pbc, [MarshalAs(UnmanagedType.LPWStr)] string szUserName, out uint pchEaten, out IntPtr ppmk);

[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2050:UnrecognizedReflectionPattern",
Justification = "The calling method is annotated with RequiresUnreferencedCode")]
[LibraryImport(Interop.Libraries.Ole32)]
private static partial int BindMoniker(IntPtr pmk, uint grfOpt, ref Guid iidResult, out IntPtr ppvResult);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3852,8 +3852,6 @@ private void CreateInstanceCheckThis()

[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2082:UnrecognizedReflectionPattern",
Justification = "Implementation detail of Activator that linker intrinsically recognizes")]
[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2085:UnrecognizedReflectionPattern",
Justification = "Implementation detail of Activator that linker intrinsically recognizes")]
internal object? CreateInstanceImpl(
BindingFlags bindingAttr, Binder? binder, object?[]? args, CultureInfo? culture)
{
Expand Down Expand Up @@ -3933,6 +3931,7 @@ private void CreateInstanceCheckThis()
}

// fast path??
// IL2082
instance = Activator.CreateInstance(this, nonPublic: true, wrapExceptions: wrapExceptions);
}
else
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -102,8 +102,6 @@ public static partial class Activator
Justification = "Implementation detail of Activator that linker intrinsically recognizes")]
[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2072:UnrecognizedReflectionPattern",
Justification = "Implementation detail of Activator that linker intrinsically recognizes")]
[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2096:UnrecognizedReflectionPattern",
Justification = "Implementation detail of Activator that linker intrinsically recognizes")]
private static ObjectHandle? CreateInstanceInternal(string assemblyString,
string typeName,
bool ignoreCase,
Expand All @@ -125,8 +123,10 @@ public static partial class Activator
assembly = RuntimeAssembly.InternalLoad(assemblyName, ref stackMark, AssemblyLoadContext.CurrentContextualReflectionContext);
}

// Issues IL2026 warning.
Type? type = assembly.GetType(typeName, throwOnError: true, ignoreCase);

//Issues IL2072 warning.
object? o = CreateInstance(type!, bindingAttr, binder, args, culture, activationAttributes);

return o != null ? new ObjectHandle(o) : null;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -175,8 +175,6 @@ public sealed override Type MakeArrayType(int rank)

[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.Interfaces)]
[return: DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.Interfaces)]
[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2063:UnrecognizedReflectionPattern",
Justification = "Linker doesn't recognize always throwing method. https://github.com/mono/linker/issues/2025")]
public sealed override Type GetInterface(string name, bool ignoreCase) => throw new NotSupportedException(SR.NotSupported_SignatureType);

[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors | DynamicallyAccessedMemberTypes.NonPublicConstructors)]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,14 +45,6 @@ internal ResourceReader(Stream stream, Dictionary<string, ResourceLocator> resCa
ReadResources();
}

[UnconditionalSuppressMessage("AotAnalysis", "IL3050:RequiresDynamicCode",
Justification = "InitializeBinaryFormatter will get trimmed out when AllowCustomResourceTypes is set to false. " +
"When set to true, we will already throw a warning for this feature switch, so we suppress this one in order for" +
"the user to only get one error.")]
[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2026:RequiresUnreferencedCode",
Justification = "InitializeBinaryFormatter will get trimmed out when AllowCustomResourceTypes is set to false. " +
"When set to true, we will already throw a warning for this feature switch, so we suppress this one in order for" +
"the user to only get one error.")]
private object DeserializeObject(int typeIndex)
{
if (!AllowCustomResourceTypes)
Expand All @@ -65,9 +57,22 @@ private object DeserializeObject(int typeIndex)
throw new NotSupportedException(SR.NotSupported_ResourceObjectSerialization);
}

[UnconditionalSuppressMessage("AotAnalysis", "IL3050:RequiresDynamicCode",
Justification = "InitializeBinaryFormatter will get trimmed out when AllowCustomResourceTypes is set to false. " +
"When set to true, we will already throw a warning for this feature switch, so we suppress this one in order for" +
"the user to only get one error.")]
[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2026:RequiresUnreferencedCode",
Justification = "InitializeBinaryFormatter will get trimmed out when AllowCustomResourceTypes is set to false. " +
"When set to true, we will already throw a warning for this feature switch, so we suppress this one in order for" +
"the user to only get one error.")]
bool InitializeBinaryFormatterLocal()
{
return InitializeBinaryFormatter();
}

if (Volatile.Read(ref _binaryFormatter) is null)
{
if (!InitializeBinaryFormatter())
if (!InitializeBinaryFormatterLocal())
{
// The linker trimmed away the BinaryFormatter implementation and we can't call into it.
// We'll throw an exception with the same text that BinaryFormatter would have thrown
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -939,10 +939,6 @@ private void _ReadResources()
// This allows us to delay-initialize the Type[]. This might be a
// good startup time savings, since we might have to load assemblies
// and initialize Reflection.
[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2026:RequiresUnreferencedCode",
Justification = "UseReflectionToGetType will get trimmed out when AllowCustomResourceTypes is set to false. " +
"When set to true, we will already throw a warning for this feature switch, so we suppress this one in order for" +
"the user to only get one error.")]
private Type FindType(int typeIndex)
{
if (!AllowCustomResourceTypes)
Expand All @@ -955,7 +951,16 @@ private Type FindType(int typeIndex)
throw new BadImageFormatException(SR.BadImageFormat_InvalidType);
}

return _typeTable[typeIndex] ?? UseReflectionToGetType(typeIndex);
[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2026:RequiresUnreferencedCode",
Justification = "UseReflectionToGetType will get trimmed out when AllowCustomResourceTypes is set to false. " +
"When set to true, we will already throw a warning for this feature switch, so we suppress this one in order for" +
"the user to only get one error.")]
Type UseReflectionToGetTypeLocal(int typeIndex)
{
return UseReflectionToGetType(typeIndex);
}

return _typeTable[typeIndex] ?? UseReflectionToGetTypeLocal(typeIndex);
}

[RequiresUnreferencedCode("The CustomResourceTypesSupport feature switch has been enabled for this app which is being trimmed. " +
Expand Down
11 changes: 6 additions & 5 deletions src/libraries/System.Private.CoreLib/src/System/Type.Helpers.cs
Original file line number Diff line number Diff line change
Expand Up @@ -373,17 +373,18 @@ public virtual bool IsAssignableFrom([NotNullWhen(true)] Type? c)
Justification = "The GetInterfaces technically requires all interfaces to be preserved" +
"But this method only compares the result against the passed in ifaceType." +
"So if ifaceType exists, then trimming should have kept it implemented on any type.")]
// IL2075 is produced due to the BaseType not returning annotated value and used in effectively this.BaseType.GetInterfaces()
[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2075:UnrecognizedReflectionPattern",
Justification = "The GetInterfaces technically requires all interfaces to be preserved" +
"But this method only compares the result against the passed in ifaceType." +
"So if ifaceType exists, then trimming should have kept it implemented on any type.")]
internal bool ImplementInterface(Type ifaceType)
{
Type? t = this;
while (t != null)
{
// IL2075 is produced due to the BaseType not returning annotated value and used in effectively this.BaseType.GetInterfaces()
// The GetInterfaces technically requires all interfaces to be preserved
// But this method only compares the result against the passed in ifaceType.
// So if ifaceType exists, then trimming should have kept it implemented on any type.
#pragma warning disable IL2075
Type[] interfaces = t.GetInterfaces();
#pragma warning restore IL2075
if (interfaces != null)
{
for (int i = 0; i < interfaces.Length; i++)
Expand Down