Skip to content

Commit

Permalink
[generator] Better support deprecated property getter/setters. (#1062)
Browse files Browse the repository at this point in the history 
Fixes: #1033

Context: dotnet/roslyn#32571

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

	class Example {
	  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` block; it could only be on the entire property:

	partial class Example {
	  [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:

 1. The `get` is obsolete and there is no `set`
 2. 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 C# compiler team has determined that preventing
`[Obsolete]` on property accessors was a bug, and has fixed it in C#8
via dotnet/roslyn#32571.

Update `generator` to support scenarios in which only the Java getter
or setter is marked as `@Deprecated`, by placing `[Obsolete]` on the
appropriate `get` or `set` block:

	partial class Example {
	    public bool HasOptionsMenu {
	        get => …

	        [Obsolete]
	        set =>…
	    }
	}
  • Loading branch information
@jpobst
jpobst committed Dec 14, 2022
1 parent 5e6209e commit f8d77fa
Show file tree
Hide file tree
Showing 3 changed files with 201 additions and 7 deletions.
164 changes: 162 additions & 2 deletions tests/generator-Tests/Unit-Tests/CodeGeneratorTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -565,7 +565,8 @@ public void ObsoletedOSPlatformAttributeSupport ()
Assert.True (writer.ToString ().Contains ("[global::System.Runtime.Versioning.ObsoletedOSPlatform (\"android25.0\", @\"This is a field deprecated since 25!\")]"), writer.ToString ());
Assert.True (writer.ToString ().Contains ("[global::System.Runtime.Versioning.ObsoletedOSPlatform (\"android25.0\", @\"This is a constructor deprecated since 25!\")]"), writer.ToString ());
Assert.True (writer.ToString ().Contains ("[global::System.Runtime.Versioning.ObsoletedOSPlatform (\"android25.0\", @\"This is a method deprecated since 25!\")]"), writer.ToString ());
Assert.True (writer.ToString ().Contains ("[global::System.Runtime.Versioning.ObsoletedOSPlatform (\"android25.0\", @\"This is a property getter deprecated since 25! This is a property setter deprecated since 25!\")]"), writer.ToString ());
Assert.True (writer.ToString ().Contains ("[global::System.Runtime.Versioning.ObsoletedOSPlatform (\"android25.0\", @\"This is a property getter deprecated since 25!\")]"), writer.ToString ());
Assert.True (writer.ToString ().Contains ("[global::System.Runtime.Versioning.ObsoletedOSPlatform (\"android25.0\", @\"This is a property setter deprecated since 25!\")]"), writer.ToString ());
}

[Test]
Expand Down Expand Up @@ -611,7 +612,152 @@ public void ObsoletedOSPlatformAttributeUnneededSupport ()
}

[Test]
[NonParallelizable] // We are setting a static property on Report
public void ObsoleteGetterOnlyProperty ()
{
var xml = @"<api>
<package name='java.lang' jni-name='java/lang'>
<class abstract='false' deprecated='not deprecated' final='false' name='Object' static='false' visibility='public' jni-signature='Ljava/lang/Object;' />
</package>
<package name='com.xamarin.android' jni-name='com/xamarin/android'>
<class abstract='false' deprecated='not deprecated' extends='java.lang.Object' extends-generic-aware='java.lang.Object' jni-extends='Ljava/lang/Object;' final='false' name='MyClass' static='false' visibility='public' jni-signature='Lcom/xamarin/android/MyClass;'>
<method abstract='false' deprecated='deprecated' final='false' name='getCount' jni-signature='()I' bridge='false' native='false' return='int' jni-return='I' static='false' synchronized='false' synthetic='false' visibility='public'></method>
</class>
</package>
</api>";

var gens = ParseApiDefinition (xml);
var iface = gens.Single (g => g.Name == "MyClass");

generator.Context.ContextTypes.Push (iface);
generator.WriteType (iface, string.Empty, new GenerationInfo ("", "", "MyAssembly"));
generator.Context.ContextTypes.Pop ();

// This should use [Obsolete] on the entire property because the getter is obsolete and there is no setter
Assert.True (StripRegisterAttributes (writer.ToString ()).NormalizeLineEndings ().Contains ("[global::System.Obsolete (@\"deprecated\")]public virtual unsafe int Count".NormalizeLineEndings ()), writer.ToString ());

// Ensure we don't write getter attribute
Assert.False (StripRegisterAttributes (writer.ToString ()).NormalizeLineEndings ().Contains ("[global::System.Obsolete(@\"deprecated\")]get"), writer.ToString ());
}

[Test]
public void ObsoletePropertyGetter ()
{
var xml = @"<api>
<package name='java.lang' jni-name='java/lang'>
<class abstract='false' deprecated='not deprecated' final='false' name='Object' static='false' visibility='public' jni-signature='Ljava/lang/Object;' />
</package>
<package name='com.xamarin.android' jni-name='com/xamarin/android'>
<class abstract='false' deprecated='not deprecated' extends='java.lang.Object' extends-generic-aware='java.lang.Object' jni-extends='Ljava/lang/Object;' final='false' name='MyClass' static='false' visibility='public' jni-signature='Lcom/xamarin/android/MyClass;'>
<method abstract='false' deprecated='deprecated' final='false' name='getCount' jni-signature='()I' bridge='false' native='false' return='int' jni-return='I' static='false' synchronized='false' synthetic='false' visibility='public'></method>
<method abstract='false' deprecated='not deprecated' final='false' name='setCount' jni-signature='(I)V' bridge='false' native='false' return='void' jni-return='V' static='false' synchronized='false' synthetic='false' visibility='public'>
<parameter name='count' type='int' jni-type='I'></parameter>
</method>
</class>
</package>
</api>";

var gens = ParseApiDefinition (xml);
var iface = gens.Single (g => g.Name == "MyClass");

generator.Context.ContextTypes.Push (iface);
generator.WriteType (iface, string.Empty, new GenerationInfo ("", "", "MyAssembly"));
generator.Context.ContextTypes.Pop ();

// This should use [Obsolete] on just the property getter since the setter is not obsolete
Assert.True (StripRegisterAttributes (writer.ToString ()).NormalizeLineEndings ().Contains ("[global::System.Obsolete(@\"deprecated\")]get"), writer.ToString ());
}

[Test]
public void ObsoletePropertySetter ()
{
var xml = @"<api>
<package name='java.lang' jni-name='java/lang'>
<class abstract='false' deprecated='not deprecated' final='false' name='Object' static='false' visibility='public' jni-signature='Ljava/lang/Object;' />
</package>
<package name='com.xamarin.android' jni-name='com/xamarin/android'>
<class abstract='false' deprecated='not deprecated' extends='java.lang.Object' extends-generic-aware='java.lang.Object' jni-extends='Ljava/lang/Object;' final='false' name='MyClass' static='false' visibility='public' jni-signature='Lcom/xamarin/android/MyClass;'>
<method abstract='false' deprecated='not deprecated' final='false' name='getCount' jni-signature='()I' bridge='false' native='false' return='int' jni-return='I' static='false' synchronized='false' synthetic='false' visibility='public'></method>
<method abstract='false' deprecated='deprecated' final='false' name='setCount' jni-signature='(I)V' bridge='false' native='false' return='void' jni-return='V' static='false' synchronized='false' synthetic='false' visibility='public'>
<parameter name='count' type='int' jni-type='I'></parameter>
</method>
</class>
</package>
</api>";

var gens = ParseApiDefinition (xml);
var iface = gens.Single (g => g.Name == "MyClass");

generator.Context.ContextTypes.Push (iface);
generator.WriteType (iface, string.Empty, new GenerationInfo ("", "", "MyAssembly"));
generator.Context.ContextTypes.Pop ();

// This should use [Obsolete] on just the property setter since the getter is not obsolete
Assert.True (StripRegisterAttributes (writer.ToString ()).NormalizeLineEndings ().Contains ("[global::System.Obsolete(@\"deprecated\")]set"), writer.ToString ());
}

[Test]
public void ObsoleteBothPropertyMethods ()
{
var xml = @"<api>
<package name='java.lang' jni-name='java/lang'>
<class abstract='false' deprecated='not deprecated' final='false' name='Object' static='false' visibility='public' jni-signature='Ljava/lang/Object;' />
</package>
<package name='com.xamarin.android' jni-name='com/xamarin/android'>
<class abstract='false' deprecated='not deprecated' extends='java.lang.Object' extends-generic-aware='java.lang.Object' jni-extends='Ljava/lang/Object;' final='false' name='MyClass' static='false' visibility='public' jni-signature='Lcom/xamarin/android/MyClass;'>
<method abstract='false' deprecated='getter_message' final='false' name='getCount' jni-signature='()I' bridge='false' native='false' return='int' jni-return='I' static='false' synchronized='false' synthetic='false' visibility='public'></method>
<method abstract='false' deprecated='setter_message' final='false' name='setCount' jni-signature='(I)V' bridge='false' native='false' return='void' jni-return='V' static='false' synchronized='false' synthetic='false' visibility='public'>
<parameter name='count' type='int' jni-type='I'></parameter>
</method>
</class>
</package>
</api>";

var gens = ParseApiDefinition (xml);
var iface = gens.Single (g => g.Name == "MyClass");

generator.Context.ContextTypes.Push (iface);
generator.WriteType (iface, string.Empty, new GenerationInfo ("", "", "MyAssembly"));
generator.Context.ContextTypes.Pop ();

// This should use [Obsolete] on both property methods because the deprecation messages are different
Assert.True (StripRegisterAttributes (writer.ToString ()).NormalizeLineEndings ().Contains ("[global::System.Obsolete(@\"getter_message\")]get"), writer.ToString ());
Assert.True (StripRegisterAttributes (writer.ToString ()).NormalizeLineEndings ().Contains ("[global::System.Obsolete(@\"setter_message\")]set"), writer.ToString ());
}

[Test]
public void ObsoleteEntireProperty ()
{
var xml = @"<api>
<package name='java.lang' jni-name='java/lang'>
<class abstract='false' deprecated='not deprecated' final='false' name='Object' static='false' visibility='public' jni-signature='Ljava/lang/Object;' />
</package>
<package name='com.xamarin.android' jni-name='com/xamarin/android'>
<class abstract='false' deprecated='not deprecated' extends='java.lang.Object' extends-generic-aware='java.lang.Object' jni-extends='Ljava/lang/Object;' final='false' name='MyClass' static='false' visibility='public' jni-signature='Lcom/xamarin/android/MyClass;'>
<method abstract='false' deprecated='deprecated' final='false' name='getCount' jni-signature='()I' bridge='false' native='false' return='int' jni-return='I' static='false' synchronized='false' synthetic='false' visibility='public'></method>
<method abstract='false' deprecated='deprecated' final='false' name='setCount' jni-signature='(I)V' bridge='false' native='false' return='void' jni-return='V' static='false' synchronized='false' synthetic='false' visibility='public'>
<parameter name='count' type='int' jni-type='I'></parameter>
</method>
</class>
</package>
</api>";

var gens = ParseApiDefinition (xml);
var iface = gens.Single (g => g.Name == "MyClass");

generator.Context.ContextTypes.Push (iface);
generator.WriteType (iface, string.Empty, new GenerationInfo ("", "", "MyAssembly"));
generator.Context.ContextTypes.Pop ();

// This should use [Obsolete] on the entire property because the getter and setter are both obsoleted with the same message
Assert.True (StripRegisterAttributes (writer.ToString ()).NormalizeLineEndings ().Contains ("[global::System.Obsolete (@\"deprecated\")]public virtual unsafe int Count".NormalizeLineEndings ()), writer.ToString ());

// Ensure we don't write getter/setter attributes
Assert.False (StripRegisterAttributes (writer.ToString ()).NormalizeLineEndings ().Contains ("[global::System.Obsolete(@\"deprecated\")]get"), writer.ToString ());
Assert.False (StripRegisterAttributes (writer.ToString ()).NormalizeLineEndings ().Contains ("[global::System.Obsolete(@\"deprecated\")]set"), writer.ToString ());
}

[Test]
[NonParallelizable] // We are setting a static property on Report
public void WarnIfTypeNameMatchesNamespace ()
{
var @class = new TestClass ("Object", "java.myclass.MyClass");
Expand Down Expand Up @@ -650,6 +796,20 @@ public void DontWarnIfNestedTypeNameMatchesNamespace ()
// The warning should not be raised if the nested type matches enclosing namespace
Assert.False (sb.ToString ().Contains ("warning BG8403"));
}

static string StripRegisterAttributes (string str)
{
// It is hard to test if the [Obsolete] is on the setter/etc due to the [Register], so remove all [Register]s
// [global::System.Obsolete (@"setter_message")]
// [Register ("setCount", "(I)V", "GetSetCount_IHandler")]
// set {
int index;

while ((index = str.IndexOf ("[Register", StringComparison.Ordinal)) > -1)
str = str.Substring (0, index) + str.Substring (str.IndexOf (']', index) + 1);

return str;
}
}

[TestFixture]
Expand Down
27 changes: 27 additions & 0 deletions tools/generator/Java.Interop.Tools.Generator.ObjectModel/Property.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,5 +60,32 @@ public void AutoDetectEnumifiedOverrideProperties (AncestorDescendantCache cache
}

public string ExplicitInterface => Getter?.ExplicitInterface ?? Setter?.ExplicitInterface;

public bool IsWholePropertyDeprecated {
get {
// If the getter isn't deprecated then the property isn't
if (Getter?.Deprecated is null)
return false;

// If the getter is deprecated and there is no setter then the property is deprecated
if (Setter is null)
return true;

// If the setter isn't deprecated then the property isn't
if (Setter.Deprecated is null)
return false;

// If the getter/setter deprecation messages differ, don't use whole property deprecation
if (Getter.Deprecated != Setter.Deprecated)
return false;

// If the getter/setter deprecation versions differ, don't use whole property deprecation
if (Getter.DeprecatedSince != Setter.DeprecatedSince)
return false;

// Getter/Setter deprecation is the same, use whole property deprecation
return true;
}
}
}
}
17 changes: 12 additions & 5 deletions tools/generator/SourceWriters/BoundProperty.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,11 +72,18 @@ public BoundProperty (GenBase gen, Property property, CodeGenerationOptions opt,
IsOverride = false;
}

// Unlike [Register], [Obsolete] cannot be put on property accessors, so we can apply them only under limited condition...
if (property.Getter.Deprecated != null && (property.Setter == null || property.Setter.Deprecated != null)) {
var message = property.Getter.Deprecated.Trim () + (property.Setter != null && property.Setter.Deprecated != property.Getter.Deprecated ? " " + property.Setter.Deprecated.Trim () : null);
var since = property.Getter?.DeprecatedSince ?? property.Setter?.DeprecatedSince;
SourceWriterExtensions.AddObsolete (Attributes, message, opt, deprecatedSince: since);
// Add [Obsolete] or [ObsoletedOSPlatform]
if (property.IsWholePropertyDeprecated) {
// This case applies [Obsolete] to the entire property
SourceWriterExtensions.AddObsolete (Attributes, property.Getter.Deprecated.Trim (), opt, deprecatedSince: property.Getter.DeprecatedSince);
} else {
// This case applies [Obsolete] to just the getter
if (property.Getter?.Deprecated != null)
SourceWriterExtensions.AddObsolete (GetterAttributes, property.Getter.Deprecated.Trim (), opt, deprecatedSince: property.Getter?.DeprecatedSince);

// This case applies [Obsolete] to just the setter
if (property.Setter?.Deprecated != null)
SourceWriterExtensions.AddObsolete (SetterAttributes, property.Setter.Deprecated.Trim (), opt, deprecatedSince: property.Setter?.DeprecatedSince);
}

SourceWriterExtensions.AddSupportedOSPlatform (Attributes, property.Getter, opt);
Expand Down

0 comments on commit f8d77fa

Please sign in to comment.