[api-merge] Fix for correctly calculating method level 'deprecated-since' attribute. #7645
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
|
Draft commit message: Fixes: https://github.com/xamarin/xamarin-android/issues/7558
Context: https://github.com/xamarin/monodroid/commit/16338f99568db892f37f6b5ddf927c139fcd4f50
Context: a073d99a838d15a2e4977fda63ecb1064a214d49
Context: 938c349725b677f1fa291a94f12546ef3d3c5a60
Currently when `api-merge` loops through type members, its first check
is to see if there is any existing member with the same name. If there
isn't, it can short-circuit, add the new member, and move to the next
one. If there is, we compare the existing API level member to the new
API level one to see if it has been marked deprecated in this level,
and if so we can add the `deprecated-since` attribute.
However, this initial existing member check was solely done on member
*name*, not signature. 😱
If the member is a method, a later check finds the exact matching
method with the same signature. We were performing the
`deprecated-since` logic against the first method with a matching name
rather than the method with the same signature.
For example, [`PackageManager.getPackageInfo(String, int)`][0], which
was deprecated in *API-33*. Yet our binding uses `[Obsolete]`, meaning
it has been deprecated since API-21 or before!
namespace Android.Content.PM;
partial class PackageManager {
[global::System.Obsolete (@"deprecated")]
[Register ("getPackageInfo", "(Ljava/lang/String;I)Landroid/content/pm/PackageInfo;", "GetGetPackageInfo_Ljava_lang_String_IHandler")]
public override unsafe Android.Content.PM.PackageInfo? GetPackageInfo (string packageName, [global::Android.Runtime.GeneratedEnum] Android.Content.PM.PackageInfoFlags flags) => …
}
This was wrong; it should instead have used `[ObsoletedOSPlatform]`:
namespace Android.Content.PM;
partial class PackageManager {
[global::System.Runtime.Versioning.ObsoletedOSPlatform ("android33.0")]
[Register ("getPackageInfo", "(Ljava/lang/String;I)Landroid/content/pm/PackageInfo;", "GetGetPackageInfo_Ljava_lang_String_IHandler")]
public override unsafe Android.Content.PM.PackageInfo? GetPackageInfo (string packageName, [global::Android.Runtime.GeneratedEnum] Android.Content.PM.PackageInfoFlags flags) => …
}
Move the `UpdateDeprecatedSince()` invocations to be called *after*
we have the method with the matching signature.
The `acceptable-breakages` changes reflect additional `[Obsolete]`
attributes that are now being converted to
`[ObsoletedOSPlatform ("android-XX.0")]` attributes.
[0]: https://developer.android.com/reference/android/content/pm/PackageManager#getPackageInfo(java.lang.String,%20int) |
Contributor
|
@jpobst: one thing that concerns me is that Shouldn't |
Contributor
Author
|
I verified that the package CI is building does include the new attributes:
Looking at the reference assembly we use for
Our reference assembly must not have been updated since these methods were marked as deprecated. |
grendello
added a commit
to grendello/xamarin-android
that referenced
this pull request
Jan 17, 2023
* main: [Xamarin.Android.Build.Tasks] skip XA1034 logic in some cases (dotnet#7680) [ci] Move OneLocBuild task to scheduled pipeline (dotnet#7679) [Mono.Android] ServerCertificateValidationCallback() and redirects (dotnet#7662) Bump to xamarin/Java.Interop/main@cf80deb7 (dotnet#7664) Localized file check-in by OneLocBuild (dotnet#7668) [api-merge] Correctly compute //method/@deprecated-since (dotnet#7645) [Xamarin.Android.Build.Tasks] _Microsoft.Android.Resource.Designer (dotnet#6427) [Xamarin.Android.Build.Tasks] downgrade d8/r8 `warning` messages to `info` (dotnet#7643) [Xamarin.Android.Build.Tasks] fix cases of missing `@(Reference)` (dotnet#7642) [Xamarin.Android.Build.Tasks] delay ToJniName calls in ManifestDocument (dotnet#7653) [Xamarin.Android.Build.Tasks] fast path for `<CheckClientHandlerType/>` (dotnet#7652)
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Context: #7558
Currently when
api-mergeloops through type members, its first check is to see if there is any existing member with the same name. If there isn't, it can short-circuit, add the new member, and move to the next one. If there is, we compare the existing API level member to the new API level one to see if it has been marked deprecated in this level, and if so we can add thedeprecated-sinceattribute.However, this initial existing member check was solely done on member name, not signature. If the member is a method, a later check finds the exact matching method with the same signature. We are performing the
deprecated-sincelogic against the first method with a matching name rather than the method with the same signature. Thus we are producing incorrect results.This commit moves
UpdateDeprecatedSinceto be called after we have the method with the matching signature.The
acceptable-breakageschanges reflect additional[Obsolete]attributes that are now being converted to[ObsoletedOSPlatform ("android-XX.0")]attributes.