Skip to content

Fix first round of trim warnings#2451

Merged
NickCraver merged 2 commits intoStackExchange:mainfrom
eerhardt:FixTrimWarnings
May 1, 2023
Merged

Fix first round of trim warnings#2451
NickCraver merged 2 commits intoStackExchange:mainfrom
eerhardt:FixTrimWarnings

Conversation

@eerhardt
Copy link
Contributor

@eerhardt eerhardt commented Apr 27, 2023

  • DefaultOptionsProvider.TryGetAzureRoleInstanceIdNoThrow use Type.GetType to check for Microsoft.WindowsAzure.ServiceRuntime.RoleEnvironment, so the trimmer knows to preserve this type, if it is in the app
  • ChannelMessage use the added public API for ChannelReader CanCount and Count, but fallback to reflection on netcoreapp3.1, since those APIs are not available on that runtime.

Contributes to #2449

This at least removes the warnings from using Microsoft.Extensions.Caching.StackExchangeRedis.

We could also add a trimming test app as outlined in https://learn.microsoft.com/en-us/dotnet/core/deploying/trimming/prepare-libraries-for-trimming#show-all-warnings-with-sample-application, along with a unit test that publishes the app and ensures there are no new warnings. LMK if you think this is valuable.

There are still these warnings left in this library:

/_/src/StackExchange.Redis/ScriptParameterMapper.cs(173): Trim analysis warning IL2070: StackExchange.Redis.ScriptParameterMapper.IsValidParameterHash(Type,LuaScript,String&,String&): 'this' argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicConstructors', 'DynamicallyAccessedMemberTypes.PublicMethods', 'DynamicallyAccessedMemberTypes.PublicFields', 'DynamicallyAccessedMemberTypes.PublicNestedTypes', 'DynamicallyAccessedMemberTypes.PublicProperties', 'DynamicallyAccessedMemberTypes.PublicEvents' in call to 'System.Type.GetMember(String)'. The parameter 't' of method 'StackExchange.Redis.ScriptParameterMapper.IsValidParameterHash(Type,LuaScript,String&,String&)' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to. [C:\git\azure-activedirectory-identitymodel-extensions-for-dotnet\test\Microsoft.IdentityModel.AotCompatibility.TestApp\Microsoft.IdentityModel.AotCompatibility.TestApp.csproj]
/_/src/StackExchange.Redis/ScriptParameterMapper.cs(227): Trim analysis warning IL2070: StackExchange.Redis.ScriptParameterMapper.GetParameterExtractor(Type,LuaScript): 'this' argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicConstructors', 'DynamicallyAccessedMemberTypes.PublicMethods', 'DynamicallyAccessedMemberTypes.PublicFields', 'DynamicallyAccessedMemberTypes.PublicNestedTypes', 'DynamicallyAccessedMemberTypes.PublicProperties', 'DynamicallyAccessedMemberTypes.PublicEvents' in call to 'System.Type.GetMember(String)'. The parameter 't' of method 'StackExchange.Redis.ScriptParameterMapper.GetParameterExtractor(Type,LuaScript)' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to. [C:\git\azure-activedirectory-identitymodel-extensions-for-dotnet\test\Microsoft.IdentityModel.AotCompatibility.TestApp\Microsoft.IdentityModel.AotCompatibility.TestApp.csproj]
/_/src/StackExchange.Redis/ScriptParameterMapper.cs(260): Trim analysis warning IL2026: StackExchange.Redis.ScriptParameterMapper.GetParameterExtractor(Type,LuaScript): Using member 'System.Linq.Expressions.Expression.Property(Expression,String)' which has 'RequiresUnreferencedCodeAttribute' can break functionality when trimming application code. Creating Expressions requires unreferenced code because the members being referenced by the Expression may be trimmed. [C:\git\azure-activedirectory-identitymodel-extensions-for-dotnet\test\Microsoft.IdentityModel.AotCompatibility.TestApp\Microsoft.IdentityModel.AotCompatibility.TestApp.csproj]
/_/src/StackExchange.Redis/ScriptParameterMapper.cs(261): Trim analysis warning IL2026: StackExchange.Redis.ScriptParameterMapper.GetParameterExtractor(Type,LuaScript): Using member 'System.Linq.Expressions.Expression.Call(Expression,String,Type[],Expression[])' which has 'RequiresUnreferencedCodeAttribute' can break functionality when trimming application code. Creating Expressions requires unreferenced code because the members being referenced by the Expression may be trimmed. [C:\git\azure-activedirectory-identitymodel-extensions-for-dotnet\test\Microsoft.IdentityModel.AotCompatibility.TestApp\Microsoft.IdentityModel.AotCompatibility.TestApp.csproj]

Fixing those will require a bit more work, as the whole LuaScript functionality looks like it needs to be marked RequiresUnreferencedCode.

cc @mgravell @NickCraver

@eerhardt
Fix first round of trim warnings
358f011 
- DefaultOptionsProvider.TryGetAzureRoleInstanceIdNoThrow use Type.GetType to check for Microsoft.WindowsAzure.ServiceRuntime.RoleEnvironment, so the trimmer knows to preserve this type, if it is in the app
- ChannelMessage use the added public API for ChannelReader CanCount and Count, but fallback to reflection on netcoreapp3.1, since those APIs are not available on that runtime.

Contributes to StackExchange#2449
@eerhardt eerhardt mentioned this pull request Apr 27, 2023
Open
@NickCraver
Copy link
Collaborator

NickCraver commented Apr 27, 2023

Awesome! Not for this PR, but I'd be curious what a trim example looks like, and what it adds to test runtime overall. I'd like to ensure no breaks but when I last tried this the time cost was pretty high - can we do the check itself fairly quickly these days?

@eerhardt
Copy link
Contributor Author

eerhardt commented Apr 27, 2023

can we do the check itself fairly quickly these days?

There are 2 routes we can take to enable checking:

  1. Use the Roslyn Analyzers. But this requires targeting net7.0+ because the System.* APIs weren't all attributed until then (and some still aren't). We can have a "special build" that only targets net7.0 during CI, but honestly I think that is super complicated and I haven't done that or recommended it.
  2. Following https://learn.microsoft.com/en-us/dotnet/core/deploying/trimming/prepare-libraries-for-trimming#show-all-warnings-with-sample-application is the path I like because it also checks all your dependencies as well, and it doesn't require targeting a newer TFM. You can keep targeting the older ones, like netstandard2.0, and only the "test app" targets the latest TFM.
    • Following that path, looks something like this (the changes under the test folder).
    • That test takes ~30 seconds on a typical machine. It is taking about 1 minute on really slow CI machines.
    • For StackExchange.Redis, on my 2019 machine doing a clean publish like that takes 28 seconds.

Any thoughts on those options?

@eerhardt
Copy link
Contributor Author

eerhardt commented Apr 28, 2023

I'm not sure the CI failures are caused by me. But I can't really tell what failed. The Windows leg passed, so that makes me think it isn't my code.

@NickCraver NickCraver requested a review from philon-msft May 1, 2023 12:02
@NickCraver NickCraver merged commit 3ba8d2a into StackExchange:main May 1, 2023
@eerhardt eerhardt deleted the FixTrimWarnings branch May 1, 2023 22:23
@eerhardt eerhardt mentioned this pull request May 2, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@philon-msft philon-msft philon-msft approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@eerhardt @NickCraver @philon-msft