Skip to content

Conversation

@jpobst
Copy link
Contributor

@jpobst jpobst commented Sep 28, 2021

Context: #885

When updating to the latest annotations.zip file, some Mono.Android.dll members are getting multiple [RequiresPermission] attributes applied. This is because the .zip 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. The fix is to remove the "loose" match and require all matches to be "strict".

A test XA PR was run to ensure this does not cause a regression. It only changed a few attributes, and seems to have made them more accurate: https://github.com/xamarin/xamarin-android/pull/6344/files.

@jpobst jpobst marked this pull request as ready for review September 29, 2021 14:00
@jonpryor
Copy link
Contributor

jonpryor commented Oct 4, 2021

Fixes: https://github.com/xamarin/java.interop/issues/885

Context: 1e8f5137345db3160c99265ff3a56c43a132194f
Context: 3f12cd25aacb5b952e725556fb4c3d6d5cdd7703

Attempting to build the xamarin/xamarin-android repo with a
xamarin/Java.Interop between commits [1e8f5137, 3f12cd25), exclusive
-- as commit 3f12cd25 partially reverted commit 1e8f5137 -- would
result in a build failure during `make framework-assemblies`:

	# 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 cause of the error was twofold:

 1. The `annotations.zip` within `platform-tools/api/annotations.zip`
    for the Android SDK Platform-tools r30 package contained
    annotations in the `androidx.annotation` package, which commit
    1e8f5137 added support for, and

 2. The [method finding code][0] responsible for associating the
    method declared within `annotations.zip` to the `IMethodBase`
    which was responsible for C# codegen was "too loose", and could
    add annotations to methods which shouldn't get them.

Consider this XML snippet from `android/os/annotations.xml` within
`annotations.zip`, for two overloads of `Vibrator.vibrate()`:

	<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 corresponds to the C# bindings:


	partial class Vibrator {
	    public virtual void Vibrate (Android.OS.VibrationEffect vibe);
	    public virtual void Vibrate (long milliseconds);
	}

However, the `Vibrator.Vibrate(VibrationEffect)` overload wasn't
added until API-26.  When binding API-21, only
`Vibrator.Vibrate(long)` exists.  The [method finding code][0] only
checks for *number* of parameters, *not* parameter type, and thus
would add the annotation for `vibrate(VibrationEffect)` to
`vibrate(long)`, resulting in the generated C# binding:

	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);
	    }
	}

i.e. `Vibrator.Vibrate(long)` had *two* `[RequiresPermission]`
attributes, *both* with the same value.

Fix the bug by updating
`ManagedTypeFinderGeneratorTypeSystem.GetMethod()` to try only a
strict method overload match.

Finally, "revert the partial revert" of 3f12cd25, and reintroduce
support for the `androidx.annotation` package for Annotations.

[0]: https://github.com/xamarin/java.interop/blob/c936d09aec8171fa99a37fd99dba253e41fec05d/tools/generator/ManagedTypeFinderGeneratorTypeSystem.cs#L113

@jonpryor jonpryor merged commit 846f211 into main Oct 4, 2021
@jonpryor jonpryor deleted the annotations branch October 4, 2021 20:48
@github-actions github-actions bot locked and limited conversation to collaborators Apr 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants