[generator] Better support deprecated property getter/setters. (#1062)

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 =>…
}
}

Author: jpobst

tests/generator-Tests/Unit-Tests/CodeGeneratorTests.cs

@@ -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]
@@ -611,7 +612,152 @@ public void ObsoletedOSPlatformAttributeUnneededSupport ()
 }
 
 [Test]
-[NonParallelizable]// We are setting a static property on Report
+publicvoidObsoleteGetterOnlyProperty()
+{
+varxml=@"<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>";
+
+vargens=ParseApiDefinition(xml);
+variface=gens.Single(g =>g.Name=="MyClass");
+
+generator.Context.ContextTypes.Push(iface);
+generator.WriteType(iface,string.Empty,newGenerationInfo("","","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]
+publicvoidObsoletePropertyGetter()
+{
+varxml=@"<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>";
+
+vargens=ParseApiDefinition(xml);
+variface=gens.Single(g =>g.Name=="MyClass");
+
+generator.Context.ContextTypes.Push(iface);
+generator.WriteType(iface,string.Empty,newGenerationInfo("","","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]
+publicvoidObsoletePropertySetter()
+{
+varxml=@"<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>";
+
+vargens=ParseApiDefinition(xml);
+variface=gens.Single(g =>g.Name=="MyClass");
+
+generator.Context.ContextTypes.Push(iface);
+generator.WriteType(iface,string.Empty,newGenerationInfo("","","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]
+publicvoidObsoleteBothPropertyMethods()
+{
+varxml=@"<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>";
+
+vargens=ParseApiDefinition(xml);
+variface=gens.Single(g =>g.Name=="MyClass");
+
+generator.Context.ContextTypes.Push(iface);
+generator.WriteType(iface,string.Empty,newGenerationInfo("","","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]
+publicvoidObsoleteEntireProperty()
+{
+varxml=@"<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>";
+
+vargens=ParseApiDefinition(xml);
+variface=gens.Single(g =>g.Name=="MyClass");
+
+generator.Context.ContextTypes.Push(iface);
+generator.WriteType(iface,string.Empty,newGenerationInfo("","","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
 publicvoidWarnIfTypeNameMatchesNamespace()
 {
 var@class=newTestClass("Object","java.myclass.MyClass");
@@ -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"));
 }
+
+staticstringStripRegisterAttributes(stringstr)
+{
+// 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 {
+intindex;
+
+while((index=str.IndexOf("[Register",StringComparison.Ordinal))>-1)
+str=str.Substring(0,index)+str.Substring(str.IndexOf(']',index)+1);
+
+returnstr;
+}
 }
 
 [TestFixture]

tools/generator/Java.Interop.Tools.Generator.ObjectModel/Property.cs

@@ -60,5 +60,32 @@ public void AutoDetectEnumifiedOverrideProperties (AncestorDescendantCache cache
 }
 
 publicstringExplicitInterface=>Getter?.ExplicitInterface??Setter?.ExplicitInterface;
+
+publicboolIsWholePropertyDeprecated{
+get{
+// If the getter isn't deprecated then the property isn't
+if(Getter?.Deprecatedisnull)
+returnfalse;
+
+// If the getter is deprecated and there is no setter then the property is deprecated
+if(Setterisnull)
+returntrue;
+
+// If the setter isn't deprecated then the property isn't
+if(Setter.Deprecatedisnull)
+returnfalse;
+
+// If the getter/setter deprecation messages differ, don't use whole property deprecation
+if(Getter.Deprecated!=Setter.Deprecated)
+returnfalse;
+
+// If the getter/setter deprecation versions differ, don't use whole property deprecation
+if(Getter.DeprecatedSince!=Setter.DeprecatedSince)
+returnfalse;
+
+// Getter/Setter deprecation is the same, use whole property deprecation
+returntrue;
+}
+}
 }
 }

tools/generator/SourceWriters/BoundProperty.cs

@@ -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)){
-varmessage=property.Getter.Deprecated.Trim()+(property.Setter!=null&&property.Setter.Deprecated!=property.Getter.Deprecated?" "+property.Setter.Deprecated.Trim():null);
-varsince=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);