Make Extensions linker friendly. #40431
Conversation
src/libraries/Microsoft.Extensions.Logging.Console/src/ConsoleLoggerExtensions.cs
Show resolved
Hide resolved
...braries/Microsoft.Extensions.Logging.Console/tests/TrimmingTests/AddConsoleFormatterTests.cs
Show resolved
Hide resolved
I think we have to suppress these. There's nothing we can flow from here. Any open generic registered into the container needs to do the needful and mark things appropriately. |
This is unfortunate but understandable (especially because it's polymorphic) |
|
For the warnings that refer to the DisplayClass - we could work around by adding our own closure class that is properly annotated. Linker can't do much without trying to reverse engineer C# compiler operations and that's a sketchy territory (I don't think the Roslyn team would be happy if we were to take a dependency on their manglings). For MakeGenericMethod - the risk is basically that if we can end up with MakeGenericMethod creating generic methods that have generic parameters annotated with dataflow annotations, linker no longer enforces the annotations (because linker doesn't run at runtime) and things could start failing. If we're convinced that can't happen, suppression is okay. Basically suppression always means that things could start failing at runtime because we're turning off automatic checking. It needs to be done with extreme care because one missed spot and everything could fall apart with a hard to diagnose exception at runtime. We also need to remember that a method got |
Why can't we do something automagical for compiler generated types (like inherit attributes when they are from compiler generated types).
Right, the way this system works it make sense to suppress. It can break but we'll need to document this pattern in general (open generics registered in DI have this problem and there's no single fix). |
It requires threading the annotations through to the closure constructor, and to the instance fields. It's not impossible but it requires careful implementation of the right constructs that the C# compiler can emit for these. We've been avoiding doing crossmethod dataflow analysis because it's slow to compute and starts to be complicated to both compute and report warnings on once things like virtual and interface methods are supported. Closure classes might be less complicated, but it still requires hardcoding C# compiler knowledge I would rather not hardcode. |
|
It needs to be solved though, if we change people's coding patterns because they can't write annotations on closures, it basically means you'll get warnings that need to be ignored forever. Forcing me to rewrite my code to add an annotation is also not great. |
|
The code needs to be written in a way that can be statically analyzed. This pull request is doing it in e.g. ProviderAliasUtilities.cs already. There will be reflection code that will work as-is. There will be code where you add annotations and it will work. There will be code that you need to tweak before you can annotate it. There will be code that you need to throw out and write a source generator. C# has a lot of these closure-based constructs where it will generate a lot of goo that can only be analyzed with whole program dataflow for something that is a one-liner in the C# source. There's |
What's the failure mode when the properties are missing? I have bad memories of missing properties in XAML databinding in .NET Native - XAML is pretty quiet when a property doesn't exist so a bug where we trimmed something that shouldn't have been trimmed usually manifested itself with "the UI looks off, but there's no exception thrown anywhere". Those were fun to debug. |
Fix trimming tests.
Asking people rewrite their code is a non starter. It would be better if there were more well defined contracts defined between the linker and the compiler (that's why I initially focused on |
From what I can tell it silently skips the properties without setters. So you can start getting I've opened #40551 to make the Binder linker-safe.
Would it be possible for the compiler to flow the generic type attributes to the generated code? That doesn't seem unreasonable to me, and I think it could solve the problem with |
That should be feasible and seems like a clean solution. Let's open an issue with the Roslyn team about it. |
I've opened dotnet/roslyn#46646 for this. |
|
@eerhardt Why didn't we also update the ref assemblies with the new attributes? |
* Make Extensions linker friendly. Annotating the rest of the Microsoft.Extensions libraries. Fix dotnet#40396 * Disable parallelism in trimming tests to fix race condition.
Mostly because I forgot to. I've opened #40621 which is adding them. As an aside - these assemblies don't target |
ghost
locked as resolved and limited conversation to collaborators
Annotating the rest of the Microsoft.Extensions libraries.
Fix #40396
With these changes, there are the following linker warnings left in Microsoft.Extensions assemblies: