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

Support androidx.annotation.RestrictTo annotation #1081

Closed
jpobst opened this issue Feb 2, 2023 · 0 comments · Fixed by #1094
Closed

Support androidx.annotation.RestrictTo annotation #1081

jpobst opened this issue Feb 2, 2023 · 0 comments · Fixed by #1094
Labels
enhancement Proposed change to current functionality generator Issues binding a Java library (generator, class-parse, etc.)

Comments

@jpobst
Copy link
Contributor

jpobst commented Feb 2, 2023

Context: https://developer.android.com/reference/androidx/annotation/RestrictTo
Context: xamarin/AndroidX#690

Android has an annotation androidx.annotation.RestrictTo (@RestrictTo) that essentially means "this API is implemented as public, but we do not consider it public API". This means that Google reserves the right to change it at any time (see xamarin/AndroidX#690). Because we simply bind the API as public, it misleads our users into thinking this is a stable API they can rely on.

Example

In androidx.appcompat.appcompat-resources 1.6.0, Google decided to promote classes that were previously marked as @RestrictTo({RestrictTo.Scope.LIBRARY_GROUP_PREFIX}) to full public supported classes. However, when they did this they changed the names of the classes, breaking our users (including XF) who were using the "internal" classes.

1.5.1

image

1.6.0

image

Proposal

We should do something to help protect our users from this scenario. I am not currently aware of any "public but not really" construct in .NET, so we may have to get creative to come up with something.

One idea would be to (ab)use [Obsolete] with a custom warning code and message to explain the risk to users, while providing them a warning code they can suppress:

[Obsolete ("While this type is public, Google considers it internal API and reserves the right to modify or delete it in the future. Use at your own risk."), DiagnosticId = "XAxxxx"]
public class AndroidX.AppCompat.Graphics.Drawable.DrawableWrapper { ... }

A potential wrinkle is a type/member cannot contain multiple [Obsolete] attributes, so we will have to guard against that if it is obsolete for another reason.

Another option would be to do essentially the same thing with our own custom attribute and a Roslyn analyzer.

@jpobst jpobst added enhancement Proposed change to current functionality generator Issues binding a Java library (generator, class-parse, etc.) labels Feb 2, 2023
jonpryor pushed a commit that referenced this issue Apr 20, 2023
Fixes: #1081

Context: xamarin/AndroidX#690

AndroidX has a [`@RestrictTo` annotation][0] that essentially means
"this API is implemented as `public`, but we do not consider it to be
supported API".  (It can be considered as the equivalent of
[C# `internal` visibility][1] or of [.NET preview features][2].)

This means that Google reserves the right to change such types at any
time -- and they have! -- which can in turn break our apps.
Because we simply bound the API as `public`, it misled our users
into believing that this is a stable API they can rely on.

For example, consider xamarin/AndroidX#690, in which
[`androidx.appcompat.appcomat-resources`][3] "broke" API between
[version 1.5.1][4] and [version 1.6.0][5], because Google
[decided to promote classes][6] which had been
`@RestrictTo({RestrictTo.Scope.LIBRARY_GROUP_PREFIX})` in
version 1.5.1 to become "real" `public` types, and in the process
*renamed* those types, e.g. `DrawableWrapper` became
`DrawableWrapperCompat`.  This renaming broke some of our customers.

Version 1.5.1:
![image](https://user-images.githubusercontent.com/179295/216440200-ff4f3bd3-1a86-419d-adff-9460976a6355.png)

	@RestrictTo(RestrictTo.Scope.LIBRARY_GROUP_PREFIX)
	public class DrawableWrapper extends Drawable …

Version 1.6.0:
![image](https://user-images.githubusercontent.com/179295/216440273-917a74a0-a1b5-4bdc-bb56-8615be89f18e.png)

	public class DrawableWrapperCompat extends Drawable …


The root problem is that we were binding `@RestrictTo` types which
never should have been bound.  Fixing this involves two steps:
(1) detection, and (2) mitigation.

Update `class-parse` so that the `@RestrictTo` annotation is now
supported.  If the `@RestrictTo` annotation is present, then a new
`//*/@annotated-visibility` attribute within `api.xml` will contain
the [`RestrictTo.Scope` values][7]

	<class
	    name='DrawableWrapper' 
	    …
	    annotated-visibility='LIBRARY_GROUP_PREFIX'
	/>

Update `Xamarin.Android.Tools.ApiXmlAdjuster.dll` so that the new
`annotated-visibility` is supported and "passed through".

These changes allow `generator` to know that `@RestrictTo` was used
and what it was applied to, which brings us to mitigation:

We can't *not* continue binding these types; removing these types
would be an API break.

Instead, *for now*, we will add an `[Obsolete]` attribute to the
affected API with a message describing the situation:

	[Obsolete (
	    "While this type is 'public', Google considers it internal API and reserves the right to modify or delete it in the future. Use at your own risk.",
	    DiagnosticId = "XAOBS001")[]
	public partial class DrawableWrapper {
	}

This uses a custom warning code XAOBS001, which allows users to use
`$(NoWarn)` to ignore these warnings, if necessary.

Additionally, this new "`[Obsolete]` on `@RestrictTo` types" behavior
is *off by default*, and only enabled via the new
`generator --lang-features=restrict-to-attributes` option.

Note that only one `[Obsolete]` is allowed on each type/member, so if
the API already has an `[Obsolete]` attribute, e.g. because it is
`@Deprecated`, then the XAOBS001 obsolete message will be skipped.

TODO: Enable `generator --lang-features=restrict-to-attributes` in
.NET Android Binding Projects, with an MSBuild property to disable
this option if necessary.

TODO: *Eventually* we'll also need a way to *not* bind these types.

[0]: https://developer.android.com/reference/androidx/annotation/RestrictTo
[1]: https://learn.microsoft.com/dotnet/csharp/language-reference/keywords/internal
[2]: https://github.com/dotnet/designs/blob/7d0be161bfb55117543f2833b645e089b646f8ab/accepted/2021/preview-features/preview-features.md
[3]: https://maven.google.com/web/index.html?q=androidx.appcompat#androidx.appcompat:appcompat-resources
[4]: https://dl.google.com/android/maven2/androidx/appcompat/appcompat-resources/1.5.1/appcompat-resources-1.5.1.aar
[5]: https://dl.google.com/android/maven2/androidx/appcompat/appcompat-resources/1.6.0/appcompat-resources-1.6.0.aar
[6]: https://android-review.googlesource.com/c/platform/frameworks/support/+/2120177
[7]: https://developer.android.com/reference/androidx/annotation/RestrictTo.Scope
jonpryor pushed a commit to dotnet/android that referenced this issue May 2, 2023
…7990)

Context: dotnet/java-interop#1081
Context: dotnet/java-interop@b274a67
Context: xamarin/AndroidX#690

Android libraries may use the [`androidx.annotation.RestrictTo`][0]
annotation to mark a `public` Java type as "not public":

	// Java
	@RestrictTo(RestrictTo.Scope.LIBRARY_GROUP_PREFIX)
	public class DrawableWrapper extends Drawable {
	}

Unfortunately, .NET Android didn't know about this annotation, so all
such annotated types were bound as *`public`* types:

	// C# binding
	public class DrawableWrapper : Drawable {
	}

This is a problem because Google doesn't maintain API compatibility
for types with the `@RestrictTo` annotation.  This can result in
undesirable API breakage; see also xamarin/AndroidX#690.

xamarin/java.interop#b274a67f updated `class-parse` to know about the
`@RestrictTo` annotation; when present, an `//*/@annotated-visibility`
attribute is present within `api.xml`:

	<class
	    name="DrawableWrapper"
	    …
	    annotated-visibility="LIBRARY_GROUP_PREFIX"
	/>

xamarin/java.interop#b274a67f also updated `generator` to support a
`generator --lang-features=restrict-to-attributes`; when present,
types with an `//*/@annotated-visibility` attribute will be marked
as `[Obsolete]` (*not* removed!), in order to maintain API
compatibility with existing ("broken") bindings, so that customers
can migrate away from these types:

	// C# binding
	[Obsolete
	    "While this type is 'public', Google considers it internal API and reserves the right to modify or delete it in the future. Use at your own risk.",
	    DiagnosticId = "XAOBS001")
	partial class DrawableWrapper : Drawable {
	}

The new `[Obsolete]` usage also specifies a custom warning code of
`XAOBS001` so that the warning can be suppressed if desired.

Add support for a new `$(AndroidEnableRestrictToAttributes)` MSBuild
"enum-style" property.  Supported values include:

  * `obsolete`: `generator --lang-features=restrict-to-attributes`
    will be used and the `[Obsolete]` custom attribute will be placed
    on bindings of Java types which have a `@RestrictTo` annotation.

    This is the default value.

  * `disable`: Java types with a `@RestrictTo` annotation
    will *not* be marked as `[Obsolete]`, and will be bound as a
    "normal" Java `public` type.

        <AndroidEnableRestrictToAttributes>disable</AndroidEnableRestrictToAttributes>

If you would instead prefer that types with `@RestrictTo` not be
bound at all, this can be achieved via Metadata, e.g.

	<remove-node path="//*[@annotated-visibility]" />

[0]: https://developer.android.com/reference/androidx/annotation/RestrictTo
jonathanpeppers pushed a commit to dotnet/android that referenced this issue Aug 15, 2023
…7990)

Context: dotnet/java-interop#1081
Context: dotnet/java-interop@b274a67
Context: xamarin/AndroidX#690

Android libraries may use the [`androidx.annotation.RestrictTo`][0]
annotation to mark a `public` Java type as "not public":

	// Java
	@RestrictTo(RestrictTo.Scope.LIBRARY_GROUP_PREFIX)
	public class DrawableWrapper extends Drawable {
	}

Unfortunately, .NET Android didn't know about this annotation, so all
such annotated types were bound as *`public`* types:

	// C# binding
	public class DrawableWrapper : Drawable {
	}

This is a problem because Google doesn't maintain API compatibility
for types with the `@RestrictTo` annotation.  This can result in
undesirable API breakage; see also xamarin/AndroidX#690.

xamarin/java.interop#b274a67f updated `class-parse` to know about the
`@RestrictTo` annotation; when present, an `//*/@annotated-visibility`
attribute is present within `api.xml`:

	<class
	    name="DrawableWrapper"
	    …
	    annotated-visibility="LIBRARY_GROUP_PREFIX"
	/>

xamarin/java.interop#b274a67f also updated `generator` to support a
`generator --lang-features=restrict-to-attributes`; when present,
types with an `//*/@annotated-visibility` attribute will be marked
as `[Obsolete]` (*not* removed!), in order to maintain API
compatibility with existing ("broken") bindings, so that customers
can migrate away from these types:

	// C# binding
	[Obsolete
	    "While this type is 'public', Google considers it internal API and reserves the right to modify or delete it in the future. Use at your own risk.",
	    DiagnosticId = "XAOBS001")
	partial class DrawableWrapper : Drawable {
	}

The new `[Obsolete]` usage also specifies a custom warning code of
`XAOBS001` so that the warning can be suppressed if desired.

Add support for a new `$(AndroidEnableRestrictToAttributes)` MSBuild
"enum-style" property.  Supported values include:

  * `obsolete`: `generator --lang-features=restrict-to-attributes`
    will be used and the `[Obsolete]` custom attribute will be placed
    on bindings of Java types which have a `@RestrictTo` annotation.

    This is the default value.

  * `disable`: Java types with a `@RestrictTo` annotation
    will *not* be marked as `[Obsolete]`, and will be bound as a
    "normal" Java `public` type.

        <AndroidEnableRestrictToAttributes>disable</AndroidEnableRestrictToAttributes>

If you would instead prefer that types with `@RestrictTo` not be
bound at all, this can be achieved via Metadata, e.g.

	<remove-node path="//*[@annotated-visibility]" />

[0]: https://developer.android.com/reference/androidx/annotation/RestrictTo
@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
enhancement Proposed change to current functionality generator Issues binding a Java library (generator, class-parse, etc.)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant