Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[generator] Support [Obsolete]/[SupportedOSPlatform] attributes for enum members. #1066

Merged
merged 1 commit into from
Dec 14, 2022

Conversation

jpobst
Copy link
Contributor

@jpobst jpobst commented Dec 12, 2022

Fixes: #1037

Adds support for [Obsolete]/[ObsoletedOSPlatform] and [SupportedOSPlatform] attributes on enum members.

[SupportedOSPlatform]

The data for [SupportedOSPlatform] comes from the values provided in the enum-mapping file:

// src/Mono.Android/map.csv
E,29,android/media/MediaRecorder$AudioEncoder.OPUS,7,Android.Media.AudioEncoder,Opus,keep,

The 29 is the API in which we added the enum, which becomes [SupportedOSPlatform ("android-29.0")].

[Obsolete]/[ObsoletedOSPlatform]

The data for the "obsolete" attributes comes from the deprecated and deprecated-since attributes on the original field in the api.xml (if it can be found):

<field deprecated='deprecated' deprecated-since='31' api-since='30' final='true' name='BIND_CHOOSER_TARGET_SERVICE' jni-signature='Ljava/lang/String;' static='true' transient='false' type='java.lang.String' type-generic-aware='java.lang.String' value='&quot;android.permission.BIND_CHOOSER_TARGET_SERVICE&quot;' visibility='public' volatile='false' />

This example results in:

// When using classic obsolete attributes:
[global::System.Obsolete("deprecated")]

// When using new obsolete attributes:
[global::System.Runtime.Versioning.ObsoletedOSPlatform("android31.0")]

One wrinkle is we may have obsoleted the field because we want the user to use the enum instead:

<field deprecated='This constant will be removed in the future version. Use Android.App.RecentTaskFlags enum directly instead of this field.' final='true' name='RECENT_IGNORE_UNAVAILABLE' jni-signature='Ljava/lang/String;' static='true' transient='false' type='java.lang.String' type-generic-aware='java.lang.String' value='&quot;android.permission.BIND_CHOOSER_TARGET_SERVICE&quot;' visibility='public' volatile='false' deprecated-since='31' api-since='30' />

We need to detect this message and not obsolete the enum in this case.

@jpobst
Copy link
Contributor Author

jpobst commented Dec 12, 2022

All the examples provided in #1037 appear correct with this PR:

[Register("POST_NOTIFICATIONS", ApiSince=33)]
[SupportedOSPlatform("android33.0")]
public const string PostNotifications = "android.permission.POST_NOTIFICATIONS";
public enum VibrationEffectSupport
{
  [IntDefinition("Android.OS.Vibrator.VibrationEffectSupportUnknown", JniField="android/os/Vibrator.VIBRATION_EFFECT_SUPPORT_UNKNOWN")]
  [SupportedOSPlatform("android30.0")]
  Unknown = 0,
  [IntDefinition("Android.OS.Vibrator.VibrationEffectSupportYes", JniField="android/os/Vibrator.VIBRATION_EFFECT_SUPPORT_YES")]
  [SupportedOSPlatform("android30.0")]
  Yes = 1,
  [IntDefinition("Android.OS.Vibrator.VibrationEffectSupportNo", JniField="android/os/Vibrator.VIBRATION_EFFECT_SUPPORT_NO")]
  [SupportedOSPlatform("android30.0")]
  No = 2
}
[Register("ACTION_POST_CALL", ApiSince=30)]
[SupportedOSPlatform("android30.0")]
public const string ActionPostCall = "android.telecom.action.POST_CALL";

@jpobst jpobst marked this pull request as ready for review December 13, 2022 14:53
@jonpryor
Copy link
Member

One oddity reviewing this PR is that the description doesn't match my expected usage. For example, the description mentions a <field/>, but the field is for a string, which isn't something that is enumified.

I've attempted to update the commit message to "make sense in context".

[generator] Obsolete&SupportedOSPlatform attributes on enum members (#1066)

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

Adds support for emitting `[Obsolete]`, `[ObsoletedOSPlatform]`, and
`[SupportedOSPlatform]` custom attributes on enum members.

~~ [SupportedOSPlatform] ~~

The data for `[SupportedOSPlatform]` comes from the values provided
in the enum-mapping file (b00e644e), e.g.
[`xamarin-android/src/Mono.Android/map.csv`][0]:

	// src/Mono.Android/map.csv
	E,29,android/media/MediaRecorder$AudioEncoder.OPUS,7,Android.Media.AudioEncoder,Opus,keep,

The `29` is the API in which we added the enum, which becomes
`[SupportedOSPlatform("android-29.0")]`.

~~ [Obsolete] and [ObsoletedOSPlatform] ~~

The data for the "obsolete" attributes comes from the `deprecated`
and `deprecated-since` attributes on the original field in the
`api.xml`, if it can be found.  For example, given:

	<class name="AccessibilityServiceInfo" jni-signature="Landroid/accessibilityservice/AccessibilityServiceInfo;" …>
	    <field
	            deprecated="deprecated"
	            final="true"
	            name="CAPABILITY_CAN_REQUEST_ENHANCED_WEB_ACCESSIBILITY"
	            jni-signature="I"
	            static="true"
	            transient="false"
	            type="int"
	            type-generic-aware="int"
	            value="4"
	            visibility="public"
	            volatile="false"
	            deprecated-since="26"
	    />
	</class>

in combination with these `map.csv` (b00e644e) entries:

	A,0,,0,Android.AccessibilityServices.AccessibilityServiceCapabilities,None,remove,
	E,18,android/accessibilityservice/AccessibilityServiceInfo.CAPABILITY_CAN_REQUEST_ENHANCED_WEB_ACCESSIBILITY,4,Android.AccessibilityServices.AccessibilityServiceCapabilities,CanRequestEnhancedWebAccessibility,remove,

results in:

	/* partial */ enum AccessibilityServiceCapabilities {
	    // When using classic obsolete attributes:
	    [global::System.Obsolete("deprecated")]

	    // When using new obsolete attributes:
	    [global::System.Runtime.Versioning.ObsoletedOSPlatform("android31.0")]
	    CanRequestEnhancedWebAccessibility = 4,
	}

One wrinkle is we may have obsoleted the field because we want the
user to use the enum instead:

	<field
	    deprecated='This constant will be removed in the future version. Use Android.App.RecentTaskFlags enum directly instead of this field.'
	    name='RECENT_IGNORE_UNAVAILABLE'
	
	/>

We need to detect this message and not obsolete the enum in this case.

[0]: https://github.com/xamarin/xamarin-android/blob/17213ea184e23a9020451b265fec459558278489/src/Mono.Android/map.csv

@jpobst
Copy link
Contributor Author

jpobst commented Dec 14, 2022

That's fine, I likely pulled some examples from the reported issue, which covered both consts and enums. The consts aspect was fixed in #1038.

@jonpryor jonpryor merged commit 5e6209e into main Dec 14, 2022
@jonpryor jonpryor deleted the obsolete-enums branch December 14, 2022 21:54
jonpryor pushed a commit to xamarin/xamarin-android that referenced this pull request Dec 16, 2022
Context: 4da2792
Context: xamarin/java.interop@d3ea180
Context: xamarin/java.interop@15c8879

Changes: xamarin/java.interop@149d70f...f8d77fa

  * xamarin/java.interop@f8d77faf: [generator] Better support deprecated property getter/setters. (xamarin/java.interop#1062)
  * xamarin/java.interop@5e6209ea: [generator] Obsolete&SupportedOSPlatform attributes on enum members (xamarin/java.interop#1066)
  * xamarin/java.interop@15c88797: [generator] Use decl type's @deprecated-since if < member's (xamarin/java.interop#1068)
  * xamarin/java.interop@525a45d5: [Java.Interop.Dynamic-Tests] Use Microsoft.CSharp NuGet package (xamarin/java.interop#1067)

Background: member deprecations can be "historically weird" in Android.
For example, in API-21 [`android/app/ActionBar.TabListener`][0] was
deprecated:

	@deprecated
	/* partial */ interface TabListener {
	    void onTabReselected(ActionBar.Tab tab, FragmentTransaction ft);
	    // …
	}

The type being deprecated means that its members are *implicitly*
deprecated.

In API-29 the members were *explicitly* deprecated:

	@deprecated
	/* partial */ interface TabListener {
	    @deprecated
	    void onTabReselected(ActionBar.Tab tab, FragmentTransaction ft);
	    // …
	}

Before xamarin/java.interop@d3ea180c, this resulted in the binding:

	[Obsolete]
	partial interface ITabListener {
	    [Obsolete]
	    void OnTabReselected(ActionBar.Tab? tab, FragmentTransaction? ft);
	    // …
	}

Commit xamarin/java.interop@d3ea180c added support for
`[ObsoletedOSPlatform]` to `generator`, which *changed* the binding to:

	[Obsolete] // because it was deprecated in our min-supported API-21
	partial interface ITabListener {
	    [ObsoletedOSPlatform ("android29.0")]
	    void OnTabReselected(ActionBar.Tab? tab, FragmentTransaction? ft);
	    // …
	}

This resulted in *lots* of changes in commit 4da2792 to
`tests/api-compatibility/acceptable-breakages-vReference-net7.0.txt`
because `[Obsolete]` was were replaced by `[ObsoletedOSPlatform]`:

	CannotRemoveAttribute : Attribute 'System.ObsoleteAttribute' exists on 'Android.App.ActionBar.ITabListener.OnTabReselected(Android.App.ActionBar.Tab, Android.App.FragmentTransaction)' in the contract but not the implementation

Commit xamarin/java.interop@15c88797 updates type members to have the
same deprecation status as their declaring type, when the member was
deprecated *after* the type was deprecated.  This means we *now*
bind `ITabListener` as:

	[Obsolete] // because it was deprecated in our min-supported API-21
	partial interface ITabListener {
	    [Obsolete]
	    void OnTabReselected(ActionBar.Tab? tab, FragmentTransaction? ft);
	    // …
	}

which matches the state of things pre- xamarin/java.interop@d3ea180c,
which means we no longer need to ignore all those
`CannotRemoveAttribute` messages.  xamarin/java.interop@15c88797 thus
"unintentionally partially reverts" 4da2792 (yay?), simply because
the prior state of affairs in which a member was deprecated *after*
the declaring type, while valid, didn't make any sense.

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Jonathan Pobst <jonathan.pobst@microsoft.com>

[0]: https://developer.android.com/reference/android/app/ActionBar.TabListener
@github-actions github-actions bot locked and limited conversation to collaborators Apr 12, 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.

Missing SupportedOSPlatformAttribute for enum members, const members
2 participants