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] Add support for @RestrictTo. #1094

Merged
merged 2 commits into from
Apr 20, 2023
Merged

[generator] Add support for @RestrictTo. #1094

merged 2 commits into from
Apr 20, 2023

Conversation

jpobst
Copy link
Contributor

@jpobst jpobst commented Apr 10, 2023

Fixes: #1081

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 dotnet/android-libraries#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

Support

We are going to support this by adding a new [Obsolete] attribute on the affected API with a message describing the situation. We will use the new ability in ObsoleteAttribute to provide a custom warning code. This will allow users to <NoWarn> the code if they wish to ignore all of these types of warnings.

[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")]

Changes

To accomplish this, the following changes were needed:

  • Update class-parse to look for the @RestrictTo annotation, and output its value as an annotated-visibility in api.xml.
  • Update xml-adjuster to support and pass through the annotated-visibility attribute.
  • Update generator to add the appropriate [Obsolete] attribute when needed.

Note that only one [Obsolete] attribute is allowed per type/member, so if the API already has a regular [Obsolete] attribute, we do not add the @RestrictTo information.

The generator changes are gated behind --lang-features=restrict-to-attributes. It is off by default. I propose we turn it on by default using MSBuild, but provide an opt-out MSBuild property in case a user wishes to opt out.

@jpobst jpobst marked this pull request as ready for review April 11, 2023 14:12
var annotations = attributes?.OfType<RuntimeInvisibleAnnotationsAttribute> ().FirstOrDefault ()?.Annotations;

if (annotations?.FirstOrDefault (a => a.Type == "Landroidx/annotation/RestrictTo;") is Annotation annotation)
return new XAttribute ("annotated-visibility", ((annotation.Values.FirstOrDefault ().Value as AnnotationElementArray)?.Values?.FirstOrDefault () as AnnotationElementEnum)?.ConstantName ?? string.Empty);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RestrictTo.getValue() returns an array of RestrictTo.Scope values; why pick just the first value instead of printing a -separated sequence of all of them?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think I realized it was an array. By the time I figured out that monstrosity of code needed to get a value I thought I had the only value. 😁

@jonpryor
Copy link
Member

jonpryor commented Apr 18, 2023

Draft commit message:

generator] Add support for @RestrictTo (#1094)

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

Context: https://github.com/xamarin/AndroidX/issues/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

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.

Support androidx.annotation.RestrictTo annotation
2 participants