Resolving most of ILLink warnings on Microsoft.Extensions.Hosting #53646
Conversation
joperezr
added
linkable-framework
|
Tagging subscribers to this area: @eerhardt, @maryamariyan Issue DetailsResolving most of the warnings on Microsoft.Extensions.Hosting. The last two warnings are not so easily suppressible today, so the plan for now will instead be to wait for @vitek-karas's change dotnet/linker#2075 on the linker side which will make these type of warnings easier to suppress.
|
|
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. |
src/libraries/Microsoft.Extensions.Hosting/src/HostingHostBuilderExtensions.cs
Outdated
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.Hosting/src/HostingHostBuilderExtensions.cs
Outdated
Show resolved
Hide resolved
| { | ||
| bool isDevelopment = context.HostingEnvironment.IsDevelopment(); | ||
| options.ValidateScopes = isDevelopment; | ||
| options.ValidateOnBuild = isDevelopment; | ||
| }); | ||
|
|
||
| return builder; | ||
|
|
||
| [UnconditionalSuppressMessage("ReflectionAnalysis", "IL2026:RequiresUnreferencedCode", | ||
| Justification = "Calling IConfiguration.GetValue is safe when the T is bool.")] |
There was a problem hiding this comment.
I wonder if it would be valuable to add some new overloads to this method for common things, like bool, int, and string. That way we don't need to suppress them in all the callsites.
It would obviously need to go through API review, but I think it might be worth it to add.
There was a problem hiding this comment.
I agree that it would be valuable for our trimming purposes, but not so sure if that is good enough value to add the extra overloads through API Review. Anyway, I'll log an issue to track this proposal and let area-owners/ApiProposal board to decide.
There was a problem hiding this comment.
We did for JsonValue: #52773. The generic overload was unsafe, but we added overloads for the primitive values because they are "safe".
|
Hello @joperezr! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
ghost
merged commit 
ghost
locked as resolved and limited conversation to collaborators
Resolving most of the warnings on Microsoft.Extensions.Hosting.
The last two warnings are not so easily suppressible today, so the plan for now will instead be to wait for @vitek-karas's change dotnet/linker#2075 on the linker side which will make these type of warnings easier to suppress.