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

Partially annotate IL2026 #1958

Closed
wants to merge 1 commit into from
Closed

Partially annotate IL2026 #1958

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
15 changes: 15 additions & 0 deletions ...crosoft.Data.SqlClient/netcore/src/Common/src/System/Net/Logging/NetEventSource.Common.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

using System.Collections;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.Diagnostics.Tracing;
using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;
Expand Down Expand Up @@ -50,6 +51,8 @@ namespace System.Net
#endif
internal sealed partial class NetEventSource : EventSource
{
private const string EventSourceSuppressMessage = "Parameters to this method are primitive and are trimmer safe";

/// <summary>The single event source instance to use for all logging.</summary>
public static readonly NetEventSource Log = new NetEventSource();

Expand Down Expand Up @@ -518,6 +521,8 @@ private static string Format(FormattableString s)
#region Custom WriteEvent overloads

[NonEvent]
[SuppressMessage("ReflectionAnalysis", "IL2026:UnrecognizedReflectionPattern",
Copy link
Contributor

Choose a reason for hiding this comment

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

Should it be UnconditionalSuppressMessage? It's available on .NET 5+ so it needs an #if guard.

Copy link
Member

Choose a reason for hiding this comment

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

Yes - it MUST be UnconditionalSuppressMessage. SuppressMessage is "conditional" and the compiler will not emit it to IL - it disappears after compilation. As a result the trimmer doesn't get to see it.

We've been discussing some possibilities to provide an analyzer for these "bugs" but it's difficult (SuppressMessage is handled by the compiler intrinsically...). I could not find a tracking issue, so I created dotnet/runtime#93453.

Justification = EventSourceSuppressMessage)]
private unsafe void WriteEvent(int eventId, string arg1, string arg2, string arg3, string arg4)
{
if (IsEnabled())
Expand Down Expand Up @@ -566,6 +571,8 @@ private unsafe void WriteEvent(int eventId, string arg1, string arg2, string arg
}

[NonEvent]
[SuppressMessage("ReflectionAnalysis", "IL2026:UnrecognizedReflectionPattern",
Justification = EventSourceSuppressMessage)]
private unsafe void WriteEvent(int eventId, string arg1, string arg2, byte[] arg3)
{
if (IsEnabled())
Expand Down Expand Up @@ -612,6 +619,8 @@ private unsafe void WriteEvent(int eventId, string arg1, string arg2, byte[] arg
}

[NonEvent]
[SuppressMessage("ReflectionAnalysis", "IL2026:UnrecognizedReflectionPattern",
Justification = EventSourceSuppressMessage)]
private unsafe void WriteEvent(int eventId, string arg1, int arg2, int arg3, int arg4)
{
if (IsEnabled())
Expand Down Expand Up @@ -651,6 +660,8 @@ private unsafe void WriteEvent(int eventId, string arg1, int arg2, int arg3, int
}

[NonEvent]
[SuppressMessage("ReflectionAnalysis", "IL2026:UnrecognizedReflectionPattern",
Justification = EventSourceSuppressMessage)]
private unsafe void WriteEvent(int eventId, string arg1, int arg2, string arg3)
{
if (IsEnabled())
Expand Down Expand Up @@ -688,6 +699,8 @@ private unsafe void WriteEvent(int eventId, string arg1, int arg2, string arg3)
}

[NonEvent]
[SuppressMessage("ReflectionAnalysis", "IL2026:UnrecognizedReflectionPattern",
Justification = EventSourceSuppressMessage)]
private unsafe void WriteEvent(int eventId, string arg1, string arg2, int arg3)
{
if (IsEnabled())
Expand Down Expand Up @@ -725,6 +738,8 @@ private unsafe void WriteEvent(int eventId, string arg1, string arg2, int arg3)
}

[NonEvent]
[SuppressMessage("ReflectionAnalysis", "IL2026:UnrecognizedReflectionPattern",
Justification = EventSourceSuppressMessage)]
private unsafe void WriteEvent(int eventId, string arg1, string arg2, string arg3, int arg4)
{
if (IsEnabled())
Expand Down
5 changes: 5 additions & 0 deletions ...a.SqlClient/netcore/src/Common/src/System/Net/Security/NetEventSource.Security.Windows.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System.Diagnostics.CodeAnalysis;
using System.Diagnostics.Tracing;
using System.Net.Security;

Expand All @@ -10,6 +11,8 @@ namespace System.Net
internal sealed partial class NetEventSource
{
[Event(AcquireDefaultCredentialId, Keywords = Keywords.Default, Level = EventLevel.Informational)]
[SuppressMessage("ReflectionAnalysis", "IL2026:UnrecognizedReflectionPattern",
Justification = EventSourceSuppressMessage)]
public void AcquireDefaultCredential(string packageName, Interop.SspiCli.CredentialUse intent)
{
if (IsEnabled())
Expand Down Expand Up @@ -61,6 +64,8 @@ private void AcceptSecurityContext(string credential, string context, Interop.Ss
WriteEvent(AcceptSecurityContextId, credential, context, (int)inFlags);

[Event(OperationReturnedSomethingId, Keywords = Keywords.Default, Level = EventLevel.Informational)]
[SuppressMessage("ReflectionAnalysis", "IL2026:UnrecognizedReflectionPattern",
Justification = EventSourceSuppressMessage)]
public void OperationReturnedSomething(string operation, Interop.SECURITY_STATUS errorCode)
{
if (IsEnabled())
Expand Down
16 changes: 15 additions & 1 deletion ...e/src/Microsoft/Data/SqlClient/Reliability/SqlConfigurableRetryLogicManager.NetCoreApp.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

using System;
using System.Collections.Generic;
using System.Diagnostics.CodeAnalysis;
using System.IO;
using System.Reflection;
using System.Runtime.Loader;
Expand All @@ -21,6 +22,9 @@ internal sealed partial class SqlConfigurableRetryLogicLoader
/// and its related assemblies in default assembly load context if they aren't loaded yet.
/// </summary>
/// <returns>Resolved type if it could resolve the type; otherwise, the `SqlConfigurableRetryFactory` type.</returns>
#if NET6_0_OR_GREATER
[RequiresUnreferencedCode("The type might be removed")]
#endif
private static Type LoadType(string fullyQualifiedName)
{
string methodName = nameof(LoadType);
Expand Down Expand Up @@ -58,6 +62,9 @@ private static string MakeFullPath(string directory, string assemblyName, string
return File.Exists(fullPath) ? fullPath : null;
}

#if NET6_0_OR_GREATER
[RequiresUnreferencedCode("Calls System.Runtime.Loader.AssemblyLoadContext.LoadFromAssemblyPath(String)")]
#endif
private static Assembly AssemblyResolver(AssemblyName arg)
{
string methodName = nameof(AssemblyResolver);
Expand All @@ -69,6 +76,10 @@ private static Assembly AssemblyResolver(AssemblyName arg)
return fullPath == null ? null : AssemblyLoadContext.Default.LoadFromAssemblyPath(fullPath);
}


#if NET6_0_OR_GREATER
[RequiresUnreferencedCode("Calls System.Reflection.Assembly.ExportedTypes")]
#endif
private static Type TypeResolver(Assembly arg1, string arg2, bool arg3)
{
IEnumerable<Type> types = arg1?.ExportedTypes;
Expand Down Expand Up @@ -97,6 +108,9 @@ private static Type TypeResolver(Assembly arg1, string arg2, bool arg3)
/// <summary>
/// Load assemblies on request.
/// </summary>
#if NET6_0_OR_GREATER
[RequiresUnreferencedCode("Calls System.Runtime.Loader.AssemblyLoadContext.LoadFromAssemblyPath(String)")]
#endif
private static Assembly Default_Resolving(AssemblyLoadContext arg1, AssemblyName arg2)
{
string methodName = nameof(Default_Resolving);
Expand All @@ -107,6 +121,6 @@ private static Assembly Default_Resolving(AssemblyLoadContext arg1, AssemblyName

return target == null ? null : arg1.LoadFromAssemblyPath(target);
}
#endregion
#endregion
}
}
33 changes: 33 additions & 0 deletions ...a.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlClientDiagnosticListenerExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using System;
using System.Data;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.Runtime.CompilerServices;

namespace Microsoft.Data.SqlClient
Expand All @@ -14,6 +15,8 @@ namespace Microsoft.Data.SqlClient
/// </summary>
internal static class SqlClientDiagnosticListenerExtensions
{
private const string EventSourceSuppressMessage = "Parameters to this method are statically known and thus are trimmer safe";
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain why suppression is required while the parameters are genuinely trimmer-safe? I expect the analyzer to distinguish it. VS recognizes them as an unnecessary suppression that could be removed though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IL trimmer made design decision operate on IL level, when it see virtual call he don't perform full application analysys to prove that this is actuall specific call. So each virtual call should have same annotations as base class.

I know, probably that can be relaxed, but it is how it is. @vitek-karas maybe give more perspective why this is needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ooh, here. I should not be that fast in answering. EventSource.Write annotated as RequireUnreferencedCode, and trimmer think that if somebody use such methods it's dangerous. It is expected suppress warning like that

Copy link
Member

Choose a reason for hiding this comment

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

This is actually not correct and should not be suppressed as-is. This code for example means that OpenTelemetry's SqlClient instrumentation is not trim/AOT compatible. https://github.com/open-telemetry/opentelemetry-dotnet/blob/b091af1899b66a516d753109ce6bf8182be416e0/src/OpenTelemetry.Instrumentation.SqlClient/SqlClientInstrumentation.cs#L56C9-L61

Sorry it fell through cracks on my side, I meant to file an issue around this long time ago... so I finally did it: #2184. Other libraries are in similar (although not as bad) problem, so we're slowly trying to get this fixed. OTel in its latest version is now fully AOT compatible, with the exception of the SqlClient instrumentation as there's no good way to make that work correctly right now.

As described in the issue - the best solution is to declare all of these payload types as public. That way the consumer (OTel for example) can just cast they payload instance to that type and directly access its properties. That would also fix all of the trimming issues related to in-proc consumers which are using the public types.

Separate issue is all the consumers which are either using reflection or rely on diagnostic event source to do the same. For those the only solution is to explicitely hint the trimmer to preserve certain properties. For example System.Http, ASP.NET and GRPC all do this to some degree. For example HTTP stack:
https://github.com/dotnet/runtime/blob/9c4d51c0a45a129745b2eec9fb5497042cd805fb/src/libraries/System.Net.Http/src/System/Net/Http/DiagnosticsHandler.cs#L325 - this preserves all top-level properties on the payload object
https://github.com/dotnet/runtime/blob/9c4d51c0a45a129745b2eec9fb5497042cd805fb/src/libraries/System.Net.Http/src/System/Net/Http/DiagnosticsHandler.cs#L272-L276 - this preserves some nested properties on one of the payload objects.

This is not a precise solution and never will be, but it's worth preserving at least properties used by OTel and other popular consumers (personally I don't know what they would be though).


public const string DiagnosticListenerName = "SqlClientDiagnosticListener";

private const string SqlClientPrefix = "Microsoft.Data.SqlClient.";
Expand All @@ -38,6 +41,8 @@ internal static class SqlClientDiagnosticListenerExtensions
public const string SqlAfterRollbackTransaction = SqlClientPrefix + nameof(WriteTransactionRollbackAfter);
public const string SqlErrorRollbackTransaction = SqlClientPrefix + nameof(WriteTransactionRollbackError);

[SuppressMessage("ReflectionAnalysis", "IL2026:UnrecognizedReflectionPattern",
Justification = EventSourceSuppressMessage)]
public static Guid WriteCommandBefore(this SqlDiagnosticListener @this, SqlCommand sqlCommand, SqlTransaction transaction, [CallerMemberName] string operation = "")
{
if (@this.IsEnabled(SqlBeforeExecuteCommand))
Expand All @@ -62,6 +67,8 @@ public static Guid WriteCommandBefore(this SqlDiagnosticListener @this, SqlComma
return Guid.Empty;
}

[SuppressMessage("ReflectionAnalysis", "IL2026:UnrecognizedReflectionPattern",
Justification = EventSourceSuppressMessage)]
public static void WriteCommandAfter(this SqlDiagnosticListener @this, Guid operationId, SqlCommand sqlCommand, SqlTransaction transaction, [CallerMemberName] string operation = "")
{
if (@this.IsEnabled(SqlAfterExecuteCommand))
Expand All @@ -81,6 +88,8 @@ public static void WriteCommandAfter(this SqlDiagnosticListener @this, Guid oper
}
}

[SuppressMessage("ReflectionAnalysis", "IL2026:UnrecognizedReflectionPattern",
Justification = EventSourceSuppressMessage)]
public static void WriteCommandError(this SqlDiagnosticListener @this, Guid operationId, SqlCommand sqlCommand, SqlTransaction transaction, Exception ex, [CallerMemberName] string operation = "")
{
if (@this.IsEnabled(SqlErrorExecuteCommand))
Expand All @@ -100,6 +109,8 @@ public static void WriteCommandError(this SqlDiagnosticListener @this, Guid oper
}
}

[SuppressMessage("ReflectionAnalysis", "IL2026:UnrecognizedReflectionPattern",
Justification = EventSourceSuppressMessage)]
public static Guid WriteConnectionOpenBefore(this SqlDiagnosticListener @this, SqlConnection sqlConnection, [CallerMemberName] string operation = "")
{
if (@this.IsEnabled(SqlBeforeOpenConnection))
Expand All @@ -123,6 +134,8 @@ public static Guid WriteConnectionOpenBefore(this SqlDiagnosticListener @this, S
return Guid.Empty;
}

[SuppressMessage("ReflectionAnalysis", "IL2026:UnrecognizedReflectionPattern",
Justification = EventSourceSuppressMessage)]
public static void WriteConnectionOpenAfter(this SqlDiagnosticListener @this, Guid operationId, SqlConnection sqlConnection, [CallerMemberName] string operation = "")
{
if (@this.IsEnabled(SqlAfterOpenConnection))
Expand All @@ -142,6 +155,8 @@ public static void WriteConnectionOpenAfter(this SqlDiagnosticListener @this, Gu
}
}

[SuppressMessage("ReflectionAnalysis", "IL2026:UnrecognizedReflectionPattern",
Justification = EventSourceSuppressMessage)]
public static void WriteConnectionOpenError(this SqlDiagnosticListener @this, Guid operationId, SqlConnection sqlConnection, Exception ex, [CallerMemberName] string operation = "")
{
if (@this.IsEnabled(SqlErrorOpenConnection))
Expand All @@ -161,6 +176,8 @@ public static void WriteConnectionOpenError(this SqlDiagnosticListener @this, Gu
}
}

[SuppressMessage("ReflectionAnalysis", "IL2026:UnrecognizedReflectionPattern",
Justification = EventSourceSuppressMessage)]
public static Guid WriteConnectionCloseBefore(this SqlDiagnosticListener @this, SqlConnection sqlConnection, [CallerMemberName] string operation = "")
{
if (@this.IsEnabled(SqlBeforeCloseConnection))
Expand All @@ -185,6 +202,8 @@ public static Guid WriteConnectionCloseBefore(this SqlDiagnosticListener @this,
return Guid.Empty;
}

[SuppressMessage("ReflectionAnalysis", "IL2026:UnrecognizedReflectionPattern",
Justification = EventSourceSuppressMessage)]
public static void WriteConnectionCloseAfter(this SqlDiagnosticListener @this, Guid operationId, Guid clientConnectionId, SqlConnection sqlConnection, [CallerMemberName] string operation = "")
{
if (@this.IsEnabled(SqlAfterCloseConnection))
Expand All @@ -203,6 +222,8 @@ public static void WriteConnectionCloseAfter(this SqlDiagnosticListener @this, G
}
}

[SuppressMessage("ReflectionAnalysis", "IL2026:UnrecognizedReflectionPattern",
Justification = EventSourceSuppressMessage)]
public static void WriteConnectionCloseError(this SqlDiagnosticListener @this, Guid operationId, Guid clientConnectionId, SqlConnection sqlConnection, Exception ex, [CallerMemberName] string operation = "")
{
if (@this.IsEnabled(SqlErrorCloseConnection))
Expand All @@ -222,6 +243,8 @@ public static void WriteConnectionCloseError(this SqlDiagnosticListener @this, G
}
}

[SuppressMessage("ReflectionAnalysis", "IL2026:UnrecognizedReflectionPattern",
Justification = EventSourceSuppressMessage)]
public static Guid WriteTransactionCommitBefore(this SqlDiagnosticListener @this, IsolationLevel isolationLevel, SqlConnection connection, SqlInternalTransaction transaction, [CallerMemberName] string operation = "")
{
if (@this.IsEnabled(SqlBeforeCommitTransaction))
Expand All @@ -246,6 +269,8 @@ public static Guid WriteTransactionCommitBefore(this SqlDiagnosticListener @this
return Guid.Empty;
}

[SuppressMessage("ReflectionAnalysis", "IL2026:UnrecognizedReflectionPattern",
Justification = EventSourceSuppressMessage)]
public static void WriteTransactionCommitAfter(this SqlDiagnosticListener @this, Guid operationId, IsolationLevel isolationLevel, SqlConnection connection, SqlInternalTransaction transaction, [CallerMemberName] string operation = "")
{
if (@this.IsEnabled(SqlAfterCommitTransaction))
Expand All @@ -264,6 +289,8 @@ public static void WriteTransactionCommitAfter(this SqlDiagnosticListener @this,
}
}

[SuppressMessage("ReflectionAnalysis", "IL2026:UnrecognizedReflectionPattern",
Justification = EventSourceSuppressMessage)]
public static void WriteTransactionCommitError(this SqlDiagnosticListener @this, Guid operationId, IsolationLevel isolationLevel, SqlConnection connection, SqlInternalTransaction transaction, Exception ex, [CallerMemberName] string operation = "")
{
if (@this.IsEnabled(SqlErrorCommitTransaction))
Expand All @@ -283,6 +310,8 @@ public static void WriteTransactionCommitError(this SqlDiagnosticListener @this,
}
}

[SuppressMessage("ReflectionAnalysis", "IL2026:UnrecognizedReflectionPattern",
Justification = EventSourceSuppressMessage)]
public static Guid WriteTransactionRollbackBefore(this SqlDiagnosticListener @this, IsolationLevel isolationLevel, SqlConnection connection, SqlInternalTransaction transaction, string transactionName = null, [CallerMemberName] string operation = "")
{
if (@this.IsEnabled(SqlBeforeRollbackTransaction))
Expand All @@ -308,6 +337,8 @@ public static Guid WriteTransactionRollbackBefore(this SqlDiagnosticListener @th
return Guid.Empty;
}

[SuppressMessage("ReflectionAnalysis", "IL2026:UnrecognizedReflectionPattern",
Justification = EventSourceSuppressMessage)]
public static void WriteTransactionRollbackAfter(this SqlDiagnosticListener @this, Guid operationId, IsolationLevel isolationLevel, SqlConnection connection, SqlInternalTransaction transaction, string transactionName = null, [CallerMemberName] string operation = "")
{
if (@this.IsEnabled(SqlAfterRollbackTransaction))
Expand All @@ -327,6 +358,8 @@ public static void WriteTransactionRollbackAfter(this SqlDiagnosticListener @thi
}
}

[SuppressMessage("ReflectionAnalysis", "IL2026:UnrecognizedReflectionPattern",
Justification = EventSourceSuppressMessage)]
public static void WriteTransactionRollbackError(this SqlDiagnosticListener @this, Guid operationId, IsolationLevel isolationLevel, SqlConnection connection, SqlInternalTransaction transaction, Exception ex, string transactionName = null, [CallerMemberName] string operation = "")
{
if (@this.IsEnabled(SqlErrorRollbackTransaction))
Expand Down
4 changes: 4 additions & 0 deletions src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlCommand.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
using System.Data.Common;
using System.Data.SqlTypes;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.IO;
using System.Runtime.CompilerServices;
using System.Text;
Expand Down Expand Up @@ -543,6 +544,9 @@ private SqlInternalConnectionTds InternalTdsConnection
[DesignerSerializationVisibility(DesignerSerializationVisibility.Hidden)]
public SqlRetryLogicBaseProvider RetryLogicProvider
{
#if NET6_0_OR_GREATER
[RequiresUnreferencedCode("RetryLogicProvider can be read from app.config which is unsafe for trimming")]
#endif
get
{
if (_retryLogicProvider == null)
Expand Down
3 changes: 3 additions & 0 deletions src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlConnection.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,9 @@ private static readonly ConcurrentDictionary<string, IList<string>> _ColumnEncry
[DesignerSerializationVisibility(DesignerSerializationVisibility.Hidden)]
public SqlRetryLogicBaseProvider RetryLogicProvider
{
#if NET6_0_OR_GREATER
[RequiresUnreferencedCode("RetryLogicProvider can be read from app.config which is unsafe for trimming")]
Copy link
Member

Choose a reason for hiding this comment

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

Typical pattern is to define a const string field with the message and the use that. It makes it easy to improve the message and make it consistent across multiple usages. It also makes it clearer if the RequiresUnreferencedCode is "propagated" across multiple methods in a call chain - that they all belong together and are for the same reason.

#endif
get
{
if (_retryLogicProvider == null)
Expand Down
8 changes: 8 additions & 0 deletions src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlConnectionFactory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using System;
using System.Data.Common;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.IO;
using Microsoft.Data.Common;
using Microsoft.Data.ProviderBase;
Expand Down Expand Up @@ -296,6 +297,13 @@ override internal void SetInnerConnectionTo(DbConnection owningObject, DbConnect
}
}

#if NET6_0_OR_GREATER
[DynamicDependency(DynamicallyAccessedMemberTypes.All, typeof(GroupByBehavior))]
[DynamicDependency(DynamicallyAccessedMemberTypes.All, typeof(IdentifierCase))]
[DynamicDependency(DynamicallyAccessedMemberTypes.All, typeof(SupportedJoinOperators))]
Comment on lines +301 to +303
Copy link
Member

Choose a reason for hiding this comment

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

Is it really required to have All? All is weird in that it applies recursively to all base types and interfaces, so it can bring in a LOT of dependencies. If we're just reading these from XML, maybe some like "public/private properties, constructors and fields" may be enough?

[UnconditionalSuppressMessage("Trimming", "IL2026:Members annotated with 'RequiresUnreferencedCodeAttribute' require dynamic access otherwise can break functionality when trimming application code",
Justification = "All non-primitive types which are contained in the XML stream marked as dynamic dependencies.")]
Comment on lines +301 to +305
Copy link
Member

Choose a reason for hiding this comment

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

In cases like these we would generally recommend adding a unit test which validates the assumption here. So in this case a test which would deserialize the embedded XML and then walk the returned object graph (using reflection) and check that it only encountered expected set of types. You could go even the final step and read the expected set of types from the DynamicDependencyAttribute on this method.

The main goal of such a test is to make the code maintainable. If in the future somebody changes the XML and adds a new type into it, the assumption here would break, but there would be no way to tell (nothing would produce a warning). So the unit test acts as the guard of that.

We've done things like this in other repos as well - and it seems to work pretty well.

#endif
protected override DbMetaDataFactory CreateMetaDataFactory(DbConnectionInternal internalConnection, out bool cacheMetaDataFactory)
{
Debug.Assert(internalConnection != null, "internalConnection may not be null.");
Expand Down
7 changes: 7 additions & 0 deletions src/Microsoft.Data.SqlClient/src/Microsoft/Data/ProviderBase/DbMetaDataFactory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
using System.Data;
using System.Data.Common;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.Globalization;
using System.IO;

Expand Down Expand Up @@ -36,6 +37,9 @@ internal class DbMetaDataFactory
private const string SqlCommandKey = "SQLCommand";
private const string PrepareCollectionKey = "PrepareCollection";

#if NET6_0_OR_GREATER
Copy link
Contributor

Choose a reason for hiding this comment

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

"Metadata loaded from XML stream may require types that have been trimmed away" ?

[RequiresUnreferencedCode("Metadata loaded from XML stream may require types which was trimmed out")]
#endif
public DbMetaDataFactory(Stream xmlStream, string serverVersion, string normalizedServerVersion)
{
ADP.CheckArgumentNull(xmlStream, nameof(xmlStream));
Expand Down Expand Up @@ -495,6 +499,9 @@ private bool IncludeThisColumn(DataColumn sourceColumn, string[] hiddenColumnNam
return result;
}

#if NET6_0_OR_GREATER
[RequiresUnreferencedCode("LoadDataSetFromXml uses System.Data.DataSet.ReadXml(Stream)")]
Copy link
Member

Choose a reason for hiding this comment

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

In general would avoid using messages like this - the message should be telling the caller of this method what is the problem. Describing that the problem is that this method is going to call another method is not very helpful to the caller.

I know that these are private methods, so it doesn't matter much, but that's only true now. 6 months from now if somebody adds a call to this method they will get a warning with this message. And it would effectively require them to go search through the code and across multiple methods to determine the real problem. It's also a good habit, to avoid doing this on a public API.

#endif
private void LoadDataSetFromXml(Stream XmlStream)
{
_metaDataCollectionsDataSet = new DataSet
Expand Down
Loading