Partially annotate IL2026 #1958
Conversation
...core/src/Microsoft/Data/SqlClient/Reliability/SqlConfigurableRetryLogicManager.NetCoreApp.cs
Show resolved
Hide resolved
| @@ -36,6 +37,9 @@ internal class DbMetaDataFactory | |||
| private const string SqlCommandKey = "SQLCommand"; | |||
| private const string PrepareCollectionKey = "PrepareCollection"; | |||
|
|
|||
| #if NET6_0_OR_GREATER | |||
There was a problem hiding this comment.
"Metadata loaded from XML stream may require types that have been trimmed away" ?
src/Microsoft.Data.SqlClient/src/Microsoft/Data/ProviderBase/DbMetaDataFactory.cs
Show resolved
Hide resolved
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #1958 +/- ##
==========================================
- Coverage 70.67% 63.75% -6.92%
==========================================
Files 306 306
Lines 61995 61555 -440
==========================================
- Hits 43812 39247 -4565
- Misses 18183 22308 +4125
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
kant2002
force-pushed
the
kant/remove-il2026
branch
from
d7605d6 to
368a1f1
Compare
I cannot get rid all of them since rety logic used everywhere in the SqlConnection and SqlCommad and I think there maybe other better ways. Other places is more or less self-contained and did not pollute anything.
kant2002
force-pushed
the
kant/remove-il2026
branch
from
368a1f1 to
272219b
Compare
| // Fetch the section attributes values from the configuration section of the app config file. | ||
| cnnConfig = AppConfigManager.FetchConfigurationSection<SqlConfigurableRetryConnectionSection>(SqlConfigurableRetryConnectionSection.Name); | ||
| cmdConfig = AppConfigManager.FetchConfigurationSection<SqlConfigurableRetryCommandSection>(SqlConfigurableRetryCommandSection.Name); |
There was a problem hiding this comment.
@Wraith2 starting from this two lines we apply RequiresUnreferencedCode across codebase. this bubble up to override of DbDataReader like ExecuteScalar/ExecuteDbDataReader, etc. In .NET DbDataReader does not marked with RUC so it is prohibited to mark these methods with RUC in derived classes.
I'm not sure that App.config code path is actually excercised on .NET variant, but if so, I really would like to have different configuration for that. if no configuration in app.config then only one instance of class is available here, so I really would like to streamline instantiation of that class without using all these "reflection" magic somehow.
There was a problem hiding this comment.
Assuming it's uncommon to use the config file to provide a custom class, this whole thing might be a candidate for a feature switch.
Basically disable this capability by default for trimmed apps. If the user enables it explicitly via a property in the csproj file, they would get a warning and would have to deal with it on their own.
But this requires somebody with the knowledge of how this is used - the feature switches are a reasonable solution only for rarely used capabilities. They also degrade the overall experience - instead of providing compile time warnings about trim incompatibilities, the failure will happen only at runtime, so they should be used only when other solutions are not viable.
| @@ -518,6 +521,8 @@ private static string Format(FormattableString s) | |||
| #region Custom WriteEvent overloads | |||
|
|
|||
| [NonEvent] | |||
| [SuppressMessage("ReflectionAnalysis", "IL2026:UnrecognizedReflectionPattern", | |||
There was a problem hiding this comment.
Should it be UnconditionalSuppressMessage? It's available on .NET 5+ so it needs an #if guard.
There was a problem hiding this comment.
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.
| @@ -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"; | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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).
| [DynamicDependency(DynamicallyAccessedMemberTypes.All, typeof(GroupByBehavior))] | ||
| [DynamicDependency(DynamicallyAccessedMemberTypes.All, typeof(IdentifierCase))] | ||
| [DynamicDependency(DynamicallyAccessedMemberTypes.All, typeof(SupportedJoinOperators))] | ||
| [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.")] |
There was a problem hiding this comment.
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.
| [DynamicDependency(DynamicallyAccessedMemberTypes.All, typeof(GroupByBehavior))] | ||
| [DynamicDependency(DynamicallyAccessedMemberTypes.All, typeof(IdentifierCase))] | ||
| [DynamicDependency(DynamicallyAccessedMemberTypes.All, typeof(SupportedJoinOperators))] |
There was a problem hiding this comment.
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?
| @@ -121,6 +121,9 @@ private enum CultureCheckState : uint | |||
| [DesignerSerializationVisibility(DesignerSerializationVisibility.Hidden)] | |||
| public SqlRetryLogicBaseProvider RetryLogicProvider | |||
| { | |||
| #if NET6_0_OR_GREATER | |||
| [RequiresUnreferencedCode("RetryLogicProvider can be read from app.config which is unsafe for trimming")] | |||
There was a problem hiding this comment.
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.
| @@ -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)")] | |||
There was a problem hiding this comment.
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.
| #if NET6_0_OR_GREATER | ||
| [UnconditionalSuppressMessage("Trimming", "IL2026:Members annotated with 'RequiresUnreferencedCodeAttribute' require dynamic access otherwise can break functionality when trimming application code", | ||
| Justification = "Filter expression uses only primitive types and trimmer safe")] | ||
| #endif |
There was a problem hiding this comment.
I would recommend avoiding suppressions on large methods. These are problematic because:
- The suppression applies to the entire method body. It is typically intended to suppress one or two specific calls, but it covers everything. Later on if somebody adds a new code into the method, and introduces a new trim incompatibility, the suppression will still silence that warning as well - and the justification may be wrong at that time -> introducing a trim incompatibility problem.
- It's hard to read - for example I still can't tell exactly where the suppression applies and which primitive types it's talking about.
I would suggest to refactor the code such that the problematic call is isolated into a local function and the suppression is added onto the local function.
I know it makes the code a little less clean, but it's the price we have to pay for the fact that there's no way to add an attribute (or really any kind of metadata) onto a block of IL instructions.
| [UnconditionalSuppressMessage("Trimming", "IL2026:Members annotated with 'RequiresUnreferencedCodeAttribute' require dynamic access otherwise can break functionality when trimming application code", | ||
| Justification = "Usages of this property annotated with RequiresUnreferencedCode")] |
There was a problem hiding this comment.
This feels wrong - if the usages are all annotated, then why not simply mark this with RequiresUnreferencedCode?
Also of note - this suppression will not work in the trimmer. The initialization code is actually part of the static constructor for the class in IL. This is a long standing problem we haven't been able to fix yet.
The simplest solution is probably to add an explicit static constructor, initialize the values there and suppress the warnings there as well.
| [SuppressMessage("ReflectionAnalysis", "IL2026:UnrecognizedReflectionPattern", | ||
| Justification = "DataContractJsonSerializer used here deserialize only byte[] which is already kept.")] |
There was a problem hiding this comment.
Use the local function refactoring to apply the suppression only onto the specific piece of code (in this case the two lines inside the using statement)
|
@kant2002 Can you address vitek-karas' comments? I also recommend pulling/merging from main, too, as there is a fix that is needed to avoid validation pipeline failures now. |
|
Closing stale PRs, please open a new PR when ready with feedback implemented. |
I cannot get rid all of them since rety logic used everywhere in the SqlConnection and SqlCommad and I think there maybe other better ways. Other places is more or less self-contained and did not pollute anything.
/cc @Wraith2