Implement IMetadataResolver for LinkContext #2045
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This will allow extension methods that take an `IMetadataResolver` like in dotnet/java-interop#842 to use the LinkContext Resolve cache. Context: dotnet/android#5748 (comment) The Resolve cache added in dotnet#1979 requires calling `Resolve*Definition` methods directly on `LinkContext`, which means that any extension methods that do resolution logic need to take a `LinkContext`. This doesn't work well with the layering in xamarin-android, where java.interop uses a resolution cache with cecil, but doesn't depend on the linker. Instead it uses a custom `TypeResolutionCache` for extension methods like `GetBaseDefinition`: https://github.com/xamarin/java.interop/blob/main/src/Java.Interop.Tools.Cecil/Java.Interop.Tools.Cecil/MethodDefinitionRocks.cs#L16 These extension methods are also used from xamarin-android, but there's a desire to use the `LinkContext` cache in this case. @jonpryor had the idea to change the extension methods to use cecil's `IMetadataResolver`, which can be implemented by `TypeDefinitionCache` and by `LinkContext`. Java.interop will continue using their `TypeDefinitionCache`, and xamarin-android will use `LinkContext`. One limitation of this approach is that `LinkContext.TryResolve*Definition` (renamed to just `TryResolve` for consistency) methods aren't usable from the extension methods.
vitek-karas
approved these changes
May 20, 2021
marek-safar
approved these changes
May 20, 2021
jonpryor
pushed a commit
to dotnet/android
that referenced
this pull request
Jun 2, 2021
Context: dotnet/linker#1953 Context: dotnet/linker#1774 Context: dotnet/linker#1774 (comment) Context: dotnet/linker#1979 Context: dotnet/linker#2019 Context: dotnet/linker#2045 Context: dotnet/java-interop@2573dc8 Context: #5870 Context: #5878 Update the custom linker steps to use the new `IMarkHandler` API which runs logic during "MarkStep". (See [this list][0] of pipeline steps for additional context.) As part of this, I've removed the workaround that loads all referenced assemblies up-front and simplified some of the linker MSBuild targets. Some of the `MonoDroid.Tuner` steps were duplicated and changed to implement the new `IMarkHandler` interface, to avoid touching the `MonoDroid.Tuner` code. In my analysis of the custom steps, most of them "just work" if we call them only for marked members, because they ultimately either: - Just call `AddPreserved*()` to conditionally keep members of types (which with the new API will just happen when the type is marked) - In the case of `FixAbstractMethodsStep()`, inject missing interface implementations which will only be kept if they are marked for some other reason. Most of the steps have been updated in a straightforward way based on the above. The exceptions are: - `AddKeepAlivesStep` needs to run on all code that survived linking, and can run as a normal step. - `ApplyPreserveAttribute`: this step globally marks members with `PreserveAttribute`. - The step is only active for non-SDK link assemblies. Usually we root non-SDK assemblies, but it will be a problem if linking all assemblies. - For now, I updated it to use the new custom step API on assembly mark -- so it will scan for the attribute in all marked assemblies -- but we should investigate whether this is good enough. - This can't be migrated to use `IMarkHandler` because it needs to scan code for attributes, including unmarked code. - `PreserveExportedTypes`: similar to the above, this globally marks members with `Export*Attribute`. It's used for java interop in cases where the java methods aren't referenced statically, like from xml, or for special serialization methods. - It's only active for non-SDK assemblies, so like above it will be a problem only if linking all assemblies. - Like above, I've made it scan marked assemblies. - `PreserveApplications`: globally scans for `ApplicationAttribute` on types/assemblies, and sets the `TypePreserve` annotation for any types referenced by the attribute. - This step technically does a global scan, but it's likely to work on "mark type"/"mark assembly", as `ApplicationAttribute` is only used on types/assembies that are already kept. - I've updated it to use the new `IMarkHandler` API. Additionally, as per dotnet/java-interop@2573dc8c, stop using `TypeDefinitionCache` everywhere and instead use `IMetadataResolver` to support caching `TypeDefinition`s across shared steps. For .NET 6, `LinkContextMetadataResolver` implements the `IMetadataResolver` interface in terms of `LinkContext`. [0]: https://github.com/mono/linker/blob/main/src/linker/Linker/Driver.cs#L714-L730
agocke
pushed a commit
to dotnet/runtime
that referenced
this pull request
Nov 16, 2022
This will allow extension methods that take an `IMetadataResolver` like in dotnet/java-interop#842 to use the LinkContext Resolve cache. Context: dotnet/android#5748 (comment) The Resolve cache added in dotnet/linker#1979 requires calling `Resolve*Definition` methods directly on `LinkContext`, which means that any extension methods that do resolution logic need to take a `LinkContext`. This doesn't work well with the layering in xamarin-android, where java.interop uses a resolution cache with cecil, but doesn't depend on the linker. Instead it uses a custom `TypeResolutionCache` for extension methods like `GetBaseDefinition`: https://github.com/xamarin/java.interop/blob/main/src/Java.Interop.Tools.Cecil/Java.Interop.Tools.Cecil/MethodDefinitionRocks.cs#L16 These extension methods are also used from xamarin-android, but there's a desire to use the `LinkContext` cache in this case. @jonpryor had the idea to change the extension methods to use cecil's `IMetadataResolver`, which can be implemented by `TypeDefinitionCache` and by `LinkContext`. Java.interop will continue using their `TypeDefinitionCache`, and xamarin-android will use `LinkContext`. One limitation of this approach is that `LinkContext.TryResolve*Definition` (renamed to just `TryResolve` for consistency) methods aren't usable from the extension methods. Commit migrated from dotnet/linker@aaf4880
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This will allow extension methods that take an
IMetadataResolverlikein dotnet/java-interop#842 to use the
LinkContext Resolve cache.
Context: dotnet/android#5748 (comment)
The Resolve cache added in #1979
requires calling
Resolve*Definitionmethods directly onLinkContext,which means that any extension methods that do resolution logic need to
take a
LinkContext. This doesn't work well with the layering inxamarin-android, where java.interop uses a resolution cache with cecil,
but doesn't depend on the linker. Instead it uses a custom
TypeResolutionCachefor extension methods likeGetBaseDefinition:https://github.com/xamarin/java.interop/blob/main/src/Java.Interop.Tools.Cecil/Java.Interop.Tools.Cecil/MethodDefinitionRocks.cs#L16
These extension methods are also used from xamarin-android, but there's
a desire to use the
LinkContextcache in this case.@jonpryor had the idea to change the extension methods to use cecil's
IMetadataResolver, which can be implemented byTypeDefinitionCacheand by
LinkContext. Java.interop will continue using theirTypeDefinitionCache,and xamarin-android will use
LinkContext.One limitation of this approach is that
LinkContext.TryResolve*Definition(renamed to just
TryResolvefor consistency) methods aren't usable fromthe extension methods.
This will also need to be updated in xamarin-macios, which
has already startedshould also be use theusing
(Try)Resolve*Definitionmethods. /cc @spouliot @rolfbjarne