EnableAOTAnalyzer for most of Microsoft.Extensions libraries #73737
Conversation
The only libraries that aren't enabled yet are DependencyInjection and Hosting. These will come in a separate PR. Contributes to dotnet#71654
|
Note regarding the This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change. |
|
Tagging subscribers to this area: @dotnet/area-meta Issue DetailsThe only libraries that aren't enabled yet are DependencyInjection and Hosting. These will come in a separate PR. Contributes to #71654
|
|
Tagging subscribers to this area: @dotnet/area-extensions-primitives Issue DetailsThe only libraries that aren't enabled yet are DependencyInjection and Hosting. These will come in a separate PR. Contributes to #71654
|
| Assert.True(prop.PropertyType == typeof(string) || | ||
| prop.PropertyType == typeof(bool) || | ||
| prop.PropertyType == typeof(int) || |
There was a problem hiding this comment.
Are these the arguments used for MakeGeneric?
Instantiations over valuetypes need specialized code, so instantiating something over bool/int could fail (e.g. typeof(List<>).MakeGenericType(typeof(bool)) will throw unless we already have the code for List<bool> guaranteed from elsewhere (what is guaranteeing it?).
There was a problem hiding this comment.
Are these the arguments used for MakeGeneric?
No.
The way ConfigurationBinder works is that loops over the Type's properties and tries to "bind" the properties to an IConfiguration (i.e. Dictionary<string, string>).
The places that are AOT-unfriendly are handling the more complicated property types - like a IList<T> or Dictionary<T, K>. Those places need to use MakeGenericType. So if you had an object like:
class OptionsWithLists
{
public IList<int> MyInts { get; set; }
}The ConfigurationBinder will see the MyInts property and try to create an int[] instance to fill in that property with all the values in the IConfiguration.
But here all the properties are simple, which is what this test is verifying. (Except the Formatter property on JsonWriterOptions, which isn't possible to bind using an IConfiguration.) Since they are all simple (string, bool, int), ConfigurationBinder just tries parsing the string to the destination type, which will work correctly.
When/if we have AOT tests like we do for TrimmingTests, this would be a good candidate to try to configure all the properties of these types and ensure it still works correctly.
Note - after looking into the existing IL2026 suppression in the code, I found out that we aren't actually preserving the properties correctly for trimming. I opened #73822 for that issue.
| internal const string TrimmingRequiresUnreferencedCodeMessage = "TOptions's dependent types may have their members trimmed. Ensure all required members are preserved."; | ||
|
|
||
| /// <summary> | ||
| /// Adds a console logger named 'Console' to the factory. | ||
| /// </summary> | ||
| /// <param name="builder">The <see cref="ILoggingBuilder"/> to use.</param> | ||
| [UnconditionalSuppressMessage("AotAnalysis", "IL3050:RequiresDynamicCode", | ||
| Justification = "AddConsoleFormatter and RegisterProviderOptions are only called with Options types which only have simple properties.")] |
There was a problem hiding this comment.
Similar comments as @MichalStrehovsky around reference type being safe with RDC problematic APIs like MakeGenericXXX and not with Value Types. Not sure if these option types are preserved elsewhere to warrant suppress
There was a problem hiding this comment.
See the comment above. The options' properties are being preserved because we annotated the AddConsoleFormatter and RegisterProviderOptions methods with DynamicallyAccessedMemberTypes.All, which preserves the top level properties. However, there are nested properties which aren't being preserved - which I logged #73822 to address.
The only libraries that aren't enabled yet are DependencyInjection and Hosting. These will come in a separate PR.
Contributes to #71654