Duplicate annotations may be emitted

[jonpryor]

Context: 1e8f513 / PR #882
Context: dotnet/android#6327
Context? #883

After commit 1e8f513 was merged, we tried to bump Java.Interop within xamarin-android; dotnet/android#6327. This promptly failed with an API compat error:

# in xamarin-android…
% make prepare && make all && make framework-assemblies
…
…/src/Mono.Android/Mono.Android.targets(247,5): error : CompatApi command: $HOME/.nuget/packages/microsoft.dotnet.apicompat/5.0.0-beta.20181.7/tools/net472/Microsoft.DotNet.ApiCompat.exe "…/bin/Debug/lib/xamarin.android/xbuild-frameworks/MonoAndroid/v4.4.87/Mono.Android.dll" -i "…/bin/Debug/lib/xamarin.android/xbuild-frameworks/MonoAndroid/v5.0" --allow-default-interface-methods --baseline "…/tests/api-compatibility/acceptable-breakages-v5.0.txt" --validate-baseline --exclude-attributes "…/tests/api-compatibility/api-compat-exclude-attributes.txt"
…/src/Mono.Android/Mono.Android.targets(247,5): error : CheckApiCompatibility found nonacceptable Api breakages for ApiLevel: v5.0.
…/src/Mono.Android/Mono.Android.targets(247,5): error : Compat issues with assembly Mono.Android:
…/src/Mono.Android/Mono.Android.targets(247,5): error : CannotChangeAttribute : Attribute 'Android.Runtime.RequiresPermissionAttribute' on 'Android.Net.ConnectivityManager.GetNetworkInfo(Android.Net.ConnectivityType)' changed from '[RequiresPermissionAttribute("quot;android.permission.ACCESS_NETWORK_STATE&quot")]' in the contract to '[RequiresPermissionAttribute("quot;android.permission.ACCESS_NETWORK_STATE&quot")]' in the implementation.
…/src/Mono.Android/Mono.Android.targets(247,5): error : CannotChangeAttribute : Attribute 'Android.Runtime.RequiresPermissionAttribute' on 'Android.OS.Vibrator.Vibrate(System.Int64[], System.Int32)' changed from '[RequiresPermissionAttribute("quot;android.permission.VIBRATE&quot")]' in the contract to '[RequiresPermissionAttribute("quot;android.permission.VIBRATE&quot")]' in the implementation.
…/src/Mono.Android/Mono.Android.targets(247,5): error : Total Issues: 2

The error is rather non-sensical: the attribute was replaced with itself?

CannotChangeAttribute : Attribute 'Android.Runtime.RequiresPermissionAttribute' on 'Android.Net.ConnectivityManager.GetNetworkInfo(Android.Net.ConnectivityType)' changed from '[RequiresPermissionAttribute("quot;android.permission.ACCESS_NETWORK_STATE&quot")]' in the contract to '[RequiresPermissionAttribute("quot;android.permission.ACCESS_NETWORK_STATE&quot")]'

Looking at the source provides more context:

% cd src/Mono.Android
% diff -rup obj/Debug/monoandroid10/android-{20,21}/mcw/Android.Net.ConnectivityManager.cs
--- obj/Debug/monoandroid10/android-20/mcw/Android.Net.ConnectivityManager.cs   2021-09-22 18:55:32.000000000 -0400
+++ obj/Debug/monoandroid10/android-21/mcw/Android.Net.ConnectivityManager.cs   2021-09-23 13:17:02.000000000 -0400
…
@@ -277,7 +817,7 @@ namespace Android.Net {
 
                // Metadata.xml XPath method reference: path="/api/package[@name='android.net']/class[@name='ConnectivityManager']/method[@name='getNetworkInfo' and count(parameter)=1 and parameter[1][@type='int']]"
                [Register ("getNetworkInfo", "(I)Landroid/net/NetworkInfo;", "GetGetNetworkInfo_IHandler")]
-               [global::Android.Runtime.RequiresPermission ("quot;android.permission.ACCESS_NETWORK_STATE&quot")][global::Android.Runtime.RequiresPermission ("quot;android.permission.ACCESS_NETWORK_STATE&quot")]
+               [global::Android.Runtime.RequiresPermission ("quot;android.permission.ACCESS_NETWORK_STATE&quot")]
                public virtual unsafe Android.Net.NetworkInfo? GetNetworkInfo ([global::Android.Runtime.GeneratedEnum] Android.Net.ConnectivityType networkType)
                {
                        const string __id = "getNetworkInfo.(I)Landroid/net/NetworkInfo;";

The change in this case is that we went from two RequiresPermission("quot;android.permission.ACCESS_NETWORK_STATE&quot") attributes to one.

Thus, the question: Why are there two RequiresPermission attributes with the same value?

Turns out, this is kinda common? This also happens with Android.OS.Vibrator.Vibrate(long):

namespace Android.OS {
    public abstract partial class Vibrator : Java.Lang.Object {
        [global::Android.Runtime.RequiresPermission ("quot;android.permission.VIBRATE&quot")][global::Android.Runtime.RequiresPermission ("quot;android.permission.VIBRATE&quot")]
        public abstract void Vibrate (long milliseconds);
    }
}

The immediate cause of the API regression failures is commit 1e8f513, in that these regressions are "caused" by the newly added support for androidx.annotation.RequiresPermission. Previously, these attributes were ignored, so nothing happened.

However, that's not the root cause. Why are we emitting multiple attributes with the same value in the first place?

I still don't have an answer to that. I've tried tracing all changes to MethodBase.Annotation, but haven't yet found where the first set is located.

Debugging-diff:

diff --git a/src/Xamarin.Android.Tools.AnnotationSupport/ManagedTypeFinders/PermissionManagedTypeFinderExtension.cs b/src/Xamarin.Android.Tools.AnnotationSupport/ManagedTypeFinders/PermissionManagedTypeFinderExtension.cs
index d15ed59e..989ca22e 100644
--- a/src/Xamarin.Android.Tools.AnnotationSupport/ManagedTypeFinders/PermissionManagedTypeFinderExtension.cs
+++ b/src/Xamarin.Android.Tools.AnnotationSupport/ManagedTypeFinders/PermissionManagedTypeFinderExtension.cs
@@ -17,6 +17,7 @@ namespace Xamarin.AndroidTools.AnnotationSupport
 			foreach (var a in item.Annotations) {
 				if (a.Values == null || a.Name != "RequiresPermission")
                                         continue;
+				Console.Error.WriteLine ($"# jonp: ProcessAnnotation: item=`{item}`; values={{ {string.Join (", ", a.Values)} }} ");
 				for (int i = 0; i < a.Values.Count; i++) {
 					var v = a.Values [i];
 					if (v.Name != "value")
@@ -30,6 +31,9 @@ namespace Xamarin.AndroidTools.AnnotationSupport
 					string val = v.Val.Substring (1, v.Val.Length - 2);
 					if (!ext.Values.Contains (val))
 						ext.Values.Add (val);
+					if (ext.Values.Count > 1) {
+						Console.Error.WriteLine ($"# jonp: ProcessAnnotation: *two* values? {string.Join (", ", ext.Values)}");
+					}
 				}
 			}
 		}
diff --git a/src/Xamarin.Android.Tools.AnnotationSupport/Objects/AnnotationData.cs b/src/Xamarin.Android.Tools.AnnotationSupport/Objects/AnnotationData.cs
index 7baef5b1..813c1b03 100644
--- a/src/Xamarin.Android.Tools.AnnotationSupport/Objects/AnnotationData.cs
+++ b/src/Xamarin.Android.Tools.AnnotationSupport/Objects/AnnotationData.cs
@@ -39,5 +39,10 @@ namespace Xamarin.AndroidTools.AnnotationSupport
 		
 		public string Name { get; set; }
 		public IList<AnnotationValue> Values { get; private set; }
+
+		public override string ToString ()
+		{
+			return $"AnnotationData {{ Name={Name}, Values={{ {string.Join (", ", Values)} }} }} ";
+		}
 	}
 }
diff --git a/tools/generator/CodeGenerator.cs b/tools/generator/CodeGenerator.cs
index ca7b24b8..c0de5dab 100644
--- a/tools/generator/CodeGenerator.cs
+++ b/tools/generator/CodeGenerator.cs
@@ -345,18 +345,29 @@ namespace Xamarin.Android.Binder
 			foreach (var z in zips)
 				annotations.Load (z);
 
+int _v = 0;
 			foreach (var item in annotations.Data.SelectMany (d => d.Value)) {
 				if (!item.Annotations.Any (a => a.Name == "RequiresPermission"))
 					continue;
 				var cx = item.GetExtension<RequiresPermissionExtension> ();
 				if (cx == null)
 					continue;
+				if (item.ToString ().Contains ("android.os.Vibrator.vibrate(long)")) {
+					_v++;
+					Console.Error.WriteLine ($"# jonp: found vibrate: {item}");
+					Console.Error.WriteLine ($"# jonp: cur value: `{item.ManagedInfo.MethodObject.Value ().Annotation}`");
+					Console.Error.WriteLine ($"# jonp: managedinfo type: `{item.ManagedInfo.GetType ()}`");
+					Console.Error.WriteLine ($"# jonp: methodobject type: `{item.ManagedInfo.MethodObject.GetType ()}`");
+					Console.Error.WriteLine ($"# jonp: cx.Values={{ {string.Join (", ", cx.Values) } }}");
+					Console.Error.WriteLine ($"{new System.Diagnostics.StackTrace(true)}");
+				}
 				string annotation = null;
 				foreach (var value in cx.Values)
 					annotation += string.Format ("[global::Android.Runtime.RequiresPermission (\"{0}\")]", value);
 
 				AddAnnotationTo (item, annotation);
 			}
+			Console.WriteLine ($"# jonp: found {_v} instances of vibrate(long)");
 
 			foreach (var item in annotations.Data.SelectMany (d => d.Value)) {
 				if (!item.Annotations.Any (a => a.Name == "IntDef" || a.Name == "StringDef"))
@@ -386,8 +397,18 @@ namespace Xamarin.Android.Binder
 			if (item.ManagedInfo.PropertyObject != null)
 				item.ManagedInfo.PropertyObject.Value ().Annotation += annotation;
 			else if (item.ManagedInfo.MethodObject != null) {
-				if (item.ParameterIndex < 0)
+				if (item.ParameterIndex < 0) {
+					if (item.ToString ().Contains ("android.os.Vibrator.vibrate(long)")) {
+						Console.Error.WriteLine ($"# jonp: AddAnnotationTo: vibrate(long): adding annotation=`{annotation}`");
+						Console.Error.WriteLine ($"# jonp: AddAnnotationTo: cur value={item.ManagedInfo.MethodObject.Value ().Annotation}");;
+						Console.Error.WriteLine ($"{new System.Diagnostics.StackTrace(true)}");
+					}
+					if ((annotation?.Contains ("RequiresPermission") ?? false) &&
+							(item.ManagedInfo.MethodObject.Value ().Annotation?.Contains ("RequiresPermission") ?? false)) {
+						Console.Error.WriteLine ($"# jonp: two requiresperms? item={item}");
+					}
 					item.ManagedInfo.MethodObject.Value ().Annotation += annotation;
+				}
 				else
 					item.ManagedInfo.MethodObject.Value ().Parameters [item.ParameterIndex].Annotation += annotation;
 			}
diff --git a/tools/generator/Extensions/GenBaseExtensions.cs b/tools/generator/Extensions/GenBaseExtensions.cs
index cae65c97..4e12b977 100644
--- a/tools/generator/Extensions/GenBaseExtensions.cs
+++ b/tools/generator/Extensions/GenBaseExtensions.cs
@@ -66,6 +66,10 @@ namespace Xamarin.AndroidTools.AnnotationSupport
 		}
 		public static MethodBase Value (this ManagedTypeFinder.IMethodBase t)
 		{
+			var _m = ((ManagedTypeFinderGeneratorTypeSystem.TMethodBase) t)?.Value;
+			if (t != null && string.Equals("vibrate", _m?.Name, StringComparison.OrdinalIgnoreCase)) {
+				Console.Error.WriteLine ($"# jonp: getting vibrate method value; annotation=`{_m.Annotation}`");
+			}
 			return ((ManagedTypeFinderGeneratorTypeSystem.TMethodBase) t)?.Value;
 		}
 
@@ -83,6 +87,9 @@ namespace Xamarin.AndroidTools.AnnotationSupport
 		}
 		public static ManagedTypeFinder.IMethodBase Wrap (this MethodBase t)
 		{
+			if (t != null && string.Equals("vibrate", t.Name, StringComparison.OrdinalIgnoreCase)) {
+				Console.Error.WriteLine ($"# jonp: wrapping a vibrate method; annotation=`{t.Annotation}`");
+			}
 			return t == null ? null : new ManagedTypeFinderGeneratorTypeSystem.TMethodBase () { Value = t };
 		}
 

Snippet of output with above diff applied:

% mono --debug=casts "…/xamarin-android/bin/Debug/lib/xamarin.android/xbuild/Xamarin/Android/generator.exe" --public --product-version=7 --api-level=21 -o "obj/Debug/monoandroid10/android-21/mcw" --codegen-target=XAJavaInterop1 --fixup=metadata --preserve-enums --enumflags=enumflags --enumfields=map.csv --enummethods=methodmap.csv --enummetadata=obj/Debug/monoandroid10/android-21/mcw/enummetadata  --annotations="/Users/jon/android-toolchain/sdk/platform-tools/api/annotations.zip" --assembly="Mono.Android, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null" --type-map-report=obj/Debug/monoandroid10/android-21/mcw/type-mapping.txt --lang-features=nullable-reference-types --enumdir=obj/Debug/monoandroid10/android-21/mcw obj/Debug/monoandroid10/android-21/mcw/api.xml 2>&1
…
# jonp: found vibrate: @RequiresPermission(value=System.String[]) android.os.Vibrator.vibrate(long)
# jonp: getting vibrate method value; annotation=`[global::Android.Runtime.RequiresPermission ("quot;android.permission.VIBRATE&quot")]`
# jonp: cur value: `[global::Android.Runtime.RequiresPermission ("quot;android.permission.VIBRATE&quot")]`
# jonp: managedinfo type: `Xamarin.AndroidTools.AnnotationSupport.ManagedMemberInfo`
# jonp: methodobject type: `Xamarin.AndroidTools.AnnotationSupport.ManagedTypeFinderGeneratorTypeSystem+TMethodBase`
# jonp: cx.Values={ quot;android.permission.VIBRATE&quot }
…
# jonp: AddAnnotationTo: vibrate(long): adding annotation=`[global::Android.Runtime.RequiresPermission ("quot;android.permission.VIBRATE&quot")]`
# jonp: getting vibrate method value; annotation=`[global::Android.Runtime.RequiresPermission ("quot;android.permission.VIBRATE&quot")]`
# jonp: AddAnnotationTo: cur value=[global::Android.Runtime.RequiresPermission ("quot;android.permission.VIBRATE&quot")]
…
# jonp: getting vibrate method value; annotation=`[global::Android.Runtime.RequiresPermission ("quot;android.permission.VIBRATE&quot")]`
# jonp: two requiresperms? item=@RequiresPermission(value=System.String[]) android.os.Vibrator.vibrate(long)

Vibrator.vibrate(long) only appears to be processed once, yet the very first time my debugging code encounters it, it already has an Annotation set. I don't see where else the Annotation could be set, so I'm still puzzling over this one…

[jpobst]

The issue is that the annotations.zip file contains the following entries:

<item name="android.os.Vibrator void vibrate(android.os.VibrationEffect)">
  <annotation name="androidx.annotation.RequiresPermission">
    <val name="value" val="&quot;android.permission.VIBRATE&quot;" />
  </annotation>
</item>
<item name="android.os.Vibrator void vibrate(long)">
  <annotation name="androidx.annotation.RequiresPermission">
    <val name="value" val="&quot;android.permission.VIBRATE&quot;" />
  </annotation>
</item>

Which correspond to these overloads:

public virtual void Vibrate (Android.OS.VibrationEffect vibe);
public virtual void Vibrate (long milliseconds);

However, the overload that takes Android.OS.VibrationEffect wasn't added until API-26, so when binding API-21 only the overload taking long exists.

Our method finding code first does a "loose" match where it sees if there is only 1 bound method that has the same number of parameters as the annotation. If so, it uses that overload without checking if the parameter types match.

In this case, both annotations "match" the only overload with a single parameter, so both annotations get applied to it, causing the duplicate.