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] Better support deprecated property getter/setters. #1062

Merged
merged 1 commit into from
Dec 14, 2022

Conversation

jpobst
Copy link
Contributor

@jpobst jpobst commented Dec 6, 2022

Fixes: #1033

When we are converting Java getter/setter pairs to C# properties, we can hit an interesting scenario where a getter may be @Deprecated and the setter is not, or vice versa:

  public boolean hasOptionsMenu () { ... }

  @Deprecated
  public void setHasOptionsMenu (boolean hasMenu) { ... }

C# has traditionally not allowed [Obsolete] to be placed on just a get or a set, it can only be placed on the entire property.

  [Obsolete]
  public bool HasOptionsMenu { get; set; }

This can lead to confusion because using the getter will report an obsolete warning when it is not obsolete. Thus, for properties, we only add [Obsolete] in 2 cases:

  • The get is obsolete and there is no set
  • Both the get and set are obsolete

We have this comment in our code:

// Unlike [Register], [Obsolete] cannot be put on property accessors, so we can apply them only under limited condition...

However, the compiler team has determined that preventing [Obsolete] on property accessors was a bug, and has fixed it in C# 8: dotnet/roslyn#32571.

Thus we can update generator to support scenarios in which only the Java getter or setter is marked as @Deprecated.

@jpobst jpobst marked this pull request as ready for review December 13, 2022 14:52
@jonpryor jonpryor merged commit f8d77fa into main Dec 14, 2022
@jonpryor jonpryor deleted the obsolete-properties branch December 14, 2022 22:13
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.

Handle properties with getter or setter only [Obsolete] attribute
2 participants