Skip to content

Conversation

@davidfowl
Copy link
Member

@davidfowl davidfowl commented Aug 4, 2020

Follow up to #24458

  • Added RequiresUnreferencedCode to UseStartup that takes a string
  • Added some suppressions
  • Fixed ConfigureContainer calls
  • Fixed HostingStartupAttribute

The other warnings require refactoring to put attributes on code that today exists as lambdas 😢. @agocke @jaredpar I need attributes on local functions so I can put linker attributes on them.

- Added RequiresUnreferencedCode to UseStartup that takes a string
- Added some suppressions
- Fixed ConfigureContainer calls
- Fixed HostingStartupAttribute
@davidfowl davidfowl requested a review from Tratcher as a code owner August 4, 2020 06:03
@ghost ghost added the area-hosting label Aug 4, 2020
@davidfowl davidfowl requested a review from eerhardt August 4, 2020 06:04

// _builder.ConfigureContainer<T>(ConfigureContainer);
typeof(IHostBuilder).GetMethods().First(m => m.Name == nameof(IHostBuilder.ConfigureContainer))
typeof(IHostBuilder).GetMethod(nameof(IHostBuilder.ConfigureContainer))
Copy link
Member

Choose a reason for hiding this comment

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

Previously LINQ was used to get the first ConfigureContainer method for some reason. Do we know why?

I'm guessing there is only a single ConfigureContainer on IHostBuilder.

Copy link
Member Author

Choose a reason for hiding this comment

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

Previously LINQ was used to get the first ConfigureContainer method for some reason. Do we know why?

Lazy and defensive. Unless we decide to break the interface, this is "safe". 😄

I'm guessing there is only a single ConfigureContainer on IHostBuilder.

Correct.

@jaredpar
Copy link
Member

jaredpar commented Aug 4, 2020

I need attributes on local functions so I can put linker attributes on them.

This is supported as of C# 9.0 thanks to @RikkiGibson

return this;
}

[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2006:UnrecognizedReflectionPattern", Justification = "We need to call a generic method on IHostBuilder.")]
Copy link
Member

Choose a reason for hiding this comment

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

The case where this may be unsafe is if IHostBuilder.ConfigureContainer ever puts a DynamicallyAccessedMembers attribute on the TContainerBuilder type. The linker won't be able to see that you are filling in the TContainerBuilder with containerType.

Looking at the implementation In Microsoft.Extensions.Hosting, I don't see HostBuilder needing this, so I think this is safe.

Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

:shipit:

@davidfowl davidfowl merged commit cfd20ad into master Aug 4, 2020
@davidfowl davidfowl deleted the davidfowl/linker-warnings branch August 4, 2020 19:40
@amcasey amcasey added the area-hosting Includes Hosting label Jun 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-hosting Includes Hosting

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants