-
Notifications
You must be signed in to change notification settings - Fork 560
Description
I'm filing this issue to track concerns that came up in #5748 (comment). See that discussion for more context. The basic issue is that some base/override method comparisons don't account for the .override
directive or variance of parameters.
PreserveRegistrations calls the helper TryGetBaseOrInterfaceRegisterMember
to look for matching base methods:
It is also called from MonoDroidMarkStep
, but I haven't looked into this usage.
There are a few potential issues along the lines that @jonpryor mentioned in #5748 (comment), but I don't have the full context to understand how concerning these are:
-
When looking over base methods
https://github.com/xamarin/xamarin-android/blob/681887ebdbd192ce7ce1cd02221d4939599ba762/src/Xamarin.Android.Build.Tasks/Linker/MonoDroid.Tuner/Extensions.cs#L267
the parameters are compared using a check that doesn't consider contravariance of method parameters:
https://github.com/xamarin/java.interop/blob/0227cdaea1eaf11d9e77102e2e4c853f5f18441c/src/Java.Interop.Tools.Cecil/Java.Interop.Tools.Cecil/MethodDefinitionRocks.cs#L29 -
When looking over interface "base" methods
the
.override
directive isn't considered
Compare this with FixAbstractMethodsStep:
https://github.com/xamarin/xamarin-android/blob/da536fcf1b663fd71adee40c06466547a04cc6ed/src/Xamarin.Android.Build.Tasks/Linker/MonoDroid.Tuner/FixAbstractMethodsStep.cs#L228-L232
which does check for overrides
https://github.com/xamarin/xamarin-android/blob/da536fcf1b663fd71adee40c06466547a04cc6ed/src/Xamarin.Android.Build.Tasks/Linker/MonoDroid.Tuner/FixAbstractMethodsStep.cs#L172-L175
though I don't think it does anything to handle variance.
Long-term I think the ideal approach would be to build these kinds of comparisons into the linker and make the APIs available to custom steps. There may be more cases that need to be considered, such as default interface methods, and static virtual interface methods.